Enable immersive fullscreen on Windows Ash.

Enable immersive fullscreen on Windows Ash.

On ChromeOS a reveal occurs on a swipe down from the top 
edge, and hides on a swipe up from the top edge.

On Windows we can only detect one piece of information: A 
swipe up occurred from the bottom edge, or a swipe down 
occured from the top edge.  These two events are 
indistinguishable on Windows, and they do not come with any 
location information.

As a result, the CrOS behavior of these two distinct 
operations are merged into one operation here.  On Windows 
an "edge swipe" displays the shelf AND does the immersive 
reveal, whereas on CrOS these are separate operations 
triggered by a swipe on the respective edges.

Hiding should still behave the same as CrOS.

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

[zturner, 2014-03-13 01:41:34]

benwells: chrome/browser/ui/views/apps/*
sky:      chrome/browser/ui/views/frame/*
          ui/wm/core/*
          ash/shelf/*
pkotwicz: ash/wm/*

[pkotwicz, 2014-03-13 01:59:19]

I will take a look at the CL on Thursday morning

[sky, 2014-03-13 15:55:05]

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:2248: bool is_browser_fullscreen =
url.is_empty();
Shouldn't this depend upon the hostdesktype?

https://codereview.chromium.org/198413003/diff/1/ui/wm/core/compound_event_fi...
File ui/wm/core/compound_event_filter.cc (right):

https://codereview.chromium.org/198413003/diff/1/ui/wm/core/compound_event_fi...
ui/wm/core/compound_event_filter.cc:303:
static_cast<aura::Window*>(event->target()), event, false);
Who ends up enabling the mouse events?

[zturner, 2014-03-13 16:15:59]

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:2248: bool is_browser_fullscreen =
url.is_empty();
On 2014/03/13 15:55:09, sky wrote:
> Shouldn't this depend upon the hostdesktype?

Perhaps so, in any case I should probably test more thoroughly with fullscreen
in desktop mode and see how much of this code is called from desktop mode.

https://codereview.chromium.org/198413003/diff/1/ui/wm/core/compound_event_fi...
File ui/wm/core/compound_event_filter.cc (right):

https://codereview.chromium.org/198413003/diff/1/ui/wm/core/compound_event_fi...
ui/wm/core/compound_event_filter.cc:303:
static_cast<aura::Window*>(event->target()), event, false);
On 2014/03/13 15:55:09, sky wrote:
> Who ends up enabling the mouse events?

Receiving another mouse event re-enables them.  Scroll a bit higher in the file
and look for OnMouseEvent.  Also check the implementation of
SetMouseEventsEnableStateOnEvent (also in this same file) to see what disabling
mouse events actually does.

[pkotwicz, 2014-03-13 16:48:19]

Some initial comments

I also just published https://codereview.chromium.org/199283002/
Feel free to merge with the CL (or not) depending on which is easier

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc (left):

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:16: #if
defined(OS_CHROMEOS)
The new ifdef should be:
#if defined(USE_ASH)

Linux Aura does not use ash. It is possible that we will change the ifdef once
it does

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:21: }
Can you reenable immersive_mode_controller_ash_unittest.cc on Windows and make
sure that the test works properly?

You'll probably also need to update accelerator_commands_browsertest.cc

[zturner, 2014-03-13 16:54:06]

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc (left):

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:16: #if
defined(OS_CHROMEOS)
On 2014/03/13 16:48:19, pkotwicz wrote:
> The new ifdef should be:
> #if defined(USE_ASH)
> 
> Linux Aura does not use ash. It is possible that we will change the ifdef once
> it does
> 

I assume the same applies everywhere that there used to be an #if
defined(OS_CHROMEOS)?  There was one place talking about kiosk mode, but I think
that's still CrOS only, right?

So just to be clear, compiling on Linux with USE_ASH currently isn't supported
anyway, so right now USE_ASH is equivalent to OS_CHROMEOS || OS_WIN?

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:21: }
On 2014/03/13 16:48:19, pkotwicz wrote:
> Can you reenable immersive_mode_controller_ash_unittest.cc on Windows and make
> sure that the test works properly?
> 
> You'll probably also need to update accelerator_commands_browsertest.cc

Thanks, I missed these.

[zturner, 2014-03-13 17:56:39]

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc (left):

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:21: }
On 2014/03/13 16:48:19, pkotwicz wrote:
> Can you reenable immersive_mode_controller_ash_unittest.cc on Windows and make
> sure that the test works properly?
> 
> You'll probably also need to update accelerator_commands_browsertest.cc

All of these tests are broken because of this:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/b...

LMK if you think this needs to be fixed before submitting this, or if it can be
fixed as a followup.

[pkotwicz, 2014-03-13 18:08:23]

On 2014/03/13 17:56:39, zturner wrote:
>
https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
> File chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc
(left):
> 
>
https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
> chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:21: }
> On 2014/03/13 16:48:19, pkotwicz wrote:
> > Can you reenable immersive_mode_controller_ash_unittest.cc on Windows and
make
> > sure that the test works properly?
> > 
> > You'll probably also need to update accelerator_commands_browsertest.cc
> 
> All of these tests are broken because of this:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/b...
> 
> LMK if you think this needs to be fixed before submitting this, or if it can
be
> fixed as a followup.

Those tests can be fixed as a followup.

[pkotwicz, 2014-03-13 18:08:45]

Some more review comments. I will look at your new Patch Set :)

https://codereview.chromium.org/198413003/diff/1/ash/shelf/shelf_layout_manag...
File ash/shelf/shelf_layout_manager.cc (left):

https://codereview.chromium.org/198413003/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:399: WORKSPACE_WINDOW_STATE_FULL_SCREEN) {
Do we still want this behavior when
ash::wm::WindowState::hide_shelf_when_fullscreen() is true for the topmost
window?
This is the case if the topmost window is in HTML5 fullscreen. e.g
http://html5-demos.appspot.com/static/fullscreen.html

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

https://codereview.chromium.org/198413003/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:398: gesture_drag_status_ =
GESTURE_DRAG_COMPLETE_IN_PROGRESS;
Shouldn't we be calling SetAutoHideBehavior() or UpdateVisibilityState() as
appropriate similar to ShelfLayoutManager::CompleteGestureDrag()

Otherwise, we might revert back to SHELF_VISIBLE the next time that
UpdateVisibilityState() is called?

https://codereview.chromium.org/198413003/diff/1/ash/wm/immersive_fullscreen_...
File ash/wm/immersive_fullscreen_controller.cc (right):

https://codereview.chromium.org/198413003/diff/1/ash/wm/immersive_fullscreen_...
ash/wm/immersive_fullscreen_controller.cc:108: // the mouse can trigger
revealing the top-of-window views. The height must be
The part of this comment starting with: "The height must be greater than 1px"
should be moved to the #else block because it applies to CrOS only. We do not
support multiple displays for Win Ash?

https://codereview.chromium.org/198413003/diff/1/ash/wm/system_gesture_event_...
File ash/wm/system_gesture_event_filter.cc (right):

https://codereview.chromium.org/198413003/diff/1/ash/wm/system_gesture_event_...
ash/wm/system_gesture_event_filter.cc:68: // Do not stop propagation, since the
immersive fullscreen controller may
Nit: the comma is unnecessary

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:2243: #if defined(OS_CHROMEOS)
Kiosk mode is actually not CrOS only. switches::kKioskMode is used on
browser_window_controller_private.mm

(Thanks for your question, I now know slightly more about kiosk mode)

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc (left):

https://codereview.chromium.org/198413003/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:16: #if
defined(OS_CHROMEOS)
That is correct. You can remove the ifdef completely in files under ash/ though.

[pkotwicz, 2014-03-13 18:34:32]

https://codereview.chromium.org/198413003/diff/20001/ash/wm/immersive_fullscr...
File ash/wm/immersive_fullscreen_controller.cc (right):

https://codereview.chromium.org/198413003/diff/20001/ash/wm/immersive_fullscr...
ash/wm/immersive_fullscreen_controller.cc:805: 
Nuke this change

https://codereview.chromium.org/198413003/diff/20001/ash/wm/system_gesture_ev...
File ash/wm/system_gesture_event_filter.cc (right):

https://codereview.chromium.org/198413003/diff/20001/ash/wm/system_gesture_ev...
ash/wm/system_gesture_event_filter.cc:70: return;
Nits: - Return is unncessary given that this is the end of the function?
      - "the immersive fullscreen controller" -> ImmersiveFullscreenController

https://codereview.chromium.org/198413003/diff/20001/ash/wm/window_state.cc
File ash/wm/window_state.cc (left):

https://codereview.chromium.org/198413003/diff/20001/ash/wm/window_state.cc#o...
ash/wm/window_state.cc:99: #if defined(OS_CHROMEOS)
Can you merge with https://codereview.chromium.org/199283002/
That CL removes the code in the ifdef completely :)

https://codereview.chromium.org/198413003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:2249: #if defined(USE_ASH)
What was the result of your investigation into whether checking
Browser::host_desktop_type() is necessary?

https://codereview.chromium.org/198413003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc (right):

https://codereview.chromium.org/198413003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_factory.cc:5: #include
"chrome/browser/ui/views/frame/immersive_mode_controller.h"
Nit: Keep the blank line

Including immersive_mode_controller.h is unnecessary. (That's a bug I
introduced)

[zturner, 2014-03-13 19:28:08]

https://codereview.chromium.org/198413003/diff/1/ash/shelf/shelf_layout_manag...
File ash/shelf/shelf_layout_manager.cc (left):

https://codereview.chromium.org/198413003/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:399: WORKSPACE_WINDOW_STATE_FULL_SCREEN) {
On 2014/03/13 18:08:45, pkotwicz wrote:
> Do we still want this behavior when
> ash::wm::WindowState::hide_shelf_when_fullscreen() is true for the topmost
> window?
> This is the case if the topmost window is in HTML5 fullscreen. e.g
> http://html5-demos.appspot.com/static/fullscreen.html

Good point, I'll make this fix.

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

https://codereview.chromium.org/198413003/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:398: gesture_drag_status_ =
GESTURE_DRAG_COMPLETE_IN_PROGRESS;
On 2014/03/13 18:08:45, pkotwicz wrote:
> Shouldn't we be calling SetAutoHideBehavior() or UpdateVisibilityState() as
> appropriate similar to ShelfLayoutManager::CompleteGestureDrag()
> 
> Otherwise, we might revert back to SHELF_VISIBLE the next time that
> UpdateVisibilityState() is called?

Unfortunately this class and the logic is a little confusing.  We basically want
to mimic the behavior of doing a drag gesture, but a drag gesture comes in
multiple parts and a win8 edge swipe is just a single notification.  So the
logic of beginning a drag, updating a drag, and completing a drag is merged
here.

If all we do is call SetAutoHideBehavior() or UpdateVisibilityState(), the value
we would want to set it to is the same as the value it already is.  i.e. we
don't actually want to change the auto-hide behavior.  All we want to do is make
the currently auto-hidden shelf reveal itself temporarily.

https://codereview.chromium.org/198413003/diff/20001/ash/wm/immersive_fullscr...
File ash/wm/immersive_fullscreen_controller.cc (right):

https://codereview.chromium.org/198413003/diff/20001/ash/wm/immersive_fullscr...
ash/wm/immersive_fullscreen_controller.cc:805: 
On 2014/03/13 18:34:32, pkotwicz wrote:
> Nuke this change

Done.

https://codereview.chromium.org/198413003/diff/20001/ash/wm/system_gesture_ev...
File ash/wm/system_gesture_event_filter.cc (right):

https://codereview.chromium.org/198413003/diff/20001/ash/wm/system_gesture_ev...
ash/wm/system_gesture_event_filter.cc:70: return;
On 2014/03/13 18:34:32, pkotwicz wrote:
> Nits: - Return is unncessary given that this is the end of the function?
>       - "the immersive fullscreen controller" -> ImmersiveFullscreenController

I put return here so that it is clear this should be an early out, so that
anyone modifying the function later will simply need to add to the end and
reduce the risk of introducing a bug.

https://codereview.chromium.org/198413003/diff/20001/ash/wm/window_state.cc
File ash/wm/window_state.cc (left):

https://codereview.chromium.org/198413003/diff/20001/ash/wm/window_state.cc#o...
ash/wm/window_state.cc:99: #if defined(OS_CHROMEOS)
On 2014/03/13 18:34:32, pkotwicz wrote:
> Can you merge with https://codereview.chromium.org/199283002/
> That CL removes the code in the ifdef completely :)

Will wait for that patch to land and then rebase to ToT and re-upload.

https://codereview.chromium.org/198413003/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:2249: #if defined(USE_ASH)
On 2014/03/13 18:34:32, pkotwicz wrote:
> What was the result of your investigation into whether checking
> Browser::host_desktop_type() is necessary?

Didn't get to it yet, but doing so after I upload the next patchset.

[pkotwicz, 2014-03-13 19:52:48]

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

https://codereview.chromium.org/198413003/diff/1/ash/shelf/shelf_layout_manag...
ash/shelf/shelf_layout_manager.cc:398: gesture_drag_status_ =
GESTURE_DRAG_COMPLETE_IN_PROGRESS;
Is it possible for a user to do an edge swipe while the shelf has
ShelfVisibilityState SHELF_VISIBLE

[zturner, 2014-03-13 20:46:30]

I think this should take care of everything so far.  Still waiting on
https://codereview.chromium.org/199283002/ to be submitted before I rebase.

[sky, 2014-03-13 22:08:25]

LGTM

https://codereview.chromium.org/198413003/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.cc:2256: else
nit: style says no else after a return.

https://codereview.chromium.org/198413003/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/frame/browser_view.h (right):

https://codereview.chromium.org/198413003/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/browser_view.h:483: NORMAL_FULLSCREEN,
nit: follow naming pattern we used for content
(http://dev.chromium.org/developers/content-module/content-api )
FULLSCREEN_MODE_NORMAL FULLSCREEN_MODE_METRO_SNAP.

[benwells, 2014-03-14 02:22:12]

lgtm

[pkotwicz, 2014-03-14 05:08:26]

Ping me for another review once you have rebased to ToT :)

https://codereview.chromium.org/198413003/diff/1/ash/wm/immersive_fullscreen_...
File ash/wm/immersive_fullscreen_controller.cc (right):

https://codereview.chromium.org/198413003/diff/1/ash/wm/immersive_fullscreen_...
ash/wm/immersive_fullscreen_controller.cc:108: // the mouse can trigger
revealing the top-of-window views. The height must be
You still need to address this comment?

[zturner, 2014-03-17 21:15:58]

Rebased to ToT and addressed last remaining issue.

https://codereview.chromium.org/198413003/diff/1/ash/wm/immersive_fullscreen_...
File ash/wm/immersive_fullscreen_controller.cc (right):

https://codereview.chromium.org/198413003/diff/1/ash/wm/immersive_fullscreen_...
ash/wm/immersive_fullscreen_controller.cc:108: // the mouse can trigger
revealing the top-of-window views. The height must be
On 2014/03/14 05:08:27, pkotwicz wrote:
> You still need to address this comment?

Done.  Also, you are correct.  Windows Ash (and metro apps in general) cannot
occupy more than one display.

[pkotwicz, 2014-03-18 14:56:38]

Looks really good

The state of Win8 Ash browser tests seems to be dismal, however can you add
TODOs to enable browser tests on Windows for:
BrowserNonClientFrameViewAshTest.ImmersiveFullscreen
ZoomBubbleBrowserTest.ImmersiveFullscreen
AcceleratorCommandsBrowserTest.ToggleFullscreen
AcceleratorCommandsPlatformAppFullscreenBrowserTest.ToggleFullscreen
DetachToBrowserInSeparateDisplayTabDragControllerTest.DragTabToImmersiveBrowserOnSeparateDisplay

And you said that you would fix ImmersiveModeControllerAshTest.* as part of a
separate CL I believe

[pkotwicz, 2014-03-18 14:56:46]

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

https://codereview.chromium.org/198413003/diff/100001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:395: void
ShelfLayoutManager::OnGestureEdgeSwipe(const ui::GestureEvent& gesture) {
Would you be able to replace this entire function with this:

if (visibility_state() == SHELF_AUTO_HIDE) {
  gesture_drag_status_ = GESTURE_DRAG_COMPLETE_IN_PROGRESS;
  gesture_drag_auto_hide_state_ = SHELF_AUTO_HIDE_SHOWN;
  UpdateVisibilityState();
  gesture_drag_status_ = GESTURE_DRAG_DONE;
}

https://codereview.chromium.org/198413003/diff/100001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:404: return;
With the code as you have it, won't you early return if the topmost window is in
immersive fullscreen (thus the shelf is auto hidden) but the shelf was visible
before entering immersive fullscreen?

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2229: #if defined(OS_CHROMEOS)
Kiosk mode is not CrOS only (e.g. it is used in
browser_window_controller_private.mm)
I believe that it was behind an "#if defined" because immersive fullscreen used
to be CrOS only.

Some people on the internet seem to have used kiosk mode on Windows too.
http://think2loud.com/868-google-chrome-full-screen-kiosk-mode/

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2235: #if defined(USE_ASH)
Is the #if defined() necessary here?

[zturner, 2014-03-18 18:12:59]

On 2014/03/18 14:56:38, pkotwicz wrote:
> Looks really good
> 
> The state of Win8 Ash browser tests seems to be dismal, however can you add
> TODOs to enable browser tests on Windows for:
> BrowserNonClientFrameViewAshTest.ImmersiveFullscreen
Actually BrowserNonClientFrameViewAshTest.* is disabled at the gyp level, and
ImmersiveFullscreen is disabled additionally at the source level, but I'll add
this anyway so that when the file is re-enabled, this will get fixed.

> ZoomBubbleBrowserTest.ImmersiveFullscreen
Done.

> AcceleratorCommandsBrowserTest.ToggleFullscreen
> AcceleratorCommandsPlatformAppFullscreenBrowserTest.ToggleFullscreen
These two are already enabled on Windows, but like the rest of browser_tests,
they
won't run in Ash without --ash-browsertests.  However, it seems that entire flag
is 
broken, so that will need some separate investigation.

>
DetachToBrowserInSeparateDisplayTabDragControllerTest.DragTabToImmersiveBrowserOnSeparateDisplay
Ash doesn't support multiple displays on Windows, so I think this test doesn't
apply?

> 
> And you said that you would fix ImmersiveModeControllerAshTest.* as part of a
> separate CL I believe
Yes, and I went ahead and added a comment mentioning that this needs to be
enabled, and how
to enable it.

[pkotwicz, 2014-03-18 18:17:10]

Did you forget to upload an updated CL?

[zturner, 2014-03-18 18:26:36]

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

https://codereview.chromium.org/198413003/diff/100001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:404: return;
On 2014/03/18 14:56:47, pkotwicz wrote:
> With the code as you have it, won't you early return if the topmost window is
in
> immersive fullscreen (thus the shelf is auto hidden) but the shelf was visible
> before entering immersive fullscreen?

Hmm, the code works in practice.  The relationship between visibility states and
auto hide states is still very confusing to me and the flow isn't very easy to
follow, but the test I've run is:

1) Uncheck the Autohide Shelf checkbox so that we have SHELF_VISIBLE.
2) Press F11 to enter immersive fullscreen.
3) Edge swipe.

The shelf is displayed and the immersive reveal happens, and moving the mouse
hides both.

[zturner, 2014-03-18 18:28:19]

On 2014/03/18 18:17:10, pkotwicz wrote:
> Did you forget to upload an updated CL?

Sorry, was responding to both of your messages and then uploading CL.  CL should
be up now.

[pkotwicz, 2014-03-18 19:35:26]

Still looking good

The AcceleratorCommandsBrowserTest.* and
AcceleratorCommandsPlatformAppFullscreenBrowserTest.* tests  used to pass
locally on Windows Ash. Your CL should make them fail. Can you disable/fix them
on Windows Ash?

Can you update the comment as to why
DetachToBrowserInSeparateDisplayTabDragControllerTest.DragTabToImmersiveBrowserOnSeparateDisplay
is disabled?

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

https://codereview.chromium.org/198413003/diff/100001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:404: return;
Ahh, I understand. This works because you are checking SHELF_VISIBLE when not
fullscreen.

I believe that the code that I suggested is equivalent and simpler

harrym@ is an expert wrt to the shelf and is conveniently in MTV. He should be
able to explain everyting :)

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2229: #if defined(OS_CHROMEOS)
You still need to address this comment?

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2235: #if defined(USE_ASH)
You still need to address this comment too?

https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
File
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc
(right):

https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:98:
// Immersive fullscreen is CrOS only for now.
Nit: might as well edit the comment here too

https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc:65: //
Immersive fullscreen is CrOS only for now.
Nit: Might as well correct the above comment as well

[zturner, 2014-03-18 20:54:54]

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2229: #if defined(OS_CHROMEOS)
On 2014/03/18 19:35:28, pkotwicz wrote:
> You still need to address this comment?

Done.

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2235: #if defined(USE_ASH)
On 2014/03/18 14:56:47, pkotwicz wrote:
> Is the #if defined() necessary here?

Linux Aura still doesn't support Ash, so I believe this condition is still
needed because of that.

https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
File
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc
(right):

https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:98:
// Immersive fullscreen is CrOS only for now.
On 2014/03/18 19:35:28, pkotwicz wrote:
> Nit: might as well edit the comment here too

Done.

https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
File chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc
(right):

https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc:65: //
Immersive fullscreen is CrOS only for now.
On 2014/03/18 19:35:28, pkotwicz wrote:
> Nit: Might as well correct the above comment as well

Done.

[zturner, 2014-03-18 20:55:41]

On 2014/03/18 20:54:54, zturner wrote:
>
https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_view.cc (right):
> 
>
https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_view.cc:2229: #if defined(OS_CHROMEOS)
> On 2014/03/18 19:35:28, pkotwicz wrote:
> > You still need to address this comment?
> 
> Done.
> 
>
https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_view.cc:2235: #if defined(USE_ASH)
> On 2014/03/18 14:56:47, pkotwicz wrote:
> > Is the #if defined() necessary here?
> 
> Linux Aura still doesn't support Ash, so I believe this condition is still
> needed because of that.
> 
>
https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
> File
> chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
>
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc:98:
> // Immersive fullscreen is CrOS only for now.
> On 2014/03/18 19:35:28, pkotwicz wrote:
> > Nit: might as well edit the comment here too
> 
> Done.
> 
>
https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
> File chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/198413003/diff/110001/chrome/browser/ui/views...
> chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc:65: //
> Immersive fullscreen is CrOS only for now.
> On 2014/03/18 19:35:28, pkotwicz wrote:
> > Nit: Might as well correct the above comment as well
> 
> Done.

Forgot to mention.  I tested your suggested change to
ShelfLayoutManager::OnGestureEdgeSwipe(), and it works well.  Thanks for the
suggestion.

[pkotwicz, 2014-03-18 21:54:31]

LGTM with a few comments.

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2235: #if defined(USE_ASH)
Linux Aura should have HOST_DESKTOP_TYPE_ASH defined. The condition below should
never be true for Linux Aura though

https://codereview.chromium.org/198413003/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/198413003/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1787: //
CrOS.
How about: "Immersive fullscreen is Ash only. However, Windows Ash does not
support multiple displays."

https://codereview.chromium.org/198413003/diff/130001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/198413003/diff/130001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1885:
'browser/ui/ash/accelerator_commands_browsertest.cc',
Can you remove the windows specific code from
accelerator_commands_browsertest.cc and kill
|AcceleratorCommandsFullscreenBrowserTest::put_window_in_immersive_|

[zturner, 2014-03-18 21:58:11]

https://codereview.chromium.org/198413003/diff/130001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/198413003/diff/130001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:1885:
'browser/ui/ash/accelerator_commands_browsertest.cc',
On 2014/03/18 21:54:32, pkotwicz wrote:
> Can you remove the windows specific code from
> accelerator_commands_browsertest.cc and kill
> |AcceleratorCommandsFullscreenBrowserTest::put_window_in_immersive_|

The Windows specific code is still correct, should we really remove it?  The
same code will have to be added back when these tests are fixed.

[pkotwicz, 2014-03-18 22:33:43]

Shouldn't |AcceleratorCommandsFullscreenBrowserTest::put_window_in_immersive_|
be true on Windows Ash?
I am fine if you keep the checks for switches::kAshBrowserTests because those
are correct

[zturner, 2014-03-18 22:35:26]

On 2014/03/18 22:33:43, pkotwicz wrote:
> Shouldn't |AcceleratorCommandsFullscreenBrowserTest::put_window_in_immersive_|
> be true on Windows Ash?
> I am fine if you keep the checks for switches::kAshBrowserTests because those
> are correct

Yes, in fact I just uploaded a new CL that fixes that, as well as the other
issues.

[zturner, 2014-03-18 22:35:44]

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/198413003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:2235: #if defined(USE_ASH)
On 2014/03/18 21:54:32, pkotwicz wrote:
> Linux Aura should have HOST_DESKTOP_TYPE_ASH defined. The condition below
should
> never be true for Linux Aura though

Fair enough.  Done.

https://codereview.chromium.org/198413003/diff/130001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
(right):

https://codereview.chromium.org/198413003/diff/130001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:1787: //
CrOS.
On 2014/03/18 21:54:32, pkotwicz wrote:
> How about: "Immersive fullscreen is Ash only. However, Windows Ash does not
> support multiple displays."

Done.

[pkotwicz, 2014-03-18 22:52:09]

Still LGTM. Thank you very much for bearing with me

[zturner, 2014-03-18 22:57:51]

The CQ bit was checked by zturner@chromium.org

[commit-bot: I haz the power, 2014-03-18 23:00:00]

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

[commit-bot: I haz the power, 2014-03-18 23:08:30]

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

[commit-bot: I haz the power, 2014-03-18 23:08:32]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[zturner, 2014-03-19 00:54:03]

The CQ bit was checked by zturner@chromium.org

[commit-bot: I haz the power, 2014-03-19 00:54:38]

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

[commit-bot: I haz the power, 2014-03-19 10:53:29]

Change committed as 257909