Add handling for immersive fullscreen to the zoom bubble

This CL
- Anchors the zoom bubble to the magnifying glass when the zoom bubble is visible and the top-of-window view are revealed in immersive fullscreen. The top-of-window views stay revealed as long as the zoom bubble is visible.
- Positions the zoom bubble in the top right when in immersive fullscreen and the top-of-window views are not already revealed. For the sake of simplicity the zoom bubble is closed if the user reveals the top-of-window views.

BUG=181062
TEST=Manual, see bug

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208008

[pkotwicz, 2013-06-15 23:32:58]

James, PTAL

[James Cook, 2013-06-17 16:13:41]

How hard would it be to write some kind of test for this?

https://codereview.chromium.org/16998006/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/immersive_mode_controller.h (right):

https://codereview.chromium.org/16998006/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/immersive_mode_controller.h:47: virtual void
OnStartedImmersiveReveal() {}
nit: OnImmersiveRevealStarted()?  Might be a bit more similar to
FullscreenStateChanged() below, etc.

https://codereview.chromium.org/16998006/diff/6001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.h (right):

https://codereview.chromium.org/16998006/diff/6001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.h:287:
base::WeakPtrFactory<ImmersiveModeControllerAsh> weak_ptr_factory_;
Good catch that weak_prt_factory_ should be last.

https://codereview.chromium.org/16998006/diff/6001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/16998006/diff/6001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:101: BrowserView*
browser_view)
Can you just pass in immersive mode controller (and maybe fullscreen controller)
here?

[pkotwicz, 2013-06-18 05:49:48]

James, can you please take another look?

[pkotwicz, 2013-06-18 13:57:04]

I was really tempted to just go with a notification. I made
ImmersiveModeController non-virtual so that every implementor (including the
mock ImmersiveModeControllers) does not need not implement notifying its
observers about destruction.

[James Cook, 2013-06-18 17:03:38]

LGTM with a couple optional comments

https://codereview.chromium.org/16998006/diff/20001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller.cc (right):

https://codereview.chromium.org/16998006/diff/20001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller.cc:11:
FOR_EACH_OBSERVER(Observer, observers_, OnImmersiveModeControllerDestroyed());
This seems fine.

https://codereview.chromium.org/16998006/diff/20001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/16998006/diff/20001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc:23: }
optional nit: These could go on one line.

https://codereview.chromium.org/16998006/diff/20001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc:28:
ImmersiveFullscreenConfiguration::EnableImmersiveFullscreenForTest();
It might be clearer to call this at the beginning of the ImmersiveFullscreen
test below, so you wouldn't have to explain in NonImmersiveFullscreen why the
setting doesn't affect the test.

https://codereview.chromium.org/16998006/diff/20001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc:125: }
Nice tests.

[pkotwicz, 2013-06-18 23:25:46]

Scott, can you please take a look at the changes in
chrome/browser/ui/views/location_bar?

https://codereview.chromium.org/16998006/diff/20001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/16998006/diff/20001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc:28:
ImmersiveFullscreenConfiguration::EnableImmersiveFullscreenForTest();
We would need to call for
ImmersiveFullscreenConfiguration::DisableImmersiveFullscreenForTest() in
ZoomBubbleBrowserTest.NonImmersiveFullscreen.
I think the comment about tab fullscreen is still valuable. Otherwise, someone
may think that we can ifdef out ZoomBubbleBrowserTest.NonImmersiveFullscreen for
CrOS.

[pkotwicz, 2013-06-19 02:18:53]

Scott, can you please take a look?

[pkotwicz, 2013-06-19 02:19:31]

In particular, can you review the changes in
chrome/browser/ui/views/location_bar?

[sky, 2013-06-19 13:32:29]

LGTM

https://codereview.chromium.org/16998006/diff/34001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller.h (right):

https://codereview.chromium.org/16998006/diff/34001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller.h:45: virtual
~Observer() {}
Make this protected.

https://codereview.chromium.org/16998006/diff/34001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc (right):

https://codereview.chromium.org/16998006/diff/34001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc:938: if
(previous_reveal_state == CLOSED) {
nit: no {}

[commit-bot: I haz the power, 2013-06-21 03:03:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16998006/44001

[commit-bot: I haz the power, 2013-06-21 04:13:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16998006/59001

[commit-bot: I haz the power, 2013-06-21 06:44:45]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-06-21 14:47:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16998006/74001

[commit-bot: I haz the power, 2013-06-21 17:37:59]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-06-21 19:46:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16998006/86001

[commit-bot: I haz the power, 2013-06-22 02:34:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16998006/86001

[commit-bot: I haz the power, 2013-06-22 03:02:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16998006/86001

[commit-bot: I haz the power, 2013-06-22 03:31:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16998006/86001

[commit-bot: I haz the power, 2013-06-22 03:40:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/16998006/86001

[commit-bot: I haz the power, 2013-06-22 06:55:33]

Change committed as 208008