Fully support the autohide shelf option for touch UI on Windows.

Fully support the autohide shelf option for touch UI on Windows.

This change does the following:

1) Adds a new gesture referred to as an EdgeSwipe gesture.
This is Win8 / Metro specific, and refers to a swipe up from
the bottom edge, or swipe down from the top edge.  The
semantics of this gesture are slightly different than those
of a bezel gesture, which can occur on any edge, and for
which you can determine specifically which edge it was (not
possible with Win8 edge swipe).

2) edge swipe no longer enters / exits full screen mode.
Instead it makes the shelf visible (if it was hidden, and
auto-hide was checked), disables auto-hide, and exits full
screen.

BUG=340772
R=asvitkine@chromium.org, cpu@chromium.org, harrym@chromium.org, jschuh@chromium.org, sadrul@chromium.org, sky@chromium.org

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

[zturner, 2014-02-08 01:34:47]

asvitkine:
  tools\metrics\histograms\histograms.xml
harrym:
  ash\shelf\shelf_layout_manager.cc
  ash\shelf\shelf_layout_manager.h
ananta:
  win8\metro_driver\chrome_app_view_ash.cc
jschuh:
  ui\metro_viewer\metro_viewer_messages.h
sky:
  ash\shelf\shelf_layout_manager.cc
  ash\shelf\shelf_layout_manager.h
  ash\touch\touch_uma.cc
  ash\wm\gestures\shelf_gesture_handler.cc
  ash\wm\system_gesture_event_filter.cc
  ash\wm\system_gesture_event_filter.h
  chrome\browser\ui\ash\launcher\chrome_launcher_controller.cc
  ui\aura\remote_root_window_host_win.cc
  ui\aura\remote_root_window_host_win.h
  ui\events\event.cc
  ui\events\event.h
  ui\events\event_constants.h
sadrul:
  ash\wm\gestures\shelf_gesture_handler.cc

[jschuh, 2014-02-08 03:48:50]

ipc security lgtm. unsandboxed -> unsandboxed

[Harry McCleave, 2014-02-08 03:50:52]

On 2014/02/08 03:48:50, Justin Schuh wrote:
> ipc security lgtm. unsandboxed -> unsandboxed

lgtm for ash/shelf

[Alexei Svitkine (slow), 2014-02-08 04:03:54]

histograms lgtm

[sadrul, 2014-02-08 17:14:20]

https://codereview.chromium.org/143023003/diff/20001/ui/events/event_constants.h
File ui/events/event_constants.h (right):

https://codereview.chromium.org/143023003/diff/20001/ui/events/event_constant...
ui/events/event_constants.h:53: ET_GESTURE_EDGE_SWIPE,
Hm. All ET_GESTURE events are currently constructed by the gesture-recognizer
(ui/events/gestures/). Should we call this one ET_WIN8_GESTURE_EDGE_SWIPE (or
something similar) that makes it clearer that this event doesn't happen in other
platforms?

[sky, 2014-02-10 15:28:38]

