Handle UNPIN operation in ash.

Handle UNPIN operation in ash.

With this CL, Chrome will have an ability to force unpinning the pinned window.

BUG=b/27995648
TEST=ash_unittests. Ran manually.

Committed: https://crrev.com/fe0dc745d3c7dd42248a3a111353d48597a82d06
Cr-Commit-Position: refs/heads/master@{#400592}

[hidehiko, 2016-06-17 18:20:29]

Patchset #2 (id:20001) has been deleted

[hidehiko, 2016-06-17 18:22:03]

Description was changed from

==========
WIP: Handle UNPIN operation in ash.
==========

to

==========
Handle UNPIN operation in ash.

With this CL, Chrome will have an ability to force unpinning the pinned window.

BUG=b/27995648	
TEST=ash_unittests. Ran manually.
==========

[hidehiko, 2016-06-17 18:27:07]

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

[hidehiko, 2016-06-17 18:27:07]

PS1 is the PS5 (latest) of crrev.com/2072853002.
The diff between PS1 and PS2 is the content of this CL.

Could you take a look?

[oshima, 2016-06-18 04:18:13]

looks ok, except that accelerator table. Let's discuss that in the other CL.

https://codereview.chromium.org/2078883003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/2078883003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_commands.cc:111: wm::WindowState* window_state =
wm::GetActiveWindowState();
Didn't we have a way to get pinned window?

https://codereview.chromium.org/2078883003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/2078883003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_table.cc:129: { true, ui::VKEY_ESCAPE,
ui::EF_SHIFT_DOWN | ui::EF_COMMAND_DOWN, UNPIN },
New shortcuts should be assigned with search key.
I'm ok to land this as temporary, but please contact UX/PM and discuss which key
this should be assigned.

(I also wonder if Android has something similar/equivalent shortcut)

[hidehiko, 2016-06-18 05:38:17]

Thank you for review. Rebased. PTAL.

https://codereview.chromium.org/2078883003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/2078883003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_commands.cc:111: wm::WindowState* window_state =
wm::GetActiveWindowState();
On 2016/06/18 04:18:13, oshima wrote:
> Didn't we have a way to get pinned window?

No from here.
We can add, but for consistency with other accelerator keys, which communicate
with an Active Window, this sounds ok?

https://codereview.chromium.org/2078883003/diff/60001/ash/accelerators/accele...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/2078883003/diff/60001/ash/accelerators/accele...
ash/accelerators/accelerator_table.cc:129: { true, ui::VKEY_ESCAPE,
ui::EF_SHIFT_DOWN | ui::EF_COMMAND_DOWN, UNPIN },
On 2016/06/18 04:18:13, oshima wrote:
> New shortcuts should be assigned with search key.
> I'm ok to land this as temporary, but please contact UX/PM and discuss which
key
> this should be assigned.
> 
> (I also wonder if Android has something similar/equivalent shortcut)
> 

I already agreed with UX/PM to use ESC + SHIFT + SEARCH key combination for
this.

[oshima, 2016-06-18 09:48:22]

lgtm with one suggestion

https://codereview.chromium.org/2078883003/diff/120001/ash/accelerators/accel...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/2078883003/diff/120001/ash/accelerators/accel...
ash/accelerators/accelerator_commands.cc:111: wm::WindowState* window_state =
wm::GetActiveWindowState();
I think it's safer to get pinned window from ScreenPinningController.
It *should* be same, but active window can change even during pinned (system
modal, lock screen are example)

[hidehiko, 2016-06-18 10:02:23]

Thank you for review.
Will send to CQ, after the depended CL is landed.

https://codereview.chromium.org/2078883003/diff/120001/ash/accelerators/accel...
File ash/accelerators/accelerator_commands.cc (right):

https://codereview.chromium.org/2078883003/diff/120001/ash/accelerators/accel...
ash/accelerators/accelerator_commands.cc:111: wm::WindowState* window_state =
wm::GetActiveWindowState();
On 2016/06/18 09:48:22, oshima wrote:
> I think it's safer to get pinned window from ScreenPinningController.
> It *should* be same, but active window can change even during pinned (system
> modal, lock screen are example)

I see. Done.

[hidehiko, 2016-06-18 12:40:41]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2016-06-18 12:40:42]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2078883003/#ps140001
(title: "address comments")

[commit-bot: I haz the power, 2016-06-18 12:40:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2078883003/140001

[commit-bot: I haz the power, 2016-06-18 13:34:33]

Description was changed from

==========
Handle UNPIN operation in ash.

With this CL, Chrome will have an ability to force unpinning the pinned window.

BUG=b/27995648	
TEST=ash_unittests. Ran manually.
==========

to

==========
Handle UNPIN operation in ash.

With this CL, Chrome will have an ability to force unpinning the pinned window.

BUG=b/27995648	
TEST=ash_unittests. Ran manually.
==========

[commit-bot: I haz the power, 2016-06-18 13:34:34]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-06-18 13:36:21]

Description was changed from

==========
Handle UNPIN operation in ash.

With this CL, Chrome will have an ability to force unpinning the pinned window.

BUG=b/27995648	
TEST=ash_unittests. Ran manually.
==========

to

==========
Handle UNPIN operation in ash.

With this CL, Chrome will have an ability to force unpinning the pinned window.

BUG=b/27995648
TEST=ash_unittests. Ran manually.

Committed: https://crrev.com/fe0dc745d3c7dd42248a3a111353d48597a82d06
Cr-Commit-Position: refs/heads/master@{#400592}
==========

[commit-bot: I haz the power, 2016-06-18 13:36:22]

Patchset 7 (id:??) landed as
https://crrev.com/fe0dc745d3c7dd42248a3a111353d48597a82d06
Cr-Commit-Position: refs/heads/master@{#400592}