Set focusibility correctly when initializing a window in mus+ash.

Set focusibility correctly when initializing a window in mus+ash.

Saves |init_params.CanActivate()| in mus properties, and calls
SetCanFocus() from WM just after creating the window.

BUG=684048
Review-Url: https://codereview.chromium.org/2650833002
Cr-Commit-Position: refs/heads/master@{#446460}
Committed: https://chromium.googlesource.com/chromium/src/+/c17142e8e1a64aa75c26b843c2ed23ebec9d39b9

[Hadi, 2017-01-23 20:28:08]

Description was changed from

==========
Set focusibility correctly when initializing a window in mus+ash.

BUG=
==========

to

==========
Set focusibility correctly when initializing a window in mus+ash.

Saves |init_params.CanActivate()| in mus properties, and calls
SetCanFocus() from WM just after creating the window.

BUG=684048
==========

[Hadi, 2017-01-23 20:28:20]

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

[commit-bot: I haz the power, 2017-01-23 20:28:45]

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

[Hadi, 2017-01-23 20:30:29]

moshayedi@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[Hadi, 2017-01-23 20:30:29]

PTAL.

If this CL looks fine, I'll fix init_params.accept_events in a follow-up CL.

Thanks.

[sky, 2017-01-23 21:28:29]

https://codereview.chromium.org/2650833002/diff/1/services/ui/public/interfac...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2650833002/diff/1/services/ui/public/interfac...
services/ui/public/interfaces/window_manager.mojom:39: const string
kActivatable_InitProperty = "init:activatable";
Name this Focusable to match what is changes.

https://codereview.chromium.org/2650833002/diff/1/services/ui/ws/window_tree.cc
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2650833002/diff/1/services/ui/ws/window_tree....
services/ui/ws/window_tree.cc:1550: if (window &&
access_policy_->CanSetFocus(window))
This needs to go to the window manager too, otherwise it won't be in sync with
the client.

https://codereview.chromium.org/2650833002/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2650833002/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:1452: auto activatable_iter =
The WindowManagerDelegate should be responsible for applying this, not the
windowtreeclient. To do it here means all clients want to honor the property,
some may not.

https://codereview.chromium.org/2650833002/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/2650833002/diff/1/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:144: bool Widget::InitParams::CanActivate() const {
Are you sure you need to create a function for this? Isn't activatable be set to
yes/no before MusClient::ConfigurePropertiesFromParams is called?

https://codereview.chromium.org/2650833002/diff/1/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:147: } else {
no else after a return (see style guide).

[commit-bot: I haz the power, 2017-01-23 21:36:05]

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

[commit-bot: I haz the power, 2017-01-23 21:36:06]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-01-24 17:34:49]

https://codereview.chromium.org/2650833002/diff/1/services/ui/ws/window_tree.cc
File services/ui/ws/window_tree.cc (left):

https://codereview.chromium.org/2650833002/diff/1/services/ui/ws/window_tree....
services/ui/ws/window_tree.cc:1551: window->set_can_focus(can_focus);
if (!window)
  return;

if (ShouldRouteToWndowManager(window) || access_policy_->CanSetFocus(window))
  window->set_can_focus(can_focus);

https://codereview.chromium.org/2650833002/diff/1/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2650833002/diff/1/ui/views/mus/mus_client.cc#...
ui/views/mus/mus_client.cc:155:
mojo::ConvertTo<std::vector<uint8_t>>(init_params.CanActivate());
Instead of introducing a property for this, can mus-client just use the
WindowTree::SetCanFocus() API?

[sadrul, 2017-01-24 17:35:23]

https://codereview.chromium.org/2650833002/diff/1/services/ui/ws/window_tree.cc
File services/ui/ws/window_tree.cc (left):

https://codereview.chromium.org/2650833002/diff/1/services/ui/ws/window_tree....
services/ui/ws/window_tree.cc:1551: window->set_can_focus(can_focus);
On 2017/01/24 17:34:49, sadrul wrote:
> if (!window)
>   return;
> 
> if (ShouldRouteToWndowManager(window) || access_policy_->CanSetFocus(window))
>   window->set_can_focus(can_focus);

Whoops, sorry. Please ignore this comment.