https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:394: void
ShelfLayoutManager::OnGestureEdgeSwipe(const ui::GestureEvent& gesture) {
Don't we want to do the same thing as in StartGestureDrag and UpdateGestureDrag?

[zturner, 2014-02-10 18:09:16]

On 2014/02/10 15:28:38, sky wrote:
>
https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
> File ash/shelf/shelf_layout_manager.cc (right):
> 
>
https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
> ash/shelf/shelf_layout_manager.cc:394: void
> ShelfLayoutManager::OnGestureEdgeSwipe(const ui::GestureEvent& gesture) {
> Don't we want to do the same thing as in StartGestureDrag and
UpdateGestureDrag?

In theory, and we can get the same behavior by calling StartGestureDrag() 
followed immediately by CompleteGestureDrag().  But I felt like this wasn't 
really clear from a code standpoint (since those functions also have a lot of 
support built in for animating, cancelling, etc), and also that it probably 
violated some assumptions of the [Start/Update/Complete]GestureDrag() 
functions, since certain codepaths of those functions rely on the position 
members of the GestureEvent, which are always going to be 0 for this particular 
event.  That last point in particular, it seems likely that in the future 
someone modifying the code for Start / CompleteGestureDrag might make use of 
the position variables outside of the proper if block, and break the behavior
in Windows.

Having the two line implementation which simply sets it to visible and updates
the AutoHideBehavior seemed the clearest and most future-proof.

[zturner, 2014-02-10 19:01:13]

Updated enum constant name based on suggestion from sadrul@

[sky, 2014-02-10 21:31:08]

Also, how about some test coverage?

https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
ash/shelf/shelf_layout_manager.cc:400: // Process the window list and check if
there are any visible windows.
This comment isn't right. And it isn't clear why you need to toggle fullscren
here? If fullscreen windows needs to be toggled when changing auto-hide
behavior, shouldn't that be done else where?

[zturner, 2014-02-10 21:35:49]

On 2014/02/10 21:31:08, sky wrote:
> Also, how about some test coverage?
> 
>
https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
> File ash/shelf/shelf_layout_manager.cc (right):
> 
>
https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
> ash/shelf/shelf_layout_manager.cc:400: // Process the window list and check if
> there are any visible windows.
> This comment isn't right. And it isn't clear why you need to toggle fullscren
> here? If fullscreen windows needs to be toggled when changing auto-hide
> behavior, shouldn't that be done else where?

That was just the decision from PMs about how the UI should behave here.  When
doing an edge swipe while the user is in fullscreen, it should exit fullscreen
and display the tray.

I'll fix the comment and add some tests, let me know if you still have concerns
about the fullscreen behavior.

[sky, 2014-02-11 00:32:00]

Ok, makes sense.

On Mon, Feb 10, 2014 at 1:35 PM,  <zturner@chromium.org> wrote:
> On 2014/02/10 21:31:08, sky wrote:
>>
>> Also, how about some test coverage?
>
>
>
>
https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
>>
>> File ash/shelf/shelf_layout_manager.cc (right):
>
>
>
>
https://codereview.chromium.org/143023003/diff/20001/ash/shelf/shelf_layout_m...
>>
>> ash/shelf/shelf_layout_manager.cc:400: // Process the window list and
>> check if
>> there are any visible windows.
>> This comment isn't right. And it isn't clear why you need to toggle
>> fullscren
>> here? If fullscreen windows needs to be toggled when changing auto-hide
>> behavior, shouldn't that be done else where?
>
>
> That was just the decision from PMs about how the UI should behave here.
> When
> doing an edge swipe while the user is in fullscreen, it should exit
> fullscreen
> and display the tray.
>
> I'll fix the comment and add some tests, let me know if you still have
> concerns
> about the fullscreen behavior.
>
> https://codereview.chromium.org/143023003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2014-02-11 18:55:21]

https://codereview.chromium.org/143023003/diff/350001/ash/wm/gestures/shelf_g...
File ash/wm/gestures/shelf_gesture_handler.cc (right):

https://codereview.chromium.org/143023003/diff/350001/ash/wm/gestures/shelf_g...
ash/wm/gestures/shelf_gesture_handler.cc:46: #if defined(OS_WIN)
Is there a reason we need the ifdefs here and in the test? The events won't be
generated other than win8, but it should be fine to test, right?

[zturner, 2014-02-11 19:17:49]

On 2014/02/11 18:55:21, sky wrote:
>
https://codereview.chromium.org/143023003/diff/350001/ash/wm/gestures/shelf_g...
> File ash/wm/gestures/shelf_gesture_handler.cc (right):
> 
>
https://codereview.chromium.org/143023003/diff/350001/ash/wm/gestures/shelf_g...
> ash/wm/gestures/shelf_gesture_handler.cc:46: #if defined(OS_WIN)
> Is there a reason we need the ifdefs here and in the test? The events won't be
> generated other than win8, but it should be fine to test, right?

I was kind of on the fence about whether or not to include them.  The events
will
never be generated outside of Win8, so it's a little wasteful to compile them in
the first place.  That was the only thing.  I'll remove all the ifdefs though if
you prefer.

[sadrul, 2014-02-11 21:20:47]

https://codereview.chromium.org/143023003/diff/410001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/143023003/diff/410001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:410: }
Can you use accelerators::ToggleFullscreen() (or something similar) here instead
of iterating over the list of windows?

https://codereview.chromium.org/143023003/diff/410001/ash/wm/system_gesture_e...
File ash/wm/system_gesture_event_filter.cc (right):

https://codereview.chromium.org/143023003/diff/410001/ash/wm/system_gesture_e...
ash/wm/system_gesture_event_filter.cc:92:
shelf_gesture_handler_->ProcessGestureEvent(*event)) {
Do you really need an instance of the ShelfGestureHandler here? Can you just
call into Shelf::OnGestureEdgeSwipe() from here?

[zturner, 2014-02-11 21:30:56]

https://codereview.chromium.org/143023003/diff/410001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/143023003/diff/410001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:410: }
On 2014/02/11 21:20:48, sadrul wrote:
> Can you use accelerators::ToggleFullscreen() (or something similar) here
instead
> of iterating over the list of windows?

Yes, thank you.  This should make the code much cleaner.

https://codereview.chromium.org/143023003/diff/410001/ash/wm/system_gesture_e...
File ash/wm/system_gesture_event_filter.cc (right):

https://codereview.chromium.org/143023003/diff/410001/ash/wm/system_gesture_e...
ash/wm/system_gesture_event_filter.cc:92:
shelf_gesture_handler_->ProcessGestureEvent(*event)) {
On 2014/02/11 21:20:48, sadrul wrote:
> Do you really need an instance of the ShelfGestureHandler here? Can you just
> call into Shelf::OnGestureEdgeSwipe() from here?

SystemGestureEventFilter doesn't contain an instance of the shelf, you have to
go through the ShelfGestureHandler, and we can't make the method static either. 
Unless I'm missing another way?  I think logically it makes the most sense this
way, since it doesn't stray from the accepted paradigm of having the event
filter simply forward the gesture to the appropriate handler.

[sadrul, 2014-02-11 21:37:30]

LGTM (with the ToggleFullscreen() change from the previous review comment)

https://codereview.chromium.org/143023003/diff/410001/ash/wm/system_gesture_e...
File ash/wm/system_gesture_event_filter.cc (right):

https://codereview.chromium.org/143023003/diff/410001/ash/wm/system_gesture_e...
ash/wm/system_gesture_event_filter.cc:92:
shelf_gesture_handler_->ProcessGestureEvent(*event)) {
On 2014/02/11 21:30:57, zturner wrote:
> On 2014/02/11 21:20:48, sadrul wrote:
> > Do you really need an instance of the ShelfGestureHandler here? Can you just
> > call into Shelf::OnGestureEdgeSwipe() from here?
> 
> SystemGestureEventFilter doesn't contain an instance of the shelf, you have to
> go through the ShelfGestureHandler, and we can't make the method static
either. 

You could do: Shell::GetPrimaryRootWindowController()->GetShelfLayoutManager()?

> Unless I'm missing another way?  I think logically it makes the most sense
this
> way, since it doesn't stray from the accepted paradigm of having the event
> filter simply forward the gesture to the appropriate handler.

This is true.

I don't feel strongly either way.

[sky, 2014-02-11 22:19:53]

LGTM

[cpu_(ooo_6.6-7.5), 2014-02-12 00:48:39]

lgtm

[zturner, 2014-02-12 00:49:48]

The CQ bit was checked by zturner@chromium.org

[commit-bot: I haz the power, 2014-02-12 00:50:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zturner@chromium.org/143023003/490001

[commit-bot: I haz the power, 2014-02-12 05:35:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 05:35:26]

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

[zturner, 2014-02-12 17:29:59]

Committed patchset #7 manually as r250738 (presubmit successful).