Fullscreen/immersive mode is allowed in touch view mode, the shelf gets hidden and an edge swipe br…

Fullscreen/immersive mode is allowed in touch view mode, the shelf gets hidden and an edge swipe brings the shelf back.


Allowing immersive / full screen mode in maximize mode

BUG=359606
TEST=ash tests and visual

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

[Mr4D (OOO till 08-26), 2014-04-22 21:05:32]

Please have a look!

[oshima, 2014-04-23 15:57:38]

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_window_state.cc:126: true);
can you refactor the code in default_state and use the same logic?

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_window_state.cc:150: case
wm::WM_EVENT_SET_BOUNDS:
a window in fullscreen state or maximzied state cannot be resized, so you can
just ignore these cases.

In addition, these events (SET_BOUNS to DISPLAY_BOUNDS_CHANGED)
won't change the window state type. Can you refactor ShowState (or
MaximizeOrCenterWindow) into one that simply update the bounds based on the
current state and another that updates the state? Then I think you don't need
another ShowState enum.

[Mr4D (OOO till 08-26), 2014-04-23 19:20:31]

Please have another look!

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_window_state.cc:126: true);
Not quite sure what you want. Either way - that should be done in a different
patch?

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_window_state.cc:150: case
wm::WM_EVENT_SET_BOUNDS:
Not entirely correct. A window could have (changing) restrictions which can lead
to new dimensions.
Even more so I have seen that the calculator apparently creates a window first
which is maximizable and then another window gets put inside it which is not.
Then the window changes state and bounds to make it not resizable (and not
maximizable).

I moved the update state into a separate function as requested. I also removed
the enum (which I had in the past but I didn't like) which lead into the fact
that the state calculation has now be performed before entering the function
(which I tried to avoid).

[oshima, 2014-04-24 00:35:14]

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_window_state.cc:150: case
wm::WM_EVENT_SET_BOUNDS:
On 2014/04/23 19:20:31, Mr4D wrote:
> Not entirely correct. A window could have (changing) restrictions which can
lead
> to new dimensions.

I don't think that's supposed to be possible because CanMaximize is not
modifiable (that is, we don't listen to its change).
I'm guessing that you're seeing this because we set the property too late in

https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/na...

?

> Even more so I have seen that the calculator apparently creates a window first
> which is maximizable and then another window gets put inside it which is not.

What do you mean by "another window gets put inside"?

> Then the window changes state and bounds to make it not resizable (and not
> maximizable).
> 
> I moved the update state into a separate function as requested. I also removed
> the enum (which I had in the past but I didn't like) which lead into the fact
> that the state calculation has now be performed before entering the function
> (which I tried to avoid).

[Mr4D (OOO till 08-26), 2014-04-24 21:52:12]

Done. Please have another look!

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_window_state.cc:150: case
wm::WM_EVENT_SET_BOUNDS:
Checked with scheib@ and this should be correct for ChromeOS. It would be a
different story for Windows though.

Done.

[Mr4D (OOO till 08-26), 2014-04-25 14:40:05]

Merged with other landed changes.

Please have another look!

[Mr4D (OOO till 08-26), 2014-04-25 17:36:28]

Please have another look

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_window_state.cc:126: true);
Your comment was a bit confusing. It sounded like that you wanted me to change
something in default_state. Done.

[oshima, 2014-04-25 18:46:55]

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/247363005/diff/1/ash/wm/maximize_mode/maximiz...
ash/wm/maximize_mode/maximize_mode_window_state.cc:126: true);
On 2014/04/25 17:36:29, Mr4D wrote:
> Your comment was a bit confusing. It sounded like that you wanted me to change
> something in default_state. Done.

Yes, that's what I meant. The same code should work, so we should refactor and
use the same code instead of duplicating.

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

https://codereview.chromium.org/247363005/diff/80001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:701: // Full
screen mode should now be off and it should not come back while in
I didn't understand this comment. Fullscreen should stay on right?

https://codereview.chromium.org/247363005/diff/80001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:727: wm::WMEvent
event(wm::WM_EVENT_TOGGLE_FULLSCREEN);
move this to 745, just before OnWMEvent

[Mr4D (OOO till 08-26), 2014-04-25 19:55:19]

Have another look!

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

https://codereview.chromium.org/247363005/diff/80001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:701: // Full
screen mode should now be off and it should not come back while in
Right. It should stay on. Changed

https://codereview.chromium.org/247363005/diff/80001/ash/wm/maximize_mode/max...
ash/wm/maximize_mode/maximize_mode_window_manager_unittest.cc:727: wm::WMEvent
event(wm::WM_EVENT_TOGGLE_FULLSCREEN);
On 2014/04/25 18:46:55, oshima wrote:
> move this to 745, just before OnWMEvent

Done.

[oshima, 2014-04-25 20:14:58]

lgtm with nits

https://codereview.chromium.org/247363005/diff/100001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/247363005/diff/100001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:59: gfx::Rect
GetMaximizedAndCenteredBounds(wm::WindowState* state_object) {
nit: can you come up with better name as it now also return bounds for
fullscreen. (I'm fine with something like GetBoundsInMaximizedMode)

https://codereview.chromium.org/247363005/diff/100001/ash/wm/window_state_util.h
File ash/wm/window_state_util.h (right):

https://codereview.chromium.org/247363005/diff/100001/ash/wm/window_state_uti...
ash/wm/window_state_util.h:14: void ToggleFullScreen(WindowState* window_state,
WindowStateDelegate* delegate);
you don't need to pass delegate. Just get it from window_state.

[Mr4D (OOO till 08-26), 2014-04-25 21:00:48]

Thanks for your review!

https://codereview.chromium.org/247363005/diff/100001/ash/wm/maximize_mode/ma...
File ash/wm/maximize_mode/maximize_mode_window_state.cc (right):

https://codereview.chromium.org/247363005/diff/100001/ash/wm/maximize_mode/ma...
ash/wm/maximize_mode/maximize_mode_window_state.cc:59: gfx::Rect
GetMaximizedAndCenteredBounds(wm::WindowState* state_object) {
On 2014/04/25 20:14:59, oshima wrote:
> nit: can you come up with better name as it now also return bounds for
> fullscreen. (I'm fine with something like GetBoundsInMaximizedMode)

Done.

https://codereview.chromium.org/247363005/diff/100001/ash/wm/window_state_util.h
File ash/wm/window_state_util.h (right):

https://codereview.chromium.org/247363005/diff/100001/ash/wm/window_state_uti...
ash/wm/window_state_util.h:14: void ToggleFullScreen(WindowState* window_state,
WindowStateDelegate* delegate);
Since it is there private (and it is good that it is) that does not work. Left
as is (as discussed).

[Mr4D (OOO till 08-26), 2014-04-25 21:00:53]

The CQ bit was checked by skuhne@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:02:42]

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

[commit-bot: I haz the power, 2014-04-26 10:13:14]

Change committed as 266350