ash/immersive mode: Hide the launcher when entering immersive mode

ash/immersive mode: Hide the launcher when entering immersive mode

* Set shelf to auto-hide when enabling immersive mode
* Directly access ash::Shell for auto-hide so we don't overwrite
  the user's preferences

BUG=168592
TEST=added to browser_tests ImmersiveModeController


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

[James Cook, 2013-01-09 00:13:14]

sky, PTAL. This is a feature request from Ben that seems like a good idea to me.

[sky, 2013-01-09 00:52:37]

This makes me nervous. How do you know someone else hasn't tried to change the
auto-hide behavior while immersive?

[James Cook, 2013-01-09 03:49:50]

On 2013/01/09 00:52:37, sky wrote:
> This makes me nervous. How do you know someone else hasn't tried to change the
> auto-hide behavior while immersive?

Does the idea for the feature make you nervous, or the implementation?

Now that you point it out, I think we don't want to let the user change the
launcher auto-hide preference while in immersive mode.  Perhaps a better way to
do implement this feature would be:

* Make the shell/launcher aware of the state of immersive mode
* Do auto-hiding of the launcher both when the user has the pref set and then
we're in immersive mode
* Hide or disable the "Autohide launcher" context menu item when in immersive
mode

What do you think?

[sky, 2013-01-09 17:18:30]

On Tue, Jan 8, 2013 at 7:49 PM,  <jamescook@chromium.org> wrote:
> On 2013/01/09 00:52:37, sky wrote:
>>
>> This makes me nervous. How do you know someone else hasn't tried to change
>> the
>> auto-hide behavior while immersive?
>
>
> Does the idea for the feature make you nervous, or the implementation?

Just the implementation.

> Now that you point it out, I think we don't want to let the user change the
> launcher auto-hide preference while in immersive mode.  Perhaps a better way
> to
> do implement this feature would be:
>
> * Make the shell/launcher aware of the state of immersive mode
> * Do auto-hiding of the launcher both when the user has the pref set and
> then
> we're in immersive mode
> * Hide or disable the "Autohide launcher" context menu item when in
> immersive
> mode
>
> What do you think?

This seems reasonable, but you might run it by the ux folks for their input.

  -Scott

>
>
> https://codereview.chromium.org/11830008/

[James Cook, 2013-01-09 17:28:38]

> This seems reasonable, but you might run it by the ux folks for their input.

OK. I ran it by Alex who thinks this approach is reasonable and that we should
hide the "Autohide launcher" menu item while in this state.  I'll go down that
path.  Thanks!

[James Cook, 2013-01-09 21:55:04]

Sky, PTAL.  While it touches more files, I think this is cleaner.  I had to
touch the BrowserWindow interface to allow communication from
c/b/ui/ash/chrome_shell_delegate.cc into c/b/ui/views/frame/browser_view.cc

[sky, 2013-01-09 22:24:38]

https://codereview.chromium.org/11830008/diff/6001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/11830008/diff/6001/ash/shell_delegate.h#newco...
ash/shell_delegate.h:163: virtual bool IsImmersiveMode() const = 0;
Could we instead use a property on the window for this? Such an approach is less
invasive and more in line with how we deal with similar things (such as full
screen).

[James Cook, 2013-01-10 18:38:28]

Sky, here's another revision.  It seems to touch fewer Ash <-> Chrome
interfaces.  PTAL.

https://codereview.chromium.org/11830008/diff/10001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace.cc (right):

https://codereview.chromium.org/11830008/diff/10001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace.cc:94: bool Workspace::IsImmersiveMode() const {
Does this seem OK here?  It could also live in WorkspaceController or even
RootWindowController.

[sky, 2013-01-10 19:02:05]

https://codereview.chromium.org/11830008/diff/10001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace.cc (right):

https://codereview.chromium.org/11830008/diff/10001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace.cc:94: bool Workspace::IsImmersiveMode() const {
On 2013/01/10 18:38:28, James Cook (Chromium) wrote:
> Does this seem OK here?  It could also live in WorkspaceController or even
> RootWindowController.

It doesn't look like the workspace code cares about this state at all, so I'm
inclined to move it else where. Maybe put it in RootWindowController?

https://codereview.chromium.org/11830008/diff/10001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace.cc:97: if (child->GetProperty(kImmersiveModeKey))
Shouldn't you check if the child is visible?

https://codereview.chromium.org/11830008/diff/10001/ash/wm/workspace/workspac...
File ash/wm/workspace/workspace_manager.h (right):

https://codereview.chromium.org/11830008/diff/10001/ash/wm/workspace/workspac...
ash/wm/workspace/workspace_manager.h:74: Workspace* active_workspace() { return
active_workspace_; }
I've tried to not expose Workspace at all, why do you need to expose this?

https://codereview.chromium.org/11830008/diff/10001/ash/wm/workspace_controll...
File ash/wm/workspace_controller.h (right):

https://codereview.chromium.org/11830008/diff/10001/ash/wm/workspace_controll...
ash/wm/workspace_controller.h:44: // Returns the currently active workspace.
Same comment about not exposing workspace. I'm ok exposing the parent window,
but not Worskpace.

[James Cook, 2013-01-10 19:42:18]

Sky, PTAL.

(As an aside, it seems like this patch has taken a lot more revision than I
expected for a relatively simple feature... Should I have talked to you in more
detail before starting?  Am I missing some core concept in how we glue
chrome/browser to ash/ ?)

[sky, 2013-01-10 21:15:03]

LGTM

[commit-bot: I haz the power, 2013-01-10 21:43:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/11830008/2007

[commit-bot: I haz the power, 2013-01-10 23:37:17]

Change committed as 176206