Enable shelf gestures in immersive mode

This CL modifies the check that prevents shelf gestures from working in
fullscreen mode so that it allows gestures during immersive mode.
    
This also modifies the drag completion behaviour to keep the shelf in the
correct state while immersive.
    
BUG=chromium:224301
BUG=chromium:230039
TEST=Tested that swipe in/out works in immersive mode.
     Tested that transitioning between immersive and non-immersive doesn't break
     the shelf.
     Tested that swiping doesn't work in traditional fullscreen mode.

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

[rharrison, 2013-05-02 15:17:59]

This is another change that I am not sure if this is the correct approach, PTAL
and comment.

[James Cook, 2013-05-02 18:49:42]

On 2013/05/02 15:17:59, rharrison wrote:
> This is another change that I am not sure if this is the correct approach,
PTAL
> and comment.

Hey, when you're asking 4 people to review a change, can you pick one person to
look at it first?

I'm happy to OWNERS approve this change, but I want someone more familiar with
shelf gestures to look first.

Sadrul?

[rharrison, 2013-05-02 20:30:55]

On 2013/05/02 18:49:42, James Cook (Chromium) wrote:
> On 2013/05/02 15:17:59, rharrison wrote:
> > This is another change that I am not sure if this is the correct approach,
> PTAL
> > and comment.
> 
> Hey, when you're asking 4 people to review a change, can you pick one person
to
> look at it first?
> 
> I'm happy to OWNERS approve this change, but I want someone more familiar with
> shelf gestures to look first.
> 
> Sadrul?

Sorry about that James. I will remove you as a reviewer until I am ready for an
OWNERS review.

[Harry McCleave, 2013-05-02 20:36:32]

other than that lgtm with the obvious I am new to reviewing and not an owner
disclaimers.

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:450: aura::Window* active =
wm::GetActiveWindow();
Would this not be better served as an if/else pairing (the current flow of
SetAuto...(ALWAYS); SetAutoHide...(NEVER) in the
active/fullscreen/MinimalChromeKey case seems cumbersome.

[sadrul, 2013-05-02 20:43:58]

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:453:
active->GetProperty(kFullscreenUsesMinimalChromeKey))
I am not familiar with this magical key. But it would be good to
SetAutoHideBehavior just once, instead of setting it to ALWAYS (449), and
setting it back to NEVER (454).

[pkotwicz, 2013-05-06 20:50:37]

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:454:
SetAutoHideBehavior(SHELF_AUTO_HIDE_BEHAVIOR_NEVER);
Drive by:
- You should probably use RootWindowController::GetFullscreenWindow() For
instance, in immersive fullscreen, the bookmark bubble could be active, but we
are theoretically still fullscreen.

[rharrison, 2013-05-16 17:31:04]

PTAL, I think I have addressed all of the concerns raised.

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:450: aura::Window* active =
wm::GetActiveWindow();
On 2013/05/02 20:36:32, Harry McCleave wrote:
> Would this not be better served as an if/else pairing (the current flow of
> SetAuto...(ALWAYS); SetAutoHide...(NEVER) in the
> active/fullscreen/MinimalChromeKey case seems cumbersome.

Done.

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:453:
active->GetProperty(kFullscreenUsesMinimalChromeKey))
On 2013/05/02 20:43:58, sadrul wrote:
> I am not familiar with this magical key. But it would be good to
> SetAutoHideBehavior just once, instead of setting it to ALWAYS (449), and
> setting it back to NEVER (454).

Done.

https://codereview.chromium.org/14740012/diff/1/ash/shelf/shelf_layout_manage...
ash/shelf/shelf_layout_manager.cc:454:
SetAutoHideBehavior(SHELF_AUTO_HIDE_BEHAVIOR_NEVER);
On 2013/05/06 20:50:37, pkotwicz wrote:
> Drive by:
> - You should probably use RootWindowController::GetFullscreenWindow() For
> instance, in immersive fullscreen, the bookmark bubble could be active, but we
> are theoretically still fullscreen.

Done.

[pkotwicz, 2013-05-16 19:11:49]

LGTM with comments

https://codereview.chromium.org/14740012/diff/16001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/14740012/diff/16001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:444: 
Nit: remove blank line

https://codereview.chromium.org/14740012/diff/16001/ash/wm/gestures/shelf_ges...
File ash/wm/gestures/shelf_gesture_handler.cc (right):

