Hide the tab indicators and the shelf when in immersive + tab fullscreen

Hide the tab indicators and the shelf when in immersive + tab fullscreen. 

BUG=234447, 233271
Test=ImmersiveModeControllerAshTest.TabAndBrowserFullscreen, 
     RootWindowController.GetFullscreenWindow

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

[pkotwicz, 2013-04-23 00:56:20]

I moved the CL from https://codereview.chromium.org/14156005/

James, can you please take another look?
Scott, can you please look at the changes in chrome/browser/fullscreen*.*

Review comments from: https://codereview.chromium.org/14156005/
Nice patch. A couple comments.

Can you add a test for the GetFullscreenWindow() function you added?
- Done. I moved the functionality being tested in
fullscreen_chromeos_unittest.cc (plus more) to root_window_controller_unittest
to avoid redundancy.

Do we still need kImmersiveModeKey?  I don't think we do - I think it was only
being used by RootWindowController to detect immersive mode, in spite of the
comment.
- You are right. I would however like to wait till
https://codereview.chromium.org/14195016/ lands to make that change.
I am optimistic, but I am unsure if the chrome_shell_delegate approach is the
one we will stick with because using the chrome_shell_delegate may result in
adding extra dependancies in chrome_shell_delegate to ImmersiveModeControllerAsh
which I think we want to avoid.

Optional: Consider re-introducing the LAYOUT_YES, LAYOUT_NO enum:
Done!

[James Cook, 2013-04-23 19:37:36]

On 2013/04/23 00:56:20, pkotwicz wrote:
> I moved the CL from https://codereview.chromium.org/14156005/
> 
> James, can you please take another look?
> Scott, can you please look at the changes in chrome/browser/fullscreen*.*
> 
> Review comments from: https://codereview.chromium.org/14156005/
> Nice patch. A couple comments.
> 
> Can you add a test for the GetFullscreenWindow() function you added?
> - Done. I moved the functionality being tested in
> fullscreen_chromeos_unittest.cc (plus more) to root_window_controller_unittest
> to avoid redundancy.
> 
> Do we still need kImmersiveModeKey?  I don't think we do - I think it was only
> being used by RootWindowController to detect immersive mode, in spite of the
> comment.
> - You are right. I would however like to wait till
> https://codereview.chromium.org/14195016/ lands to make that change.
> I am optimistic, but I am unsure if the chrome_shell_delegate approach is the
> one we will stick with because using the chrome_shell_delegate may result in
> adding extra dependancies in chrome_shell_delegate to
ImmersiveModeControllerAsh
> which I think we want to avoid.
> 
> Optional: Consider re-introducing the LAYOUT_YES, LAYOUT_NO enum:
> Done!

I'll be delayed for a bit, in the middle of a 27 stable blocker.

[James Cook, 2013-04-23 20:02:13]

https://codereview.chromium.org/14340007/diff/1/ash/root_window_controller_un...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/14340007/diff/1/ash/root_window_controller_un...
ash/root_window_controller_unittest.cc:375: // Test that GetFullscreenWindow()
returns a fullscreen window only if the
Hooray, a test!

https://codereview.chromium.org/14340007/diff/1/ash/wm/window_properties.cc
File ash/wm/window_properties.cc (right):

https://codereview.chromium.org/14340007/diff/1/ash/wm/window_properties.cc#n...
ash/wm/window_properties.cc:28: DEFINE_WINDOW_PROPERTY_KEY(bool,
kImmersiveModeKey, false);
I still think this key should go away. I don't like the ChromeShellDelegate ->
ImmersiveModeController dependency either, but there needs to be a single owner
of the "this browser is in immersive mode" information and right now that is
ImmersiveModeController.

If you feel strongly that this key is useful we should refactor
ImmersiveModeController so the key is the "owner" of the enabled/disabled state.

[pkotwicz, 2013-04-23 21:15:22]

James, I removed the key. Can you take another look?

[James Cook, 2013-04-23 21:33:04]

lgtm

[pkotwicz, 2013-04-25 04:00:27]

Scott, can you please take a look at the changes for
chrome/browser/fullscreen*.*

[sky, 2013-04-25 14:56:32]

https://codereview.chromium.org/14340007/diff/17001/chrome/browser/fullscreen...
File chrome/browser/fullscreen_chromeos.cc (right):

https://codereview.chromium.org/14340007/diff/17001/chrome/browser/fullscreen...
chrome/browser/fullscreen_chromeos.cc:11: aura::Window* active_window =
ash::wm::GetActiveWindow();
Again, don't we want this exact code for the win-aura when in metro mode?

[pkotwicz, 2013-04-25 16:38:52]

https://codereview.chromium.org/14340007/diff/17001/chrome/browser/fullscreen...
File chrome/browser/fullscreen_chromeos.cc (right):

https://codereview.chromium.org/14340007/diff/17001/chrome/browser/fullscreen...
chrome/browser/fullscreen_chromeos.cc:11: aura::Window* active_window =
ash::wm::GetActiveWindow();
We can't really go through the same code path either in Win Aura or Win Ash due
to the dependency of this code path on ash::internal::RootWindowController.
In metro mode fullscreen (correct me if I am wrong), we can't open transient
windows so the bug that IsFullscreenMode() does not return true when a transient
window is active does not seem to be present.

[sky, 2013-04-25 18:19:25]

https://codereview.chromium.org/14340007/diff/17001/chrome/browser/fullscreen...
File chrome/browser/fullscreen_chromeos.cc (right):

https://codereview.chromium.org/14340007/diff/17001/chrome/browser/fullscreen...
chrome/browser/fullscreen_chromeos.cc:11: aura::Window* active_window =
ash::wm::GetActiveWindow();
On 2013/04/25 16:38:52, pkotwicz wrote:
> We can't really go through the same code path either in Win Aura or Win Ash
due
> to the dependency of this code path on ash::internal::RootWindowController.
> In metro mode fullscreen (correct me if I am wrong), we can't open transient
> windows so the bug that IsFullscreenMode() does not return true when a
transient
> window is active does not seem to be present.

None-the-less we should share the code when possible, right? I would assume for
the win-aura side there is no controller but for win-ash there is, right? Seems
like this code path could handle both of those and then you use this code on
both win-aura/ash and chromeos.

[pkotwicz, 2013-04-26 19:03:10]

I simplified the code for fullscreen_chromeos and fullscreen_win. sky@, can you
please take another look?

[sky, 2013-04-26 19:57:40]

Much better, thank you. LGTM

[pkotwicz, 2013-04-27 04:17:37]

James, can you please take another look.
I moved the initialization of the window observers to the first call to
ImmersiveModeControllerAsh::SetEnabled() because FullscreenController is created
after BrowserView is created / inited.
(content::NotificationService::AllSource() == NULL which is why I did not catch
this in testing earlier)

[James Cook, 2013-04-29 16:50:01]

lgtm

[commit-bot: I haz the power, 2013-04-29 21:41:56]

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

[commit-bot: I haz the power, 2013-04-29 22:53:54]

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

[commit-bot: I haz the power, 2013-04-30 15:19:11]

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

[commit-bot: I haz the power, 2013-04-30 15:22:51]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-04-30 15:31:43]

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

[commit-bot: I haz the power, 2013-04-30 17:49:22]

Change committed as 197400