Add WM_EVENT_TRUSTED_PIN and WINDOW_STATE_TYPE_TRUSTED_PINNED to Ash.

Add WM_EVENT_TRUSTED_PIN and WINDOW_STATE_TYPE_TRUSTED_PINNED to Ash.

The event and the state is similar to WM_EVENT_PIN and WINDOW_STATE_TYPE_PINNED
respectively unless TRUSTED_PINNED cannot be exited by user operation.

BUG=b/29325086
TEST=Test with local Android build that sends setPinWithTrusted message, ash_unittests

Committed: https://crrev.com/94b6193abeb2e2057f17b32da9381767c6b3d9a8
Cr-Commit-Position: refs/heads/master@{#419685}

[hirono, 2016-08-26 07:34:32]

Description was changed from

==========
Add WM_EVENT_TRUSTED_PIN and WINDOW_STATE_TYPE_TRUSTED_PINNED to Ash.

The event and the state is similar to WM_EVENT_PIN and WINDOW_STATE_TYPE_PINNED
respectively unless TRUSTED_PINNED cannot be exited by user operation.

BUG=29325086
TEST=Test with local Android build that sends setPinWithTrusted message,
ash_unittests
==========

to

==========
Add WM_EVENT_TRUSTED_PIN and WINDOW_STATE_TYPE_TRUSTED_PINNED to Ash.

The event and the state is similar to WM_EVENT_PIN and WINDOW_STATE_TYPE_PINNED
respectively unless TRUSTED_PINNED cannot be exited by user operation.

BUG=b/29325086
TEST=Test with local Android build that sends setPinWithTrusted message,
ash_unittests
==========

[hirono, 2016-08-26 07:34:47]

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

[commit-bot: I haz the power, 2016-08-26 07:35:03]

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

[hirono, 2016-08-26 07:35:51]

hirono@chromium.org changed reviewers:
+ hidehiko@chromium.org, oshima@chromium.org, reveman@chromium.org

[hirono, 2016-08-26 07:35:52]

PTAL,

hidehiko@ Overall design
reveman@ components/exo/*
oshima@ ash/*

Thank you!

[commit-bot: I haz the power, 2016-08-26 09:05:52]

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

[commit-bot: I haz the power, 2016-08-26 09:05:53]

Dry run: This issue passed the CQ dry run.

[reveman, 2016-08-26 12:44:29]

components/exo lgtm

https://codereview.chromium.org/2285633002/diff/1/components/exo/shell_surfac...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2285633002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:408: TRACE_EVENT1("exo",
"ShellSurface::SetPinned", "pinned", pinned);
nit: add trusted flag to trace event

[hirono, 2016-08-29 00:24:51]

Thank you!

https://codereview.chromium.org/2285633002/diff/1/components/exo/shell_surfac...
File components/exo/shell_surface.cc (right):

https://codereview.chromium.org/2285633002/diff/1/components/exo/shell_surfac...
components/exo/shell_surface.cc:408: TRACE_EVENT1("exo",
"ShellSurface::SetPinned", "pinned", pinned);
On 2016/08/26 12:44:29, reveman wrote:
> nit: add trusted flag to trace event

Done.

[hidehiko, 2016-08-29 14:31:39]

Overall, LGTM with minor comments.
Defer to oshima@-san, specifically for ash implementation details.

Sorry for delayed review.

https://codereview.chromium.org/2285633002/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate_aura.cc (right):

https://codereview.chromium.org/2285633002/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:316: bool
CanHandleUnpin() {
Looks to work practically, but is it ok to leave UNPIN in the table?
https://cs.chromium.org/chromium/src/ash/common/accelerators/accelerator_tabl...

https://codereview.chromium.org/2285633002/diff/20001/ash/wm/screen_pinning_c...
File ash/wm/screen_pinning_controller_unittest.cc (right):

https://codereview.chromium.org/2285633002/diff/20001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:164:
EXPECT_TRUE(WmShell::Get()->IsPinned());
Could you explicitly comment that the "no-unpin" is expected behavior, here?

https://codereview.chromium.org/2285633002/diff/20001/components/exo/shell_su...
File components/exo/shell_surface.h (right):

https://codereview.chromium.org/2285633002/diff/20001/components/exo/shell_su...
components/exo/shell_surface.h:116: void SetPinned(bool pinned, bool trusted);
Could you comment that |trusted| is intentionally ignored, if pinned == false?

[hirono, 2016-08-30 07:16:08]

Thank you!

https://codereview.chromium.org/2285633002/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_delegate_aura.cc (right):

https://codereview.chromium.org/2285633002/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_delegate_aura.cc:316: bool
CanHandleUnpin() {
On 2016/08/29 14:31:38, hidehiko wrote:
> Looks to work practically, but is it ok to leave UNPIN in the table?
>
https://cs.chromium.org/chromium/src/ash/common/accelerators/accelerator_tabl...

I tried to create a new list in accelerator_table.cc for trusted pin state 
after getting the comment. But now I feel like keeping the implementation as it
it.

The accelerator action list is referred by accelerator controller. And
accelerator controller accesses wm::Shell to get current shell state. However 
wm::Shell does not know the difference between trusted pinned and normal pinned,
which I'd like to keep hiding internally.

WDYT?

https://codereview.chromium.org/2285633002/diff/20001/ash/wm/screen_pinning_c...
File ash/wm/screen_pinning_controller_unittest.cc (right):

https://codereview.chromium.org/2285633002/diff/20001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:164:
EXPECT_TRUE(WmShell::Get()->IsPinned());
On 2016/08/29 14:31:39, hidehiko wrote:
> Could you explicitly comment that the "no-unpin" is expected behavior, here?

Done.

https://codereview.chromium.org/2285633002/diff/20001/components/exo/shell_su...
File components/exo/shell_surface.h (right):

https://codereview.chromium.org/2285633002/diff/20001/components/exo/shell_su...
components/exo/shell_surface.h:116: void SetPinned(bool pinned, bool trusted);
On 2016/08/29 14:31:39, hidehiko wrote:
> Could you comment that |trusted| is intentionally ignored, if pinned == false?

Done.

[oshima, 2016-09-02 14:15:05]

do we need both? I had an impression that a user can't exit in pinned mode and
that's the key difference from immersive mode. Is there a doc that explain the
difference?

[hirono, 2016-09-05 23:27:30]

On 2016/09/02 14:15:05, oshima wrote:
> do we need both? I had an impression that a user can't exit in pinned mode and
> that's the key difference from immersive mode. Is there a doc that explain the
> difference?

I wrote the comparison for three modes (immersive mode, screen pinning, screen
locking).
https://docs.google.com/document/d/1y2fqqPFlQPgau-GMijI6xPZCTEu4ckRt41rwHWb_O...

Tl;dl

 * immersive mode: Users can exit the mode in few ways.
 * screen pinning: Users can exit the mode by shortcut keys. (CTRL+SHIFT+SEARCH)
 * screen locking: Users cannot exit the mode.

[hirono, 2016-09-05 23:27:38]

https://codereview.chromium.org/2285633002/diff/40001/components/exo/wayland/...
File components/exo/wayland/server.cc (right):

https://codereview.chromium.org/2285633002/diff/40001/components/exo/wayland/...
components/exo/wayland/server.cc:1845:
zwp_remote_surface_v1_send_state_type_changed(resource, state_type);
Only accelerator keys behave differently for pinning and locking. So we may be
able to use WINDOW_STATE_TYPE_PINNED for both pinning and locking with the
trusted flag.

The problem is that we report the Ash state to Android here. And we need to
distinguish the pinning and the locking in Android side. So if we use
WINDOW_STATE_TYPE_PINNED for both pinning and locking, we need to introduce a
new enum here which has different values for pinning and locking.

[oshima, 2016-09-09 07:18:02]

lgtm with a nit

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

https://codereview.chromium.org/2285633002/diff/40001/ash/common/wm/default_s...
ash/common/wm/default_state.cc:215: << "already in pinned mode.";
nit: can you update the log message to make it clear that the request was
failed?

[hirono, 2016-09-14 00:54:41]

Thank you!

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

https://codereview.chromium.org/2285633002/diff/40001/ash/common/wm/default_s...
ash/common/wm/default_state.cc:215: << "already in pinned mode.";
On 2016/09/09 07:18:02, oshima wrote:
> nit: can you update the log message to make it clear that the request was
> failed?

Done.

[hirono, 2016-09-14 01:03:54]

On 2016/09/14 00:54:41, hirono wrote:
> Thank you!
> 
>
https://codereview.chromium.org/2285633002/diff/40001/ash/common/wm/default_s...
> File ash/common/wm/default_state.cc (right):
> 
>
https://codereview.chromium.org/2285633002/diff/40001/ash/common/wm/default_s...
> ash/common/wm/default_state.cc:215: << "already in pinned mode.";
> On 2016/09/09 07:18:02, oshima wrote:
> > nit: can you update the log message to make it clear that the request was
> > failed?
> 
> Done.

Hi Hidehiko-san,

> On 2016/08/29 14:31:38, hidehiko wrote:
> > Looks to work practically, but is it ok to leave UNPIN in the table?
> >
>
https://cs.chromium.org/chromium/src/ash/common/accelerators/accelerator_tabl...
> 
> I tried to create a new list in accelerator_table.cc for trusted pin state 
> after getting the comment. But now I feel like keeping the implementation as
it
> it.

> The accelerator action list is referred by accelerator controller. And
> accelerator controller accesses wm::Shell to get current shell state. However 
> wm::Shell does not know the difference between trusted pinned and normal
pinned,
> which I'd like to keep hiding internally.
> 
> WDYT?

Could you take a look at this? I think it practically works and this is a design
matter.
If you are uncomfortable about leaving the UNPIN in the table, the second plan
is exposing trusted pin state to wm::Shell and letting wm::Shell refer the
different list that contains allowed action for trusted pin state.
Thank you!

[hirono, 2016-09-20 03:24:54]

The CQ bit was checked by hirono@chromium.org

[hirono, 2016-09-20 03:24:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, hidehiko@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2285633002/#ps60001
(title: "Update error log.")

[commit-bot: I haz the power, 2016-09-20 03:25:12]

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-09-20 03:27:39]

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

[commit-bot: I haz the power, 2016-09-20 03:27:40]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[hirono, 2016-09-20 05:02:09]

The CQ bit was checked by hirono@chromium.org

[hirono, 2016-09-20 05:02:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org, reveman@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2285633002/#ps80001
(title: "Rebased.")

[commit-bot: I haz the power, 2016-09-20 05:02:18]

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-09-20 05:28:23]

Description was changed from

==========
Add WM_EVENT_TRUSTED_PIN and WINDOW_STATE_TYPE_TRUSTED_PINNED to Ash.

The event and the state is similar to WM_EVENT_PIN and WINDOW_STATE_TYPE_PINNED
respectively unless TRUSTED_PINNED cannot be exited by user operation.

BUG=b/29325086
TEST=Test with local Android build that sends setPinWithTrusted message,
ash_unittests
==========

to

==========
Add WM_EVENT_TRUSTED_PIN and WINDOW_STATE_TYPE_TRUSTED_PINNED to Ash.

The event and the state is similar to WM_EVENT_PIN and WINDOW_STATE_TYPE_PINNED
respectively unless TRUSTED_PINNED cannot be exited by user operation.

BUG=b/29325086
TEST=Test with local Android build that sends setPinWithTrusted message,
ash_unittests
==========

[commit-bot: I haz the power, 2016-09-20 05:28:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-20 05:29:41]

Description was changed from

==========
Add WM_EVENT_TRUSTED_PIN and WINDOW_STATE_TYPE_TRUSTED_PINNED to Ash.

The event and the state is similar to WM_EVENT_PIN and WINDOW_STATE_TYPE_PINNED
respectively unless TRUSTED_PINNED cannot be exited by user operation.

BUG=b/29325086
TEST=Test with local Android build that sends setPinWithTrusted message,
ash_unittests
==========

to

==========
Add WM_EVENT_TRUSTED_PIN and WINDOW_STATE_TYPE_TRUSTED_PINNED to Ash.

The event and the state is similar to WM_EVENT_PIN and WINDOW_STATE_TYPE_PINNED
respectively unless TRUSTED_PINNED cannot be exited by user operation.

BUG=b/29325086
TEST=Test with local Android build that sends setPinWithTrusted message,
ash_unittests

Committed: https://crrev.com/94b6193abeb2e2057f17b32da9381767c6b3d9a8
Cr-Commit-Position: refs/heads/master@{#419685}
==========

[commit-bot: I haz the power, 2016-09-20 05:29:42]

Patchset 5 (id:??) landed as
https://crrev.com/94b6193abeb2e2057f17b32da9381767c6b3d9a8
Cr-Commit-Position: refs/heads/master@{#419685}