No need to suppress ZoomBubble if !window_->IsActive().

No need to suppress ZoomBubble if !window_->IsActive().

In Browser::OnZoomChanged() we currently filter on both (1) the event
having been generated by the active webcontents, and (2) the browser
window being active.

However, the window can receive <ctrl>-<scrollwheel> events even when
it is not active, and these will result in zoom changing without a zoom
bubble being shown.

This CL removes the check for window_->IsActive() so that the zoom
bubble always shows if the zoom changes for the active web contents by
a means other than the wrench menu.

TEST= Open chrome, make window inactive, hover over window and zoom
      with the mouse scroll-wheel; the zoom bubble should appear.
BUG=459322
Committed: https://crrev.com/ff3d0f6b28841ec02c2d3555ce0d819fc18fa585
Cr-Commit-Position: refs/heads/master@{#318089}

[wjmaclean, 2015-02-18 19:42:29]

*tiny* CL - PTAL?

[wjmaclean, 2015-02-18 19:49:43]

wjmaclean@chromium.org changed reviewers:
+ thestig@chromium.org

[wjmaclean, 2015-02-18 19:50:32]

*tiny* CL - PTAL? (re-sent since I didn't have you added as reviewer the first
time ...)

[Lei Zhang, 2015-02-18 20:35:02]

lgtm

- I changed "we contents" to "webcontents" in the CL description.
- It would be nice if we can add a test for this.

[wjmaclean, 2015-02-18 21:54:45]

On 2015/02/18 20:35:02, Lei Zhang (Soon to be OOO) wrote:
> lgtm
> 
> - I changed "we contents" to "webcontents" in the CL description.
> - It would be nice if we can add a test for this.

I'll look into devising a test ... if it looks like the test will take a while
to figure out I may land this and file a bug for creating a test after the fact.

[wjmaclean, 2015-02-18 22:02:43]

+ rihitrao@ and erikchen@

This rather simple looking CL seems to be causing
ZoomDecorationTest.BubbleAtDefaultZoom to fail at the following line:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

This seems odd, since a call has been made to zoom_decoration->CloseBubble().

Could this be related to the way animations are handled?

[erikchen, 2015-02-18 22:10:58]

On 2015/02/18 22:02:43, wjmaclean wrote:
> + rihitrao@ and erikchen@
> 
> This rather simple looking CL seems to be causing
> ZoomDecorationTest.BubbleAtDefaultZoom to fail at the following line:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> This seems odd, since a call has been made to zoom_decoration->CloseBubble().
> 
> Could this be related to the way animations are handled?

Close() has an animation, and SetVisible(false) gets called from the callback
after the animation is finished: OnClosed(). That's gated by the conditional "if
(IsAtDefaultZoom() && IsVisible())". I'm guessing that SetVisible(false) should
not depend on IsAtDefaultZoom().

[hcarmona, 2015-02-20 00:45:33]

hcarmona@chromium.org changed reviewers:
+ hcarmona@chromium.org

[hcarmona, 2015-02-20 00:45:34]

I wanted to comment on this because I looked at this issue yesterday and
wanted to share something I noticed. There's some weird behavior that
happens if you have the same page open on two windows. Zooming on the
background window will cause the zoom bubble to appear on the top
window.

This change introduces a similar issue but the zoom bubble will now
appear on the newest window whether it's active or not.

If you add a new data member to ZoomChangedEventData that is true if the
zoom controller is the origin, then you can have the bubble show on the
window that initiated the zoom event. ZoomController::UpdateState can
differentiate whether the event came from the same controller or another
when it checks for event_data_, this might be the place to set this
value.

[wjmaclean, 2015-02-20 15:47:08]

On 2015/02/20 00:45:34, Hector Carmona wrote:
> I wanted to comment on this because I looked at this issue yesterday and
> wanted to share something I noticed. There's some weird behavior that
> happens if you have the same page open on two windows. Zooming on the
> background window will cause the zoom bubble to appear on the top
> window.
> 
> This change introduces a similar issue but the zoom bubble will now
> appear on the newest window whether it's active or not.
> 
> If you add a new data member to ZoomChangedEventData that is true if the
> zoom controller is the origin, then you can have the bubble show on the
> window that initiated the zoom event. ZoomController::UpdateState can
> differentiate whether the event came from the same controller or another
> when it checks for event_data_, this might be the place to set this
> value.

Simpler still, just don't set "can_show_bubble" true on any event created in
UpdateState() that was not a zoom change originated by the controller. That way
we don't need to add anything to ZoomChangedEventData.

[wjmaclean, 2015-02-20 15:53:03]

On 2015/02/20 15:47:08, wjmaclean wrote:
> On 2015/02/20 00:45:34, Hector Carmona wrote:
> > 
> > This change introduces a similar issue but the zoom bubble will now
> > appear on the newest window whether it's active or not.

Worth noting: this behaviour exists because the actual zoom bubble is a
singleton, and the event gets sent to windows in the order they were created, so
the last (newest) window grabs the singleton for itself.

[wjmaclean, 2015-02-20 15:55:47]

New patchsets have been uploaded after l-g-t-m from thestig@chromium.org

[wjmaclean, 2015-02-20 15:58:46]

@hcarmona - btw, thanks for pointing out the two window behaviour, I hadn't
noticed it before. :-)