https://codereview.chromium.org/14740012/diff/16001/ash/wm/gestures/shelf_ges...
ash/wm/gestures/shelf_gesture_handler.cc:45:
!active->GetProperty(kFullscreenUsesMinimalChromeKey))
For the sake of consistency, shouldn't you use GetFullscreenWindow() here too? I
don't expect that you will get into many cases where the active window is not
the fullscreen one. One unlikely example is the user using the HTML 5 color
picker:
http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_input_type_color

Also, please update the comment.

[rharrison, 2013-05-16 20:28:30]

https://codereview.chromium.org/14740012/diff/16001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/14740012/diff/16001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:444: 
On 2013/05/16 19:11:49, pkotwicz wrote:
> Nit: remove blank line

Done.

https://codereview.chromium.org/14740012/diff/16001/ash/wm/gestures/shelf_ges...
File ash/wm/gestures/shelf_gesture_handler.cc (right):

https://codereview.chromium.org/14740012/diff/16001/ash/wm/gestures/shelf_ges...
ash/wm/gestures/shelf_gesture_handler.cc:45:
!active->GetProperty(kFullscreenUsesMinimalChromeKey))
On 2013/05/16 19:11:49, pkotwicz wrote:
> For the sake of consistency, shouldn't you use GetFullscreenWindow() here too?
I
> don't expect that you will get into many cases where the active window is not
> the fullscreen one. One unlikely example is the user using the HTML 5 color
> picker:
> http://www.w3schools.com/html/tryit.asp?filename=tryhtml5_input_type_color
> 
> Also, please update the comment.

Done.

[rharrison, 2013-05-21 17:52:33]

Talked to sadrul@, he has no further comments.

Added jamescook@ for OWNERS

[James Cook, 2013-05-22 12:04:22]

https://codereview.chromium.org/14740012/diff/41001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/14740012/diff/41001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:439:
shelf_->status_area_widget()->Deactivate();
Won't this line crash is shelf_ is NULL?  Either you need to put this into the
condition above, or the if() above isn't needed.

https://codereview.chromium.org/14740012/diff/41001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/14740012/diff/41001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:284: bool IsInImmersiveMode() const;
ash/ has no knowledge of immersive mode. That's a browser-level construct. This
should be called something like FullscreenWithMinimalChrome, similar to the
window property key.

https://codereview.chromium.org/14740012/diff/41001/ash/wm/gestures/shelf_ges...
File ash/wm/gestures/shelf_gesture_handler.cc (right):

https://codereview.chromium.org/14740012/diff/41001/ash/wm/gestures/shelf_ges...
ash/wm/gestures/shelf_gesture_handler.cc:47: if (fullscreen &&
!fullscreen->GetProperty(kFullscreenUsesMinimalChromeKey))
Can this share code with IsInImmersiveMode / FullscreenWithMinimalChrome in the
other file?

[rharrison, 2013-05-22 20:20:02]

https://codereview.chromium.org/14740012/diff/41001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/14740012/diff/41001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.cc:439:
shelf_->status_area_widget()->Deactivate();
On 2013/05/22 12:04:22, James Cook (Chromium) wrote:
> Won't this line crash is shelf_ is NULL?  Either you need to put this into the
> condition above, or the if() above isn't needed.

Done.

https://codereview.chromium.org/14740012/diff/41001/ash/shelf/shelf_layout_ma...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/14740012/diff/41001/ash/shelf/shelf_layout_ma...
ash/shelf/shelf_layout_manager.h:284: bool IsInImmersiveMode() const;
On 2013/05/22 12:04:22, James Cook (Chromium) wrote:
> ash/ has no knowledge of immersive mode. That's a browser-level construct.
This
> should be called something like FullscreenWithMinimalChrome, similar to the
> window property key.

Done.

https://codereview.chromium.org/14740012/diff/41001/ash/wm/gestures/shelf_ges...
File ash/wm/gestures/shelf_gesture_handler.cc (right):

https://codereview.chromium.org/14740012/diff/41001/ash/wm/gestures/shelf_ges...
ash/wm/gestures/shelf_gesture_handler.cc:47: if (fullscreen &&
!fullscreen->GetProperty(kFullscreenUsesMinimalChromeKey))
On 2013/05/22 12:04:22, James Cook (Chromium) wrote:
> Can this share code with IsInImmersiveMode / FullscreenWithMinimalChrome in
the
> other file?

Done.

[James Cook, 2013-05-22 20:23:50]

lgtm

[commit-bot: I haz the power, 2013-05-22 20:25:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rharrison@chromium.org/14740012/50001

[commit-bot: I haz the power, 2013-05-23 10:33:55]

Change committed as 201733