Close bubbles not on UpdateToolbar but when the activated tab is changed

Close bubbles not on UpdateToolbar but when the activated tab is changed

Now Browser::UpdateToolbar is called not only when the activated tab is
changed but also when, for example, a provisional document loader is
canceled. As a result of this, the Translate bubble is closed when a
navigation in an iframe happens. This is not an expected behavior. Apart
from a provisional document, I found some unexpected conditions where
the bubble is closed (e.g.
Browser::OnWebContentsInstantSupportDisabled calls UpdateToolbar).

This CL changes the logic to avoid closing the bubble on UpdateToolbar.
Instead, the logic is changed to close the bubble when the activated
tab is changed (Browser::ActivateTabChanged).

BUG=433272
TEST=manual

Committed: https://crrev.com/76a935cd3b88e675746a8299bd222ac5cdcf402f
Cr-Commit-Position: refs/heads/master@{#304990}

[hajimehoshi, 2014-11-19 08:20:10]

hajimehoshi@chromium.org changed reviewers:
+ pkasting@chromium.org, sky@chromium.org

[hajimehoshi, 2014-11-19 08:20:26]

PTAL

[Peter Kasting, 2014-11-19 09:18:12]

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1018: window_->CloseTranslateBubble();
Why do we need a special call for this?  Why can't this be done in
BrowserWindow::OnActiveTabChanged()?

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:974:
TranslateBubbleView::CloseBubble();
What about the other statements here, e.g. ZoomBubbleView::CloseBubble()? 
Wouldn't that be wrong too?  Can the other updating functions here do the wrong
things as well?

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1157:
TranslateBubbleView::CloseBubble();
Does this close the bubble when the main frame is navigated?  (I presume yes,
otherwise main frame navigations wouldn't close the bubble at all, which seems
wrong)

[hajimehoshi, 2014-11-19 10:02:08]

Thank you!

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:1018: window_->CloseTranslateBubble();
On 2014/11/19 09:18:12, Peter Kasting wrote:
> Why do we need a special call for this?  Why can't this be done in
> BrowserWindow::OnActiveTabChanged()?

Ah, you're right. Done.

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:974:
TranslateBubbleView::CloseBubble();
On 2014/11/19 09:18:12, Peter Kasting wrote:
> What about the other statements here, e.g. ZoomBubbleView::CloseBubble()? 
> Wouldn't that be wrong too?  Can the other updating functions here do the
wrong
> things as well?

As to the zoom bubble, maybe yes, and I can update this. I'm not sure about the
other functions.

I said 'maybe' because you said this was the right place to close the zoom
bubble before (https://codereview.chromium.org/425223003). But if my findings
are right and new to you, we should move updating the zoom bubble.

BTW, being different from the Translate bubble, the zoom bubble is closed after
several seconds automatically in some cases, so I suppose users don't notice
that the zoom bubble is closed by non-navigation events.

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/739793003/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1157:
TranslateBubbleView::CloseBubble();
On 2014/11/19 09:18:12, Peter Kasting wrote:
> Does this close the bubble when the main frame is navigated?  (I presume yes,
> otherwise main frame navigations wouldn't close the bubble at all, which seems
> wrong)

Yes. When the main frame is navigated, |enabled| should be false.

[Peter Kasting, 2014-11-19 19:23:20]

Yeah, let's move the zoom bubble closure, I wasn't thinking about subframe
navigations.

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:811: if (change_tab_contents) {
Do we need to guard the calls below under this conditional?  I'm not precisely
sure when this function gets called with this variable false -- maybe for
dragging a tab out of the window?

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:973:
ZoomBubbleView::CloseBubble();
You deleted this, but didn't move it to anywhere else.

[hajimehoshi, 2014-11-20 05:12:30]

Thank you!

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:811: if (change_tab_contents) {
On 2014/11/19 19:23:20, Peter Kasting wrote:
> Do we need to guard the calls below under this conditional?  I'm not precisely
> sure when this function gets called with this variable false -- maybe for
> dragging a tab out of the window?

You added this variable?: https://codereview.chromium.org/23536075

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (left):

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:973:
ZoomBubbleView::CloseBubble();
On 2014/11/19 19:23:20, Peter Kasting wrote:
> You deleted this, but didn't move it to anywhere else.

Oops, sorry. Moved like Translate bubble. Done.

[Peter Kasting, 2014-11-20 06:27:44]

LGTM

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/739793003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:811: if (change_tab_contents) {
On 2014/11/20 05:12:30, hajimehoshi wrote:
> On 2014/11/19 19:23:20, Peter Kasting wrote:
> > Do we need to guard the calls below under this conditional?  I'm not
precisely
> > sure when this function gets called with this variable false -- maybe for
> > dragging a tab out of the window?
> 
> You added this variable?: https://codereview.chromium.org/23536075

That in no way means I recall why I did!  :)

Seriously though, my guess is maybe it has to do with tab dragging or something.
 If tab drags seem to work like they should w.r.t. your bubble, let's not worry
about them.

https://codereview.chromium.org/739793003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/739793003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1156: 
Nit: I probably wouldn't add this blank line

[hajimehoshi, 2014-11-20 06:48:27]

Thanks!

https://codereview.chromium.org/739793003/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/739793003/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1156: 
On 2014/11/20 06:27:44, Peter Kasting wrote:
> Nit: I probably wouldn't add this blank line

Done.

[hajimehoshi, 2014-11-20 06:48:33]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-11-20 06:49:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/739793003/60001

[commit-bot: I haz the power, 2014-11-20 07:31:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-11-20 07:32:37]

Patchset 4 (id:??) landed as
https://crrev.com/76a935cd3b88e675746a8299bd222ac5cdcf402f
Cr-Commit-Position: refs/heads/master@{#304990}