Implement "pinned" mode in ash.

Implement "pinned" mode in ash.

BUG=b/27995648
TEST=Manually tested with client side implementation.
     Ran ash_unittests.

Committed: https://crrev.com/6821ca4b1c228124e2c8845ad820139635132cba
Cr-Commit-Position: refs/heads/master@{#400590}

[hidehiko, 2016-06-16 19:53:08]

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

[hidehiko, 2016-06-16 19:53:09]

Oshima-san, PTAL.

After the WIP draft CL, rebased on ToT, and added unittests.

https://codereview.chromium.org/2072853002/diff/1/ash/common/wm/workspace/wor...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/1/ash/common/wm/workspace/wor...
ash/common/wm/workspace/workspace_layout_manager.cc:50:
window_state->RemoveObserver(this);
Note: this is a bug fix of the existing code.

[oshima, 2016-06-17 10:26:54]

sorry I fell asleep. This is first half.

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

https://codereview.chromium.org/2072853002/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:1408: }
won't this also prevent power/audio accelerators?

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:37:
is_pinned_(WmShell::Get()->IsPinned()) {
I'd prefer to just use WmShell::IsPinned() because you don' thave to keep it in
sync.

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc#newcode1024
ash/shell.cc:1024: pinned_controller_.reset(new wm::PinnedController);
could you please reset explicitly?

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc#newcode1026
ash/shell.cc:1026:
window_tree_host_manager_->AddObserver(pinned_controller_.get());
Can you do this in PinnedController ctor , and remove in dtor?

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/overview/window_...
ash/wm/overview/window_selector_controller.cc:36: !WmShell::Get()->IsPinned() &&
nice catch.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
File ash/wm/pinned_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:53: }
use ContainsValue in stl_util.h

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:68: << "the pinned mode";
can this happen? If not, DCHECK

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:80: container->StackChildBelow(background_window_,
pinned_window);
Another option is to create a layer above, and use NinePatchLayer to create a
black layer with a hole.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:86: system_modal->StackChildAtTop(dim_window);
This may hide existing modal window if any.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:88: }
bye the way, we can't (and shouldn't) suppress screen lock, in which case the
background will be visible. Is that concern?

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:101: << "target window.";
ditto

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

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:19: namespace wm {
Let's keep it just ash. I know it's not consistent, but most controllers are in
just ash::.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:22: class PinnedController : public ShellObserver,
ScreenPinningController would be better because it's consistent with the
feature.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:34: WmWindow* pinned_window_ = nullptr;
new line. same for below.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:53: // WmWindowObserver override.
nit: // WmWindowObserver:

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:61: // WindowTreeHostManager::Observer override.
// WindowTreeHostManager::Observer:

[oshima, 2016-06-17 10:56:51]

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
File ash/wm/pinned_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:146: // If a window is being removed from the
parent, stop observing it.
new_parent == nullptr is removal.

I actually wonder if you should use WindowStackingClient instead. You need to
keep the existing one for transient, but you can insert yours in between (and
reset when un-pinned)

That way, you don't have to listen too all windows.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:158: // handled.
What if someone add a window to AlwaysOnTop?

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:186: void
PinnedController::OnWindowDestroying(WmWindow* window) {
or you can listen to OnWndowAdded/Removed on container.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
File ash/wm/pinned_controller_unittest.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller_unittest.cc:23: const aura::Window* target) {
you can get index using

auto iter = std::find(...);
return iter - windows.begin();

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:67:
viewport_->RemovePreTargetHandler(event_handler_.get());
is there any reason for this reorder?

[hidehiko, 2016-06-17 17:19:23]

Thank you for review. PTAL.

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

https://codereview.chromium.org/2072853002/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.cc:1408: }
On 2016/06/17 10:26:53, oshima wrote:
> won't this also prevent power/audio accelerators?

Good point. I added the table, based on the reference device's behavior. It
should be somehow reasonable, but maybe we want to update it in a follow up CL.

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:37:
is_pinned_(WmShell::Get()->IsPinned()) {
On 2016/06/17 10:26:53, oshima wrote:
> I'd prefer to just use WmShell::IsPinned() because you don' thave to keep it
in
> sync.

It looks unsafe. The order of the callbacks for OnPinnedStateChanged() looks not
guaranteed, so, when OnPinnedStateChanged() is called it is not guaranteed that
IsPinned() can return the expected value. Is there any better solution for that?

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc#newcode1024
ash/shell.cc:1024: pinned_controller_.reset(new wm::PinnedController);
On 2016/06/17 10:26:54, oshima wrote:
> could you please reset explicitly?

It is already explicit?

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc#newcode1026
ash/shell.cc:1026:
window_tree_host_manager_->AddObserver(pinned_controller_.get());
On 2016/06/17 10:26:54, oshima wrote:
> Can you do this in PinnedController ctor , and remove in dtor?

Done.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/overview/window_...
File ash/wm/overview/window_selector_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/overview/window_...
ash/wm/overview/window_selector_controller.cc:36: !WmShell::Get()->IsPinned() &&
On 2016/06/17 10:26:54, oshima wrote:
> nice catch. 

Acknowledged.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
File ash/wm/pinned_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:53: }
On 2016/06/17 10:26:54, oshima wrote:
> use ContainsValue in stl_util.h

Good to know. Done.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:68: << "the pinned mode";
On 2016/06/17 10:26:54, oshima wrote:
> can this happen? If not, DCHECK

Replaced by LOG(DFATAL).

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:80: container->StackChildBelow(background_window_,
pinned_window);
On 2016/06/17 10:26:54, oshima wrote:
> Another option is to create a layer above, and use NinePatchLayer to create a
> black layer with a hole.

Can we keep it?
The window can have a transparent region, so it looks nicer to have the
background.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:86: system_modal->StackChildAtTop(dim_window);
On 2016/06/17 10:26:54, oshima wrote:
> This may hide existing modal window if any.

Good catch. As we decided not to hide system modal windows in the target window
tree, I changed this to put at the bottom of the system modal container.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:88: }
On 2016/06/17 10:26:54, oshima wrote:
> bye the way, we can't (and shouldn't) suppress screen lock, in which case the
> background will be visible. Is that concern?