[Hadi, 2017-01-24 18:42:33]

https://codereview.chromium.org/2650833002/diff/1/services/ui/public/interfac...
File services/ui/public/interfaces/window_manager.mojom (right):

https://codereview.chromium.org/2650833002/diff/1/services/ui/public/interfac...
services/ui/public/interfaces/window_manager.mojom:39: const string
kActivatable_InitProperty = "init:activatable";
On 2017/01/23 21:28:29, sky wrote:
> Name this Focusable to match what is changes.

Done.

https://codereview.chromium.org/2650833002/diff/1/services/ui/ws/window_tree.cc
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2650833002/diff/1/services/ui/ws/window_tree....
services/ui/ws/window_tree.cc:1550: if (window &&
access_policy_->CanSetFocus(window))
On 2017/01/23 21:28:29, sky wrote:
> This needs to go to the window manager too, otherwise it won't be in sync with
> the client.

Done.

https://codereview.chromium.org/2650833002/diff/1/ui/aura/mus/window_tree_cli...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2650833002/diff/1/ui/aura/mus/window_tree_cli...
ui/aura/mus/window_tree_client.cc:1452: auto activatable_iter =
On 2017/01/23 21:28:29, sky wrote:
> The WindowManagerDelegate should be responsible for applying this, not the
> windowtreeclient. To do it here means all clients want to honor the property,
> some may not.

Done. Implementation of WindowManagerDelegate::OnWmCreateTopLevelWindow() calls
into CreateAndParentTopLevelWindow(). So moved it there.

https://codereview.chromium.org/2650833002/diff/1/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2650833002/diff/1/ui/views/mus/mus_client.cc#...
ui/views/mus/mus_client.cc:155:
mojo::ConvertTo<std::vector<uint8_t>>(init_params.CanActivate());
On 2017/01/24 17:34:49, sadrul wrote:
> Instead of introducing a property for this, can mus-client just use the
> WindowTree::SetCanFocus() API?

WindowTree::SetCanFocus() needs window_id, but I think we don't have it here
yet. Am I missing something?

https://codereview.chromium.org/2650833002/diff/1/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/2650833002/diff/1/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:144: bool Widget::InitParams::CanActivate() const {
On 2017/01/23 21:28:29, sky wrote:
> Are you sure you need to create a function for this? Isn't activatable be set
to
> yes/no before MusClient::ConfigurePropertiesFromParams is called?

I logged activatable at MusClient::ConfigurePropertiesFromParams when running
"chrome --mash". Sometimes it was called with DEFAULT.

