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

Conversation

hferreiro
Copy link
Contributor

When a BraveAction is added, the size change isn't properly notified
to BraveLocationBarView. This commit implements
ChildPreferredSizeChanged() in both BraveActionsContainer and
BraveLocationBarView, and makes sure the action's preferred size is
set after calling AddChildView().

Fixes brave/brave-browser#1276

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • [ x Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

When a `BraveAction` is added, the size change isn't properly notified
to `BraveLocationBarView`. This commit implements
`ChildPreferredSizeChanged()` in both `BraveActionsContainer` and
`BraveLocationBarView`, and makes sure the action's preferred size is
set after calling `AddChildView()`.

Fixes brave/brave-browser#1276
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.

@@ -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.

@simonhong
Copy link
Member

I assume this problem could be reproduced when action is removed during the runtime.
I think this PR only fixes about actions is added. WDYT?

Copy link
Member

@bbondy bbondy left a comment

Choose a reason for hiding this comment

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

As I understand, as long as the extensions do not get unloaded, which they don't, then this is ok. So going to merge and approve.

@bbondy bbondy merged commit f13d267 into master Oct 11, 2018
bbondy added a commit that referenced this pull request Oct 11, 2018
Forward preferred size changes from `BraveActionsContainer`
bbondy added a commit that referenced this pull request Oct 11, 2018
Forward preferred size changes from `BraveActionsContainer`
@bbondy
Copy link
Member

bbondy commented Oct 11, 2018

master: f13d267
0.56.x: 78d00c3
0.55.x: c2b33d6

@bsclifton bsclifton deleted the issue/1276-2 branch October 14, 2018 05:15
@bbondy bbondy added this to the 0.55.x - Release milestone Jan 14, 2019
fmarier pushed a commit that referenced this pull request Oct 29, 2019
Skipping 12 because it was used for some ad-hoc testing, also 13 == good luck
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tear off tab doesn't show shields until URL bar is selected
3 participants