No, as it is consistent with the referenced behavior. Also, it is not in the
requirements.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:101: << "target window.";
On 2016/06/17 10:26:54, oshima wrote:
> ditto

Done.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:146: // If a window is being removed from the
parent, stop observing it.
On 2016/06/17 10:56:51, oshima wrote:
> new_parent == nullptr is removal.
> 
> I actually wonder if you should use WindowStackingClient instead. You need to
> keep the existing one for transient, but you can insert yours in between (and
> reset when un-pinned)
> 
> That way, you don't have to listen too all windows.

Good to know. I'm tempted to use it, but it is a singleton, and it looks just
implemented for the transient windows.
So, can we move forward with the current one? We may be able to refactor later.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:158: // handled.
On 2016/06/17 10:56:51, oshima wrote:
> What if someone add a window to AlwaysOnTop?

Good catch. It should be handled by WorkspaceLayoutManager, considering the
current implementation for the fullscreen. However, it looks to have a bug in
it. I'll fix it later.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:186: void
PinnedController::OnWindowDestroying(WmWindow* window) {
On 2016/06/17 10:56:51, oshima wrote:
> or you can listen to OnWndowAdded/Removed on container.

Done. Though it needs to fully change the callback structures.

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

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:19: namespace wm {
On 2016/06/17 10:26:54, oshima wrote:
> Let's keep it just ash. I know it's not consistent, but most controllers are
in
> just ash::.

Done.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:22: class PinnedController : public ShellObserver,
On 2016/06/17 10:26:54, oshima wrote:
> ScreenPinningController would be better because it's consistent with the
> feature.

Done.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:34: WmWindow* pinned_window_ = nullptr;
On 2016/06/17 10:26:54, oshima wrote:
> new line. same for below.

Done.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:53: // WmWindowObserver override.
On 2016/06/17 10:26:54, oshima wrote:
> nit: // WmWindowObserver:

Acknowledged.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.h:61: // WindowTreeHostManager::Observer override.
On 2016/06/17 10:26:54, oshima wrote:
> // WindowTreeHostManager::Observer:

Done.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
File ash/wm/pinned_controller_unittest.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller_unittest.cc:23: const aura::Window* target) {
On 2016/06/17 10:56:51, oshima wrote:
> you can get index using
> 
> auto iter = std::find(...);
> return iter - windows.begin();

Done.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:67:
viewport_->RemovePreTargetHandler(event_handler_.get());
On 2016/06/17 10:56:51, oshima wrote:
> is there any reason for this reorder?

In sane manner, the order of callback unregistration should be the reversed
order of the registration. Though, it is not very important for this CL, so I
just reverted it.

[oshima, 2016-06-18 04:47:44]

I have a few more, and will send them separately.

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:37:
is_pinned_(WmShell::Get()->IsPinned()) {
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:26:53, oshima wrote:
> > I'd prefer to just use WmShell::IsPinned() because you don' thave to keep it
> in
> > sync.
> 
> It looks unsafe. The order of the callbacks for OnPinnedStateChanged() looks
not
> guaranteed, so, when OnPinnedStateChanged() is called it is not guaranteed
that
> IsPinned() can return the expected value.

Can you explain how? OnPinnedStateChanged must be called AFTER it's changed, so
must be safe.

> Is there any better solution for that?

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc#newcode1024
ash/shell.cc:1024: pinned_controller_.reset(new wm::PinnedController);
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:26:54, oshima wrote:
> > could you please reset explicitly?
> 
> It is already explicit?

Sorry I think I need more sleep.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
File ash/wm/pinned_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:80: container->StackChildBelow(background_window_,
pinned_window);
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:26:54, oshima wrote:
> > Another option is to create a layer above, and use NinePatchLayer to create
a
> > black layer with a hole.
> 
> Can we keep it?
> The window can have a transparent region, so it looks nicer to have the
> background.

It'll be visually same. It's not a big deal so if you're ok, then ok.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:88: }
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:26:54, oshima wrote:
> > bye the way, we can't (and shouldn't) suppress screen lock, in which case
the
> > background will be visible. Is that concern?
> 
> No, as it is consistent with the referenced behavior. Also, it is not in the
> requirements.

Acknowledged.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:146: // If a window is being removed from the
parent, stop observing it.
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:56:51, oshima wrote:
> > new_parent == nullptr is removal.
> > 
> > I actually wonder if you should use WindowStackingClient instead. You need
to
> > keep the existing one for transient, but you can insert yours in between
(and
> > reset when un-pinned)
> > 
> > That way, you don't have to listen too all windows.
> 
> Good to know. I'm tempted to use it, but it is a singleton, and it looks just
> implemented for the transient windows.
> So, can we move forward with the current one? We may be able to refactor
later.

You can use it for this type of feature too. Good thing is that it's get called
*before* window has restacked, so you don't have to deal with in_restacking_
stuff.

We can revisit this later if necessary.

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

https://codereview.chromium.org/2072853002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_table.cc:556: const AcceleratorAction
kActionsAllowedInPinnedMode[] = {
This disables accelerators for IME, accessibility etc.

Can't we just use ones for locked state?

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

https://codereview.chromium.org/2072853002/diff/80001/ash/common/wm/default_s...
ash/common/wm/default_state.cc:212: next_state_type = WmShell::Get()->IsPinned()
? current_state_type
you may want to log when the request wasn't fulfilled.

https://codereview.chromium.org/2072853002/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:316: } else if (state_.is_pinned) {
can't we just use WmShell::Get()->IsPinned()?

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

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:20: namespace test {
we keep ash::test for test utility (at least in ash).

just use ash:: so that we can detect duplicated test nave if any.

same for the rest.

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:35: };
using ScreenPinningControllerTest = test::AshTestBase;

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:71: EXPECT_NE(index1, -1);
It should be:

EXPECTE_NE(expected_value, actual_value);

that's what test harness expect when creating failure message.

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:62:
WmShell::Get()->AddShellObserver(layout_manager_);
can we do this in layout manager's ctor/dtor?

[oshima, 2016-06-18 04:47:45]

I have a few more, and will send them separately.

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:37:
is_pinned_(WmShell::Get()->IsPinned()) {
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:26:53, oshima wrote:
> > I'd prefer to just use WmShell::IsPinned() because you don' thave to keep it
> in
> > sync.
> 
> It looks unsafe. The order of the callbacks for OnPinnedStateChanged() looks
not
> guaranteed, so, when OnPinnedStateChanged() is called it is not guaranteed
that
> IsPinned() can return the expected value.

Can you explain how? OnPinnedStateChanged must be called AFTER it's changed, so
must be safe.

> Is there any better solution for that?

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/shell.cc#newcode1024
ash/shell.cc:1024: pinned_controller_.reset(new wm::PinnedController);
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:26:54, oshima wrote:
> > could you please reset explicitly?
> 
> It is already explicit?

Sorry I think I need more sleep.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
File ash/wm/pinned_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:80: container->StackChildBelow(background_window_,
pinned_window);
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:26:54, oshima wrote:
> > Another option is to create a layer above, and use NinePatchLayer to create
a
> > black layer with a hole.
> 
> Can we keep it?
> The window can have a transparent region, so it looks nicer to have the
> background.

It'll be visually same. It's not a big deal so if you're ok, then ok.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:88: }
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:26:54, oshima wrote:
> > bye the way, we can't (and shouldn't) suppress screen lock, in which case
the
> > background will be visible. Is that concern?
> 
> No, as it is consistent with the referenced behavior. Also, it is not in the
> requirements.

Acknowledged.

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:146: // If a window is being removed from the
parent, stop observing it.
On 2016/06/17 17:19:22, hidehiko wrote:
> On 2016/06/17 10:56:51, oshima wrote:
> > new_parent == nullptr is removal.
> > 
> > I actually wonder if you should use WindowStackingClient instead. You need
to
> > keep the existing one for transient, but you can insert yours in between
(and
> > reset when un-pinned)
> > 
> > That way, you don't have to listen too all windows.
> 
> Good to know. I'm tempted to use it, but it is a singleton, and it looks just
> implemented for the transient windows.
> So, can we move forward with the current one? We may be able to refactor
later.

You can use it for this type of feature too. Good thing is that it's get called
*before* window has restacked, so you don't have to deal with in_restacking_
stuff.

We can revisit this later if necessary.

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

https://codereview.chromium.org/2072853002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_table.cc:556: const AcceleratorAction
kActionsAllowedInPinnedMode[] = {
This disables accelerators for IME, accessibility etc.

Can't we just use ones for locked state?

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

https://codereview.chromium.org/2072853002/diff/80001/ash/common/wm/default_s...
ash/common/wm/default_state.cc:212: next_state_type = WmShell::Get()->IsPinned()
? current_state_type
you may want to log when the request wasn't fulfilled.

https://codereview.chromium.org/2072853002/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:316: } else if (state_.is_pinned) {
can't we just use WmShell::Get()->IsPinned()?

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

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
nit: 2016

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:20: namespace test {
we keep ash::test for test utility (at least in ash).

just use ash:: so that we can detect duplicated test nave if any.

same for the rest.

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:35: };
using ScreenPinningControllerTest = test::AshTestBase;

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:71: EXPECT_NE(index1, -1);
It should be:

EXPECTE_NE(expected_value, actual_value);

that's what test harness expect when creating failure message.

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:62:
WmShell::Get()->AddShellObserver(layout_manager_);
can we do this in layout manager's ctor/dtor?

[hidehiko, 2016-06-18 05:32:46]

Thank you for review. PTAL.

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:37:
is_pinned_(WmShell::Get()->IsPinned()) {
On 2016/06/18 04:47:44, oshima wrote:
> On 2016/06/17 17:19:22, hidehiko wrote:
> > On 2016/06/17 10:26:53, oshima wrote:
> > > I'd prefer to just use WmShell::IsPinned() because you don' thave to keep
it
> > in
> > > sync.
> > 
> > It looks unsafe. The order of the callbacks for OnPinnedStateChanged() looks
> not
> > guaranteed, so, when OnPinnedStateChanged() is called it is not guaranteed
> that
> > IsPinned() can return the expected value.
> 
> Can you explain how? OnPinnedStateChanged must be called AFTER it's changed,
so
> must be safe.
> 
> > Is there any better solution for that?
> 

The trigger is an event to the WindowState, and it notifies to the controllers
via ShellObserver. One of them, ScreenPinningController observes the event and
then set the pinned state, then IsPinned returns the correct value.

If we need to OnPinnedStateChanged is propagated after ScreenPinningController
is updated, then it is needed to have another event to communicate between
WindowState -> WmShell/Shell -> ScreenPinningController, then
ScreenPinningController sends OnPinnedStateChanged event to ShellObserver. Or,
is there a dedicate way in ash, though I couldn't find...

Can we keep this for now? Instead, I added TODO (in header).

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
File ash/wm/pinned_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/wm/pinned_controlle...
ash/wm/pinned_controller.cc:146: // If a window is being removed from the
parent, stop observing it.
On 2016/06/18 04:47:44, oshima wrote:
> On 2016/06/17 17:19:22, hidehiko wrote:
> > On 2016/06/17 10:56:51, oshima wrote:
> > > new_parent == nullptr is removal.
> > > 
> > > I actually wonder if you should use WindowStackingClient instead. You need
> to
> > > keep the existing one for transient, but you can insert yours in between
> (and
> > > reset when un-pinned)
> > > 
> > > That way, you don't have to listen too all windows.
> > 
> > Good to know. I'm tempted to use it, but it is a singleton, and it looks
just
> > implemented for the transient windows.
> > So, can we move forward with the current one? We may be able to refactor
> later.
> 
> You can use it for this type of feature too. Good thing is that it's get
called
> *before* window has restacked, so you don't have to deal with in_restacking_
> stuff.

Yes, that is why I was actually tempted to use.

> 
> We can revisit this later if necessary.

Acknowledged. Let's revisit here later if needed.

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

https://codereview.chromium.org/2072853002/diff/80001/ash/accelerators/accele...
ash/accelerators/accelerator_table.cc:556: const AcceleratorAction
kActionsAllowedInPinnedMode[] = {
On 2016/06/18 04:47:44, oshima wrote:
> This disables accelerators for IME, accessibility etc.
> 
> Can't we just use ones for locked state?

Ah, good point. I shared some with in_app_mode ones.
Though, we should have a bit more, so I keep extra ones in this table.

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

https://codereview.chromium.org/2072853002/diff/80001/ash/common/wm/default_s...
ash/common/wm/default_state.cc:212: next_state_type = WmShell::Get()->IsPinned()
? current_state_type
On 2016/06/18 04:47:44, oshima wrote:
> you may want to log when the request wasn't fulfilled.

Done.

https://codereview.chromium.org/2072853002/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.cc:316: } else if (state_.is_pinned) {
On 2016/06/18 04:47:44, oshima wrote:
> can't we just use WmShell::Get()->IsPinned()?

Ditto for workspalce_layout_manager.

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

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2016/06/18 04:47:44, oshima wrote:
> nit: 2016

Done.

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:20: namespace test {
On 2016/06/18 04:47:45, oshima wrote:
> we keep ash::test for test utility (at least in ash).
> 
> just use ash:: so that we can detect duplicated test nave if any.
> 
> same for the rest.

Done.

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:35: };
On 2016/06/18 04:47:44, oshima wrote:
> using ScreenPinningControllerTest = test::AshTestBase;

Done.

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/screen_pinning_c...
ash/wm/screen_pinning_controller_unittest.cc:71: EXPECT_NE(index1, -1);
On 2016/06/18 04:47:44, oshima wrote:
> It should be:
> 
> EXPECTE_NE(expected_value, actual_value);
> 
> that's what test harness expect when creating failure message.

Done.

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/workspace_contro...
File ash/wm/workspace_controller.cc (right):

https://codereview.chromium.org/2072853002/diff/80001/ash/wm/workspace_contro...
ash/wm/workspace_controller.cc:62:
WmShell::Get()->AddShellObserver(layout_manager_);
On 2016/06/18 04:47:45, oshima wrote:
> can we do this in layout manager's ctor/dtor?

Done.

[oshima, 2016-06-18 06:24:10]

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:37:
is_pinned_(WmShell::Get()->IsPinned()) {
On 2016/06/18 05:32:45, hidehiko wrote:
> On 2016/06/18 04:47:44, oshima wrote:
> > On 2016/06/17 17:19:22, hidehiko wrote:
> > > On 2016/06/17 10:26:53, oshima wrote:
> > > > I'd prefer to just use WmShell::IsPinned() because you don' thave to
keep
> it
> > > in
> > > > sync.
> > > 
> > > It looks unsafe. The order of the callbacks for OnPinnedStateChanged()
looks
> > not
> > > guaranteed, so, when OnPinnedStateChanged() is called it is not guaranteed
> > that
> > > IsPinned() can return the expected value.
> > 
> > Can you explain how? OnPinnedStateChanged must be called AFTER it's changed,
> so
> > must be safe.
> > 
> > > Is there any better solution for that?
> > 
> 
> The trigger is an event to the WindowState, and it notifies to the controllers
> via ShellObserver. One of them, ScreenPinningController observes the event and
> then set the pinned state, then IsPinned returns the correct value.
> 
> If we need to OnPinnedStateChanged is propagated after ScreenPinningController
> is updated, then it is needed to have another event to communicate between
> WindowState -> WmShell/Shell -> ScreenPinningController, then
> ScreenPinningController sends OnPinnedStateChanged event to ShellObserver. Or,
> is there a dedicate way in ash, though I couldn't find...
> 
> Can we keep this for now? Instead, I added TODO (in header).

I think you should change the following say

1) WindowState calls ScreenPinningContoller::SetPinnedWindow()
2) ScreenPinningCOntroller calls Shell::NotifyPinnedStateChanged()

this way, you can keep the state consistent.

[oshima, 2016-06-18 06:24:10]

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:37:
is_pinned_(WmShell::Get()->IsPinned()) {
On 2016/06/18 05:32:45, hidehiko wrote:
> On 2016/06/18 04:47:44, oshima wrote:
> > On 2016/06/17 17:19:22, hidehiko wrote:
> > > On 2016/06/17 10:26:53, oshima wrote:
> > > > I'd prefer to just use WmShell::IsPinned() because you don' thave to
keep
> it
> > > in
> > > > sync.
> > > 
> > > It looks unsafe. The order of the callbacks for OnPinnedStateChanged()
looks
> > not
> > > guaranteed, so, when OnPinnedStateChanged() is called it is not guaranteed
> > that
> > > IsPinned() can return the expected value.
> > 
> > Can you explain how? OnPinnedStateChanged must be called AFTER it's changed,
> so
> > must be safe.
> > 
> > > Is there any better solution for that?
> > 
> 
> The trigger is an event to the WindowState, and it notifies to the controllers
> via ShellObserver. One of them, ScreenPinningController observes the event and
> then set the pinned state, then IsPinned returns the correct value.
> 
> If we need to OnPinnedStateChanged is propagated after ScreenPinningController
> is updated, then it is needed to have another event to communicate between
> WindowState -> WmShell/Shell -> ScreenPinningController, then
> ScreenPinningController sends OnPinnedStateChanged event to ShellObserver. Or,
> is there a dedicate way in ash, though I couldn't find...
> 
> Can we keep this for now? Instead, I added TODO (in header).

I think you should change the following say

1) WindowState calls ScreenPinningContoller::SetPinnedWindow()
2) ScreenPinningCOntroller calls Shell::NotifyPinnedStateChanged()

this way, you can keep the state consistent.

[hidehiko, 2016-06-18 06:25:51]

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/20001/ash/common/wm/workspace...
ash/common/wm/workspace/workspace_layout_manager.cc:37:
is_pinned_(WmShell::Get()->IsPinned()) {
On 2016/06/18 06:24:10, oshima wrote:
> On 2016/06/18 05:32:45, hidehiko wrote:
> > On 2016/06/18 04:47:44, oshima wrote:
> > > On 2016/06/17 17:19:22, hidehiko wrote:
> > > > On 2016/06/17 10:26:53, oshima wrote:
> > > > > I'd prefer to just use WmShell::IsPinned() because you don' thave to
> keep
> > it
> > > > in
> > > > > sync.
> > > > 
> > > > It looks unsafe. The order of the callbacks for OnPinnedStateChanged()
> looks
> > > not
> > > > guaranteed, so, when OnPinnedStateChanged() is called it is not
guaranteed
> > > that
> > > > IsPinned() can return the expected value.
> > > 
> > > Can you explain how? OnPinnedStateChanged must be called AFTER it's
changed,
> > so
> > > must be safe.
> > > 
> > > > Is there any better solution for that?
> > > 
> > 
> > The trigger is an event to the WindowState, and it notifies to the
controllers
> > via ShellObserver. One of them, ScreenPinningController observes the event
and
> > then set the pinned state, then IsPinned returns the correct value.
> > 
> > If we need to OnPinnedStateChanged is propagated after
ScreenPinningController
> > is updated, then it is needed to have another event to communicate between
> > WindowState -> WmShell/Shell -> ScreenPinningController, then
> > ScreenPinningController sends OnPinnedStateChanged event to ShellObserver.
Or,
> > is there a dedicate way in ash, though I couldn't find...
> > 
> > Can we keep this for now? Instead, I added TODO (in header).
> 
> I think you should change the following say
> 
> 1) WindowState calls ScreenPinningContoller::SetPinnedWindow()
> 2) ScreenPinningCOntroller calls Shell::NotifyPinnedStateChanged()
> 
> this way, you can keep the state consistent.

We cannot, now. WindowState cannot access to ScreenPinningController. It is
under ash/common but it is prohibited to access ash::Shell(). It needs refactor
or DEPS exception.

[oshima, 2016-06-18 06:36:10]

https://codereview.chromium.org/2072853002/diff/130001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/130001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager.cc:84:
WindowPositioner::RearrangeVisibleWindowOnShow(child);
I think you also need to update here.

[oshima, 2016-06-18 06:43:03]

https://codereview.chromium.org/2072853002/diff/130001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/2072853002/diff/130001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:467: const AcceleratorAction
kActionsAllowedInAppMode[] = {
...InAppModeOrPinnedMode

[hidehiko, 2016-06-18 08:32:13]

Per offline chat, added SetPinnedWindow to WmShell.

PTAL.

https://codereview.chromium.org/2072853002/diff/130001/ash/accelerators/accel...
File ash/accelerators/accelerator_table.cc (right):

https://codereview.chromium.org/2072853002/diff/130001/ash/accelerators/accel...
ash/accelerators/accelerator_table.cc:467: const AcceleratorAction
kActionsAllowedInAppMode[] = {
On 2016/06/18 06:43:03, oshima wrote:
> ...InAppModeOrPinnedMode

Done.

https://codereview.chromium.org/2072853002/diff/130001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/130001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager.cc:84:
WindowPositioner::RearrangeVisibleWindowOnShow(child);
On 2016/06/18 06:36:10, oshima wrote:
> I think you also need to update here.

Done.

[oshima, 2016-06-18 08:41:39]

https://codereview.chromium.org/2072853002/diff/150001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/150001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager.cc:182: }
nuke {}

https://codereview.chromium.org/2072853002/diff/150001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager.cc:296: // window state change.
new always-on-stop may be created during fullscreen.

[hidehiko, 2016-06-18 09:33:34]

Thank you for review. PTAL.

https://codereview.chromium.org/2072853002/diff/150001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager.cc (right):

https://codereview.chromium.org/2072853002/diff/150001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager.cc:182: }
On 2016/06/18 08:41:39, oshima wrote:
> nuke {}

Done.

https://codereview.chromium.org/2072853002/diff/150001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager.cc:296: // window state change.
On 2016/06/18 08:41:39, oshima wrote:
> new always-on-stop may be created during fullscreen.

Done.

[oshima, 2016-06-18 09:42:00]

lgtm

thanks!

[hidehiko, 2016-06-18 11:08:30]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2016-06-18 11:08:36]

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

[commit-bot: I haz the power, 2016-06-18 11:12:37]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2016-06-18 11:14:22]

Description was changed from

==========
Implement "pinned" mode in ash.

BUG=b/27995648
TEST=Manually tested with client side implementation.
     Ran ash_unittests.
==========

to

==========
Implement "pinned" mode in ash.

BUG=b/27995648
TEST=Manually tested with client side implementation.
     Ran ash_unittests.

Committed: https://crrev.com/6821ca4b1c228124e2c8845ad820139635132cba
Cr-Commit-Position: refs/heads/master@{#400590}
==========

[commit-bot: I haz the power, 2016-06-18 11:14:23]

Patchset 10 (id:??) landed as
https://crrev.com/6821ca4b1c228124e2c8845ad820139635132cba
Cr-Commit-Position: refs/heads/master@{#400590}