https://codereview.chromium.org/2650833002/diff/1/ui/views/widget/widget.cc#n...
ui/views/widget/widget.cc:147: } else {
On 2017/01/23 21:28:29, sky wrote:
> no else after a return (see style guide).

Done.

[sadrul, 2017-01-25 05:31:40]

https://codereview.chromium.org/2650833002/diff/1/ui/views/mus/mus_client.cc
File ui/views/mus/mus_client.cc (right):

https://codereview.chromium.org/2650833002/diff/1/ui/views/mus/mus_client.cc#...
ui/views/mus/mus_client.cc:155:
mojo::ConvertTo<std::vector<uint8_t>>(init_params.CanActivate());
On 2017/01/24 18:42:33, Hadi wrote:
> On 2017/01/24 17:34:49, sadrul wrote:
> > Instead of introducing a property for this, can mus-client just use the
> > WindowTree::SetCanFocus() API?
> 
> WindowTree::SetCanFocus() needs window_id, but I think we don't have it here
> yet. Am I missing something?

The window-id is set pretty early on in the client-side during initialization
[1]. If we notify the server SetCanFocus() right after calling
NewTopLevelWindow() [2], would that work?

1:
https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?type=c...

2:
https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?type=c...

[Hadi, 2017-01-25 21:14:46]

On 2017/01/25 05:31:40, sadrul wrote:
> The window-id is set pretty early on in the client-side during initialization
> [1]. If we notify the server SetCanFocus() right after calling
> NewTopLevelWindow() [2], would that work?
> 
> 1:
>
https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?type=c...
> 
> 2:
>
https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?type=c...

I tried trying to do it near where you said or in the places that calls it, here
are the problems I faced:

(1) I think |window_port->server_id()| in
WindowTreeClient::CreateWindowPortForTopLevel() is a client id. It is saved as
part of WaitingForTopLevelWindowInfo() in WindowTree::NewTopLevelWindow(), and
then is mapped to the actual server id in
WindowTree::OnWindowManagerCreatedTopLevelWindow(). So we don't have the server
id here yet.

(2) In WindowTreeClient::CreateWindowPortForTopLevel() the properties are in
map<string, vector<int_8>> format, so we still need an string id for
|activatable|. We can solve this problem by doing this in
DesktopWindowTreeHostMus::OnNativeWidgetCreated() instead, but we still have the
#1 problem there.

[sky, 2017-01-25 22:29:30]

On 2017/01/25 21:14:46, Hadi wrote:
> On 2017/01/25 05:31:40, sadrul wrote:
> > The window-id is set pretty early on in the client-side during
initialization
> > [1]. If we notify the server SetCanFocus() right after calling
> > NewTopLevelWindow() [2], would that work?
> > 
> > 1:
> >
>
https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?type=c...
> > 
> > 2:
> >
>
https://cs.chromium.org/chromium/src/ui/aura/mus/window_tree_client.cc?type=c...
> 
> I tried trying to do it near where you said or in the places that calls it,
here
> are the problems I faced:
> 
> (1) I think |window_port->server_id()| in
> WindowTreeClient::CreateWindowPortForTopLevel() is a client id.

This is correct. The ids for windows are unique to each client (except for the
window-manager, which sees global ids). The server maintains the mapping between
the client assigned id and the real id.

> It is saved as
> part of WaitingForTopLevelWindowInfo() in WindowTree::NewTopLevelWindow(), and
> then is mapped to the actual server id in
> WindowTree::OnWindowManagerCreatedTopLevelWindow(). So we don't have the
server
> id here yet.

This is true. To make sure we don't have the possibility of a client doing
something with a window the window-manager hasn't yet created the client is
effectively blocked until the window-manager acks the real window. Specifically
this call
https://chromium.googlesource.com/chromium/src/+/master/services/ui/ws/window...
pauses all incoming messages. The messages are resumed once the window-manager
acks creation.

   -Scott

> (2) In WindowTreeClient::CreateWindowPortForTopLevel() the properties are in
> map<string, vector<int_8>> format, so we still need an string id for
> |activatable|. We can solve this problem by doing this in
> DesktopWindowTreeHostMus::OnNativeWidgetCreated() instead, but we still have
the
> #1 problem there.

[sadrul, 2017-01-26 06:27:36]

Ah, I see. Thanks for the explanation! Looks like you are doing the right thing
here.

One comment on what you are doing to address the TODO to notify the
window-manager:

https://codereview.chromium.org/2650833002/diff/40001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2650833002/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1555: wm_tree->SetCanFocus(transport_window_id,
can_focus);
My reading of the TODO is that you want to notify the window manager that
something changed, i.e. you should use the mojom::WindowManager api (much like
what happens you a client sets a property on the window)

[Hadi, 2017-01-26 17:08:54]

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

[commit-bot: I haz the power, 2017-01-26 17:09:19]

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

[Hadi, 2017-01-26 17:34:04]

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

[commit-bot: I haz the power, 2017-01-26 17:34:19]

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

[Hadi, 2017-01-26 17:34:25]

https://codereview.chromium.org/2650833002/diff/40001/services/ui/ws/window_t...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2650833002/diff/40001/services/ui/ws/window_t...
services/ui/ws/window_tree.cc:1555: wm_tree->SetCanFocus(transport_window_id,
can_focus);
On 2017/01/26 06:27:35, sadrul wrote:
> My reading of the TODO is that you want to notify the window manager that
> something changed, i.e. you should use the mojom::WindowManager api (much like
> what happens you a client sets a property on the window)

Done.

[commit-bot: I haz the power, 2017-01-26 17:50:24]

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

[commit-bot: I haz the power, 2017-01-26 17:50:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Hadi, 2017-01-26 18:15:43]

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

[commit-bot: I haz the power, 2017-01-26 18:16:07]

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

[sky, 2017-01-26 18:59:05]

