Fixing problem with edge swipe exiting immersive mode

Fixing problem with edge swipe exiting immersive mode

BUG=378414
TEST=several unittests

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

[Mr4D (OOO till 08-26), 2014-06-02 19:00:14]

As discussed. Please have a look!

[pkotwicz, 2014-06-02 19:14:01]

I will take a deeper look in a bit. However, you probably want to modify
ash/wm/immersive_fullscreen_controller.* Otherwise, the key will not get set if
a packaged app gets put into immersive fullscreen or if the task manager gets
put into immersive fullscreen

[pkotwicz, 2014-06-02 19:50:06]

https://codereview.chromium.org/309793005/diff/1/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/309793005/diff/1/ash/wm/window_state.h#newcod...
ash/wm/window_state.h:272: bool immersive_mode() const {
This should be called in_immersive_fullscreen().

You should also include a warning that hide_shelf_when_fullscreen() is probably
what the person is looking for and a short description of what immersive
fullscreen is.
In particular:
- Immersive fullscreen does not imply that the shelf is auto hidden. The shelf
may be hidden completely and it may not be possible to reveal the shelf using
either the mouse or touch while in immersive fullscreen
- Any window can be put into immersive fullscreen (It is possible to put the
task manager into immersive fullscreen)
- Immersive fullscreen implies that the top non client frame views are
revealable. The top non client frame views may or may not steal a few pixels
from the top edge of the screen

[Mr4D (OOO till 08-26), 2014-06-02 20:36:19]

Addressed. Added one more unit test (for all window types) but left the one for
browser windows in place (to see that that is covered as well).

Please have another look!

https://codereview.chromium.org/309793005/diff/1/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/309793005/diff/1/ash/wm/window_state.h#newcod...
ash/wm/window_state.h:272: bool immersive_mode() const {
On 2014/06/02 19:50:06, pkotwicz wrote:
> This should be called in_immersive_fullscreen().
> 
> You should also include a warning that hide_shelf_when_fullscreen() is
probably
> what the person is looking for and a short description of what immersive
> fullscreen is.
> In particular:
> - Immersive fullscreen does not imply that the shelf is auto hidden. The shelf
> may be hidden completely and it may not be possible to reveal the shelf using
> either the mouse or touch while in immersive fullscreen
> - Any window can be put into immersive fullscreen (It is possible to put the
> task manager into immersive fullscreen)
> - Immersive fullscreen implies that the top non client frame views are
> revealable. The top non client frame views may or may not steal a few pixels
> from the top edge of the screen

Done.

[pkotwicz, 2014-06-02 22:27:58]

LGTM with comments

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

https://codereview.chromium.org/309793005/diff/20001/ash/wm/immersive_fullscr...
ash/wm/immersive_fullscreen_controller.cc:282: // Mark a browser window as an
immersive mode window so that the
Can you please update this comment?

Can this be called right before UpdateShelfVisibility()?

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

https://codereview.chromium.org/309793005/diff/20001/ash/wm/immersive_fullscr...
ash/wm/immersive_fullscreen_controller_unittest.cc:783: // Check that the window
state gets properly marked for immersive mode.
Nit: immersive mode -> immersive fullscreen

https://codereview.chromium.org/309793005/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (right):

https://codereview.chromium.org/309793005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:1171:
EXPECT_FALSE(window_state->in_immersive_fullscreen());
For the sake of the record (in case someone looks at old codereviews and wonders
whether this test makes a statement about immersive fullscreen behavior) I do
not particularly like the assertion on line 1171. For any CrOS window firing
wm::WM_EVENT_TOGGLE_FULLSCREEN should enter immersive fullscreen. This test
works because we use an aura::Window with no associated widget and no associated
ash::CustomFrameViewAsh. (I think the test is fine though)

Shouldn't this test call WindowState::set_in_immersive_fullscreen() somewhere? -
I checked and this test does fail.

https://codereview.chromium.org/309793005/diff/20001/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/309793005/diff/20001/ash/wm/window_state.h#ne...
ash/wm/window_state.h:273: // allow the shelf to be shown.
The part about "It will also allow the shelf to be shown" is not necessarily
true.

It is untrue if a window is in both immersive fullscreen and HTML 5 fullscreen.

https://codereview.chromium.org/309793005/diff/20001/ash/wm/window_state.h#ne...
ash/wm/window_state.h:357: bool hide_shelf_when_fullscreen_;
Nit: Might as well group the fullscreen related properties together.

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

https://codereview.chromium.org/309793005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:267: //
immersive mode.
I don't think this test is useful. ImmersiveModeController is a thin wrapper
around ash::ImmersiveFullscreenController

[Mr4D (OOO till 08-26), 2014-06-03 14:36:40]

Thanks for your quick review!

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

https://codereview.chromium.org/309793005/diff/20001/ash/wm/immersive_fullscr...
ash/wm/immersive_fullscreen_controller.cc:282: // Mark a browser window as an
immersive mode window so that the
On 2014/06/02 22:27:58, pkotwicz wrote:
> Can you please update this comment?
> 
> Can this be called right before UpdateShelfVisibility()?

Done.

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

https://codereview.chromium.org/309793005/diff/20001/ash/wm/immersive_fullscr...
ash/wm/immersive_fullscreen_controller_unittest.cc:783: // Check that the window
state gets properly marked for immersive mode.
Somehow that is odd since there are several files naming it immersive mode.
(maybe they should be changed as well?)

Done.

https://codereview.chromium.org/309793005/diff/20001/ash/wm/maximize_mode/max...
File ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc (right):

https://codereview.chromium.org/309793005/diff/20001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:1171:
EXPECT_FALSE(window_state->in_immersive_fullscreen());
Aaargh. I am not sure where the activation line went (it is in the other test
below line 1203). Done.

https://codereview.chromium.org/309793005/diff/20001/ash/wm/window_state.h
File ash/wm/window_state.h (right):

https://codereview.chromium.org/309793005/diff/20001/ash/wm/window_state.h#ne...
ash/wm/window_state.h:273: // allow the shelf to be shown.
I am not sure if the full immersive functionality should be explained here. As
such I tried to keep this as brief as possible from the beginning. If someone is
interested in the full and actual state of immersive fullscreen he/she should
look at the explanations there. This is especially true since functionality
changes and a duplication of comments is dangerous like a duplication of code.
(I doubt that this comment would be updated). Nevertheless - I changed the
comment slightly again keeping it vague.

https://codereview.chromium.org/309793005/diff/20001/ash/wm/window_state.h#ne...
ash/wm/window_state.h:357: bool hide_shelf_when_fullscreen_;
On 2014/06/02 22:27:58, pkotwicz wrote:
> Nit: Might as well group the fullscreen related properties together.

Done.

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

https://codereview.chromium.org/309793005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:267: //
immersive mode.
Yes, it is now - and if someone changes that it might not test what we want
anymore. This was the reason why I left it here on purpose. If you strongly feel
otherwise I'll remove it.

[pkotwicz, 2014-06-03 15:15:02]

Still LGTM

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

https://codereview.chromium.org/309793005/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/frame/immersive_mode_controller_ash_unittest.cc:267: //
immersive mode.
You could theoretically make that argument for duplicating all of the tests in
immersive_fullscreen_controller_unittest.cc in
immersive_mode_controller_ash_unittest.cc

If someone changes the relationship between ImmersiveFullscreenController and
ImmersiveModeController they will need to appropriately update the tests.

[Mr4D (OOO till 08-26), 2014-06-03 15:35:20]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-06-03 15:36:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skuhne@chromium.org/309793005/60001

[commit-bot: I haz the power, 2014-06-03 18:05:56]

Change committed as 274570