[wjmaclean, 2015-02-20 16:01:41]

@erikchen -

You're likely right that the SetVisible(false) shouldn't be gated on
IsDefaultZoom(), though that doesn't fix this test result.

Question: if there's an animation involved, I'm surprised that the test doesn't
have some sort of observer to wait for the animation to finish before checking
the (new) visibility status: should it? Or is there some other behaviour here
I'm missing.

[wjmaclean, 2015-02-20 22:16:23]

On further investigation, I realize that this test *was relying on the bubble
not being shown* via the usual pathways (presumably on account of the browser
window not being active during the test). The bubble is only shown when
ShowBubble(false) is called (false => the bubble will not auto-close).

With the change in this CL, the bubble gets shown automatically a number of
times when zoom-in is called. In these cases the animation time is non-zero, and
it upsets the expectations.

[wjmaclean, 2015-02-24 21:09:53]

wjmaclean@chromium.org changed reviewers:
+ erikchen@chromium.org, rohitrao@chromium.org

[wjmaclean, 2015-02-24 21:10:21]

erikchecn@ and rohitrao@ - PTAL ...

[erikchen, 2015-02-24 22:41:04]

https://codereview.chromium.org/940673002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm
(right):

https://codereview.chromium.org/940673002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:90:
GetLocationBar()->GetWebContents())->SetShowsNotificationBubble(false);
Should this be moved to SetUpOnMainThread? It seems like your comment would
apply to all tests in this test suite.

[wjmaclean, 2015-02-25 15:08:24]

https://codereview.chromium.org/940673002/diff/60001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm
(right):

https://codereview.chromium.org/940673002/diff/60001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/location_bar/zoom_decoration_browsertest.mm:90:
GetLocationBar()->GetWebContents())->SetShowsNotificationBubble(false);
On 2015/02/24 22:41:04, erikchen wrote:
> Should this be moved to SetUpOnMainThread? It seems like your comment would
> apply to all tests in this test suite.

It can't go in SetupOnMainThread, since ZoomControllers are tied to specific
WebContents, and we don't know what the WebContent(s) will be at that point. It
could potentially go in the Zoom() function, but given that this is a
work-around behaviour it would be nice to limit its use to only those tests that
actually require it to pass.

[erikchen, 2015-02-25 18:09:11]

lgtm

[wjmaclean, 2015-02-25 18:15:47]

wjmaclean@chromium.org changed reviewers:
+ fsamuel@chromium.org

[wjmaclean, 2015-02-25 18:16:24]

fsamuel@ - can you please do a non-owner's review on zoom_controller.cc (tiny
CL)

[rohitrao (ping after 24h), 2015-02-25 18:28:01]

ui/cocoa LGTM

https://codereview.chromium.org/940673002/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/940673002/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1941: // Only show the zoom bubble for zoom changes
in the active window.
Is this comment now incorrect?

https://codereview.chromium.org/940673002/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1942:
window_->ZoomChangedForActiveTab(data.can_show_bubble);
Is it still correct for this method to have "ForActiveTab" in its name?

[Fady Samuel, 2015-02-25 18:30:01]

lgtm

[wjmaclean, 2015-02-25 18:40:12]

https://codereview.chromium.org/940673002/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/940673002/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1941: // Only show the zoom bubble for zoom changes
in the active window.
On 2015/02/25 18:28:01, rohitrao wrote:
> Is this comment now incorrect?

Ooops, yes ... I'll remove it.

https://codereview.chromium.org/940673002/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1942:
window_->ZoomChangedForActiveTab(data.can_show_bubble);
On 2015/02/25 18:28:01, rohitrao wrote:
> Is it still correct for this method to have "ForActiveTab" in its name?

I think so, though it is an unfortunate coincidence in the naming ... a tab can
be active without its window being active (by the logic that otherwise
GetActiveWebContents() would return null). It seems to be referring to the
"currently visible tab".

[wjmaclean, 2015-02-25 18:40:29]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2015-02-25 18:40:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, fsamuel@chromium.org, erikchen@chromium.org,
rohitrao@chromium.org
Link to the patchset: https://codereview.chromium.org/940673002/#ps120001
(title: "Re-remove stale comment.")

[commit-bot: I haz the power, 2015-02-25 18:41:53]

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

[commit-bot: I haz the power, 2015-02-25 19:38:54]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-25 19:39:52]

Patchset 7 (id:??) landed as
https://crrev.com/ff3d0f6b28841ec02c2d3555ce0d819fc18fa585
Cr-Commit-Position: refs/heads/master@{#318089}