https://codereview.chromium.org/2650833002/diff/100001/ash/mus/top_level_wind...
File ash/mus/top_level_window_factory.cc (right):

https://codereview.chromium.org/2650833002/diff/100001/ash/mus/top_level_wind...
ash/mus/top_level_window_factory.cc:187: // No need to persist this value.
Any properties left in properties are mirrored to the window-server. As
focusable is only useful at construction time you can remove it from properties
similar to what happens on this line.

https://codereview.chromium.org/2650833002/diff/100001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2650833002/diff/100001/ash/mus/window_manager...
ash/mus/window_manager.cc:325: window_tree_client()->SetCanFocus(window,
can_focus);
This value (and the initial one) should feed into
NonClientFrameController::CanActivate().

https://codereview.chromium.org/2650833002/diff/100001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2650833002/diff/100001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1549: return;
Please log errors. See AddWindow() for an example of the logging.

[commit-bot: I haz the power, 2017-01-26 19:12:15]

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

[commit-bot: I haz the power, 2017-01-26 19:12:16]

Dry run: This issue passed the CQ dry run.

[Hadi, 2017-01-26 20:23:17]

https://codereview.chromium.org/2650833002/diff/100001/ash/mus/top_level_wind...
File ash/mus/top_level_window_factory.cc (right):

https://codereview.chromium.org/2650833002/diff/100001/ash/mus/top_level_wind...
ash/mus/top_level_window_factory.cc:187: // No need to persist this value.
On 2017/01/26 18:59:04, sky wrote:
> Any properties left in properties are mirrored to the window-server. As
> focusable is only useful at construction time you can remove it from
properties
> similar to what happens on this line.

Done.

https://codereview.chromium.org/2650833002/diff/100001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2650833002/diff/100001/ash/mus/window_manager...
ash/mus/window_manager.cc:325: window_tree_client()->SetCanFocus(window,
can_focus);
On 2017/01/26 18:59:04, sky wrote:
> This value (and the initial one) should feed into
> NonClientFrameController::CanActivate().

I think Done. Can you please take a look to see if I did it correctly?

Thanks.

https://codereview.chromium.org/2650833002/diff/100001/services/ui/ws/window_...
File services/ui/ws/window_tree.cc (right):

https://codereview.chromium.org/2650833002/diff/100001/services/ui/ws/window_...
services/ui/ws/window_tree.cc:1549: return;
On 2017/01/26 18:59:04, sky wrote:
> Please log errors. See AddWindow() for an example of the logging.

Done.

[sky, 2017-01-26 20:40:30]

LGTM

[Hadi, 2017-01-26 20:43:31]

moshayedi@chromium.org changed reviewers:
+ tsepez@chromium.org

[Hadi, 2017-01-26 20:43:31]

tsepez@ can you please review window_manager.mojom?

Thanks.

[Hadi, 2017-01-26 20:43:54]

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

[commit-bot: I haz the power, 2017-01-26 20:44:26]

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

[Tom Sepez, 2017-01-26 21:09:15]

lgtm

[commit-bot: I haz the power, 2017-01-26 21:39:33]

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

[commit-bot: I haz the power, 2017-01-26 21:39:34]

Dry run: This issue passed the CQ dry run.

[Hadi, 2017-01-26 21:40:26]

The CQ bit was checked by moshayedi@chromium.org

[commit-bot: I haz the power, 2017-01-26 21:41:07]

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-26 21:50:44]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485466826580410,
"parent_rev": "a589a11cc8809c88d62b32a5810d6994fad45aaa", "commit_rev":
"c17142e8e1a64aa75c26b843c2ed23ebec9d39b9"}

[commit-bot: I haz the power, 2017-01-26 21:51:15]

Description was changed from

==========
Set focusibility correctly when initializing a window in mus+ash.

Saves |init_params.CanActivate()| in mus properties, and calls
SetCanFocus() from WM just after creating the window.

BUG=684048
==========

to

==========
Set focusibility correctly when initializing a window in mus+ash.

Saves |init_params.CanActivate()| in mus properties, and calls
SetCanFocus() from WM just after creating the window.

BUG=684048

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

[commit-bot: I haz the power, 2017-01-26 21:51:16]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/c17142e8e1a64aa75c26b843c2ed...