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

Implement BraveLocationBarView::ChildVisibilityChanged() #568

Closed
wants to merge 2 commits into from

Conversation

hferreiro
Copy link
Contributor

The BraveActionsContainer was not being shown when tearing off a tab
into a new window. This was due to BraveActionsContainer::Update() not
forcing a Layout() in BraveLocationBarView. This is now done by
implementing the aforementioned callback.

Fixes brave/brave-browser#1276.

  • Submitted a ticket for my issue if one did not already exist.

  • Used Github auto-closing keywords in the commit message.

  • Added/updated tests for this change (for new code or code which already has tests).

  • 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

  • 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

simonhong
simonhong previously approved these changes Oct 4, 2018
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

LGTM.

I think this approach seems fine.

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.

Can we call the base class impl as well here in case the base class ever adds something useful inside here and we don't want to miss it.

The `BraveActionsContainer` was not being shown when tearing off a tab
into a new window. This was due to `BraveActionsContainer::Update()` not
forcing a `Layout()` in `BraveLocationBarView`. This is now done by
implementing the aforementioned callback.

Fixes brave/brave-browser#1276.
@simonhong
Copy link
Member

simonhong commented Oct 4, 2018

@bbondy It calls base class Layout() methods as well.
Ah, I think you mean calling LocationBarView::ChildVisibilityChanged() like below. I think it would be good for future.

void BraveLocationBarView::ChildVisibilityChanged(views::View* child) {
  LocationBarView::ChildVisibilityChanged(child);

  if (child == brave_actions_)
    Layout();
}

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.

I added a patch for calling the base class, but this PR doesn't fully fix the problem.
Here's how it looks when you tear off:

screen shot 2018-10-04 at 11 54 26 am

Some padding goes away and it causes the count to be cut off.

@simonhong
Copy link
Member

I think we need some refactoring in this area.
For now, actions status are monitored by BraveActionsContainer and state update is done there.
With current implementation, this status update should be propagated to location bar or toolbar because this container view's bounds/visibility changing would affect their(toolbar/locaitonbar) layout.
So, I think whenever action's state is changed, this container re-layout should be triggered from location bar. WDYT? @petemill @hferreiro

@bbondy
Copy link
Member

bbondy commented Oct 11, 2018

I think this is a dupe and outdated PR, closing.

@bbondy bbondy closed this Oct 11, 2018
@bbondy bbondy deleted the issue/1276 branch October 14, 2018 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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