Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward preferred size changes from BraveActionsContainer #587

Merged
merged 1 commit into from
Oct 11, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions browser/ui/views/brave_actions/brave_actions_container.cc
Original file line number Diff line number Diff line change
Expand Up @@ -105,18 +105,20 @@ void BraveActionsContainer::AddAction(const extensions::Extension* extension,
// The button view
actions_[id].view_ = std::make_unique<ToolbarActionView>(
actions_[id].view_controller_.get(), this);
// we control destruction
actions_[id].view_->set_owned_by_client();
// Sets overall size of button but not image graphic. We set a large width
// in order to give space for the bubble.
actions_[id].view_->SetPreferredSize(gfx::Size(32, 24));
// Add extension view after separator view
// `AddChildView` should be called first, so that changes that modify
// layout (e.g. preferred size) are forwarded to its parent
if (actions_[id].position_ != ACTION_ANY_POSITION) {
DCHECK(actions_[id].position_ > 0);
AddChildViewAt(actions_[id].view_.get(), actions_[id].position_);
} else {
AddChildView(actions_[id].view_.get());
}
// we control destruction
actions_[id].view_->set_owned_by_client();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think this relocation isn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, actually I just moved the code calling AddChildView() so that it happens just after creation. It's just that git diff shows it the other way around.

// Sets overall size of button but not image graphic. We set a large width
// in order to give space for the bubble.
actions_[id].view_->SetPreferredSize(gfx::Size(32, 24));
Update();
}
}
Expand Down Expand Up @@ -244,3 +246,7 @@ void BraveActionsContainer::OnExtensionActionUpdated(
UpdateActionState(extension_action->extension_id());
}
// end ExtensionActionAPI::Observer

void BraveActionsContainer::ChildPreferredSizeChanged(views::View* child) {
PreferredSizeChanged();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of calling PreferredSizeChanged(), just calling parent()->Layout() would be more explicit.
With this, we don't need to modify BraveLocationBarView.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I think it is more clean this way. In the future, we may want to do other things instead of or besides calling Layout() in BraveLocationBarView::ChildPreferredSizeChanged(). Or we may have other BraveLocationBarView's children needing to do that. Also, this code is independent of the BraveActionsContainer's parent. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I got what you mean.

}
3 changes: 3 additions & 0 deletions browser/ui/views/brave_actions/brave_actions_container.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ class BraveActionsContainer : public views::View,
content::WebContents* web_contents,
content::BrowserContext* browser_context) override;

// views::View:
void ChildPreferredSizeChanged(views::View* child) override;

private:
// Special positions in the container designators
enum ActionPosition : int {
Expand Down
7 changes: 7 additions & 0 deletions browser/ui/views/location_bar/brave_location_bar_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,13 @@ void BraveLocationBarView::OnThemeChanged() {
RefreshBackground();
}

void BraveLocationBarView::ChildPreferredSizeChanged(views::View* child) {
if (child != brave_actions_)
return;

Layout();
}

// Provide base class implementation for Update override that has been added to
// header via a patch. This should never be called as the only instantiated
// implementation should be our |BraveLocationBarView|.
Expand Down
3 changes: 3 additions & 0 deletions browser/ui/views/location_bar/brave_location_bar_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,11 @@ class BraveLocationBarView : public LocationBarView {
void Layout() override;
void Update(const content::WebContents* contents) override;
void OnChanged() override;

// views::View:
gfx::Size CalculatePreferredSize() const override;
void OnThemeChanged() override;
void ChildPreferredSizeChanged(views::View* child) override;

private:
void UpdateBookmarkStarVisibility() override;
Expand Down