Prevent maximize/minimize/fullscreen-ize window in trusted pinned mode.

Prevent maximize/minimize/fullscreen-ize window in trusted pinned mode.

If the window is in the trusted pinned mode,
we do not expect that can enter into another mode, e.g.
maximized/minimized/fulscreen mode, without explicit
unpin action by client.

BUG=665943
TEST=Ran trybots. Tested in ARC kiosk mode, and the apps can continue
     to run on a pinnded window.

Review-Url: https://codereview.chromium.org/2590123002
Cr-Commit-Position: refs/heads/master@{#441872}
Committed: https://chromium.googlesource.com/chromium/src/+/c0ff6975cc421ada075d167ec16fdde63514264f

[hidehiko, 2016-12-20 08:31:28]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 08:31:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 09:35:59]

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

[commit-bot: I haz the power, 2016-12-20 09:36:00]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-12-20 13:15:19]

hidehiko@chromium.org changed reviewers:
+ oshima@chromium.org

[hidehiko, 2016-12-20 13:15:20]

This is the fix for M56 release blocker issue, so a target to be cherry-picked.
The original CL was https://crrev.com/2554443002/, but reflecting review comment
by reveman@, I made another completely different CL.

oshima@, could you take a look?
If this direction is good, I'll close the original CL.

BTW, where is a better place to add unittests?
default_state and maximize_mode_window_state do not seem to have their own
unittest. Any recommendation?

[oshima, 2016-12-21 01:38:58]

https://codereview.chromium.org/2590123002/diff/1/ash/common/wm/default_state.cc
File ash/common/wm/default_state.cc (right):

https://codereview.chromium.org/2590123002/diff/1/ash/common/wm/default_state...
ash/common/wm/default_state.cc:169: 
It's probably safer to reject all request in the following condition?

if (current == PINNED && event != UNPIN) {
  return;
}

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

https://codereview.chromium.org/2590123002/diff/1/ash/common/wm/maximize_mode...
ash/common/wm/maximize_mode/maximize_mode_window_state.cc:137: return;
FYI: ARC windows are no longer using this class. We may want to remove pin code,
and add assert to prevent using pinned, unless we want to use PINn'ed state for
cros windows.

[hidehiko, 2016-12-21 06:55:30]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 06:55:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hidehiko, 2016-12-21 06:57:30]

Thank you for review. PTAL.

https://codereview.chromium.org/2590123002/diff/1/ash/common/wm/default_state.cc
File ash/common/wm/default_state.cc (right):

https://codereview.chromium.org/2590123002/diff/1/ash/common/wm/default_state...
ash/common/wm/default_state.cc:169: 
On 2016/12/21 01:38:58, oshima (OOO til Jan 5th) wrote:
> It's probably safer to reject all request in the following condition?
> 
> if (current == PINNED && event != UNPIN) {
>   return;
> }

Done.

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

https://codereview.chromium.org/2590123002/diff/1/ash/common/wm/maximize_mode...
ash/common/wm/maximize_mode/maximize_mode_window_state.cc:137: return;
On 2016/12/21 01:38:58, oshima (OOO til Jan 5th) wrote:
> FYI: ARC windows are no longer using this class. We may want to remove pin
code,
> and add assert to prevent using pinned, unless we want to use PINn'ed state
for
> cros windows.

Thanks, so reverted this file.
Let's address removing pinned code from this file separately.

[commit-bot: I haz the power, 2016-12-21 07:24:30]

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

[commit-bot: I haz the power, 2016-12-21 07:24:32]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-12-21 21:18:23]

https://codereview.chromium.org/2590123002/diff/20001/ash/common/wm/default_s...
File ash/common/wm/default_state.cc (right):

https://codereview.chromium.org/2590123002/diff/20001/ash/common/wm/default_s...
ash/common/wm/default_state.cc:169: return;
move this after workspace events because you still need to handle them in pinned
state and please add test to cover workspace/display size change.

[hidehiko, 2016-12-22 16:26:32]

The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 16:26:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-22 16:56:14]

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

[commit-bot: I haz the power, 2016-12-22 16:56:15]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-12-22 16:57:57]

Thank you for review. PTAL.

https://codereview.chromium.org/2590123002/diff/20001/ash/common/wm/default_s...
File ash/common/wm/default_state.cc (right):

https://codereview.chromium.org/2590123002/diff/20001/ash/common/wm/default_s...
ash/common/wm/default_state.cc:169: return;
On 2016/12/21 21:18:23, oshima (OOO til Jan 5th) wrote:
> move this after workspace events because you still need to handle them in
pinned
> state and please add test to cover workspace/display size change.

Done. Added test in ash/wm/window_state_unittest.cc. Is it right place for this
case?

[oshima, 2017-01-05 20:51:44]

lgtm

[hidehiko, 2017-01-06 03:54:12]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2017-01-06 03:54:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 04:24:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483674852477490,
"parent_rev": "7254e3507c46f8b090662342ec71ccd36da468c5", "commit_rev":
"c0ff6975cc421ada075d167ec16fdde63514264f"}

[commit-bot: I haz the power, 2017-01-06 04:25:09]

Description was changed from

==========
Prevent maximize/minimize/fullscreen-ize window in trusted pinned mode.

If the window is in the trusted pinned mode,
we do not expect that can enter into another mode, e.g.
maximized/minimized/fulscreen mode, without explicit
unpin action by client.

BUG=665943
TEST=Ran trybots. Tested in ARC kiosk mode, and the apps can continue
     to run on a pinnded window.
==========

to

==========
Prevent maximize/minimize/fullscreen-ize window in trusted pinned mode.

If the window is in the trusted pinned mode,
we do not expect that can enter into another mode, e.g.
maximized/minimized/fulscreen mode, without explicit
unpin action by client.

BUG=665943
TEST=Ran trybots. Tested in ARC kiosk mode, and the apps can continue
     to run on a pinnded window.

Review-Url: https://codereview.chromium.org/2590123002
Cr-Commit-Position: refs/heads/master@{#441872}
Committed:
https://chromium.googlesource.com/chromium/src/+/c0ff6975cc421ada075d167ec16f...
==========

[commit-bot: I haz the power, 2017-01-06 04:25:10]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c0ff6975cc421ada075d167ec16f...