chromeos: fix focus for mushrome

chromeos: fix focus for mushrome

There were two issues preventing focus from working correctly:
. In ash there is a single FocusClient used. FocusSynchronizer didn't
  work well with this, and as a result never told mus about focus
  changes.
. Mushrome mode was never configuring activation parents in mus. This
  meant even once the previous item was fixed mus would ignore the
  focus request. The fix is to move configuration of activation
  parents to the right place.

BUG=none
TEST=covered by tests
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2843193002
Cr-Commit-Position: refs/heads/master@{#467581}
Committed: https://chromium.googlesource.com/chromium/src/+/68612719a16f976f78ab504a98295dc120d14852

[sky, 2017-04-26 21:43:27]

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

[commit-bot: I haz the power, 2017-04-26 21:44:03]

Dry run: CQ is trying da patch.

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

[msw, 2017-04-26 23:01:09]

Perhaps riajiang@ or perkj@ should review the FocusSynchronizer change?

https://codereview.chromium.org/2843193002/diff/20001/ash/aura/shell_port_cla...
File ash/aura/shell_port_classic.h (right):

https://codereview.chromium.org/2843193002/diff/20001/ash/aura/shell_port_cla...
ash/aura/shell_port_classic.h:88: void OnCreatedRootWindowContainers(
nit: belongs after CreateAshWindowTreeHost, ditto for definition

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
File ash/mus/bridge/shell_port_mash.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.cc:53: #include
"services/ui/public/interfaces/constants.mojom.h"
nit: remove if not needed.

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.cc:55: #include
"ui/aura/mus/focus_synchronizer.h"
nit: remove if not needed.

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
File ash/mus/bridge/shell_port_mash.h (right):

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.h:114: void OnCreatedRootWindowContainers(
nit: belongs after CreateAshWindowTreeHost, ditto for definition

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

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:24: #include "ash/public/cpp/shell_window_ids.h"
nit: remove if no longer needed

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
File ash/mus/window_manager_common_unittests.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:9: #include
"ui/aura/mus/window_tree_client.h"
nit: remove if not needed

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:18: using WindowManagerCommonTest =
test::AshTestBase;
What prompts the 'common' part of this name and the distinction from
ash/mus/window_manager_unittest.cc? Maybe describe WindowManagerCommonTest's
purpose (ie. for multiple wm impls?)

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:24: // Mus will deny focus requests
if there is no ancestors marked as
nit: "there are", maybe rephrase like "Ensure that activation parents have been
added, an ancestor of |window| must support activation for the focus attempt
below to succeed."?

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:30: EXPECT_NE(0u,
optional nit: ASSERT_LT

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

https://codereview.chromium.org/2843193002/diff/20001/ash/shell.cc#newcode157
ash/shell.cc:157: #include "ui/display/types/native_display_delegate.h"
nit: remove if not needed

https://codereview.chromium.org/2843193002/diff/20001/ash/shell.cc#newcode746
ash/shell.cc:746: if (GetAshConfig() != Config::CLASSIC &&
nit: use |config| (line 592)

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/focus_synch...
File ui/aura/mus/focus_synchronizer.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/focus_synch...
ui/aura/mus/focus_synchronizer.cc:45: if (!is_singleton_focus_client_ &&
active_focus_client_root_ != root)
If |is_singleton_focus_client_| is true, should this
[D]CHECK_EQ(active_focus_client_, client::GetFocusClient(root))?

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/focus_synch...
File ui/aura/mus/focus_synchronizer.h (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/focus_synch...
ui/aura/mus/focus_synchronizer.h:31: // and the mus server. FocusSynchronizer
may be configured in two distinct
Should riajiang@ or perkj@ review this or be CC'ed? (I've never heard of
focus_synchronizer before this CL)

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:738: // Tests may not set
|window_manager_client_|.
Should this say "Some tests may not set |window_manager_client_|."? Otherwise it
sounds like tests are not allowed to set |window_manager_client_|, which
contradict the header's comment.

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
File ui/aura/test/mus/test_window_manager_client.h (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
ui/aura/test/mus/test_window_manager_client.h:31: struct Change {
nit: remove struct if it won't be expanded imminently.

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
ui/aura/test/mus/test_window_manager_client.h:36: void
AddAccelerators(std::vector<ui::mojom::WmAcceleratorPtr> accelerators,
nit: match order with interface in window_manager.mojom

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
File ui/aura/test/mus/test_window_tree_client_setup.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
ui/aura/test/mus/test_window_tree_client_setup.cc:32: WindowTreeClientPrivate
wtc_private(window_tree_client_.get());
optional nit: avoid acronyms

[sky, 2017-04-26 23:29:52]

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

[sky, 2017-04-26 23:30:29]

https://codereview.chromium.org/2843193002/diff/20001/ash/aura/shell_port_cla...
File ash/aura/shell_port_classic.h (right):

https://codereview.chromium.org/2843193002/diff/20001/ash/aura/shell_port_cla...
ash/aura/shell_port_classic.h:88: void OnCreatedRootWindowContainers(
On 2017/04/26 23:01:07, msw wrote:
> nit: belongs after CreateAshWindowTreeHost, ditto for definition

Done.

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
File ash/mus/bridge/shell_port_mash.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.cc:53: #include
"services/ui/public/interfaces/constants.mojom.h"
On 2017/04/26 23:01:08, msw wrote:
> nit: remove if not needed.

Done.

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.cc:55: #include
"ui/aura/mus/focus_synchronizer.h"
On 2017/04/26 23:01:08, msw wrote:
> nit: remove if not needed.

Done.

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
File ash/mus/bridge/shell_port_mash.h (right):

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/bridge/shell_po...
ash/mus/bridge/shell_port_mash.h:114: void OnCreatedRootWindowContainers(
On 2017/04/26 23:01:08, msw wrote:
> nit: belongs after CreateAshWindowTreeHost, ditto for definition

Done.

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

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager....
ash/mus/window_manager.cc:24: #include "ash/public/cpp/shell_window_ids.h"
On 2017/04/26 23:01:08, msw wrote:
> nit: remove if no longer needed

Done.

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
File ash/mus/window_manager_common_unittests.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:9: #include
"ui/aura/mus/window_tree_client.h"
On 2017/04/26 23:01:08, msw wrote:
> nit: remove if not needed

Done.

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:18: using WindowManagerCommonTest =
test::AshTestBase;
On 2017/04/26 23:01:08, msw wrote:
> What prompts the 'common' part of this name and the distinction from
> ash/mus/window_manager_unittest.cc? Maybe describe WindowManagerCommonTest's
> purpose (ie. for multiple wm impls?)

Added comment.

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:24: // Mus will deny focus requests
if there is no ancestors marked as
On 2017/04/26 23:01:08, msw wrote:
> nit: "there are", maybe rephrase like "Ensure that activation parents have
been
> added, an ancestor of |window| must support activation for the focus attempt
> below to succeed."?

Done.

https://codereview.chromium.org/2843193002/diff/20001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:30: EXPECT_NE(0u,
On 2017/04/26 23:01:08, msw wrote:
> optional nit: ASSERT_LT

Done.

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

https://codereview.chromium.org/2843193002/diff/20001/ash/shell.cc#newcode157
ash/shell.cc:157: #include "ui/display/types/native_display_delegate.h"
On 2017/04/26 23:01:08, msw wrote:
> nit: remove if not needed

Done.

https://codereview.chromium.org/2843193002/diff/20001/ash/shell.cc#newcode746
ash/shell.cc:746: if (GetAshConfig() != Config::CLASSIC &&
On 2017/04/26 23:01:08, msw wrote:
> nit: use |config| (line 592)

Done.

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/focus_synch...
File ui/aura/mus/focus_synchronizer.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/focus_synch...
ui/aura/mus/focus_synchronizer.cc:45: if (!is_singleton_focus_client_ &&
active_focus_client_root_ != root)
On 2017/04/26 23:01:08, msw wrote:
> If |is_singleton_focus_client_| is true, should this
> [D]CHECK_EQ(active_focus_client_, client::GetFocusClient(root))?

Done.

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/focus_synch...
File ui/aura/mus/focus_synchronizer.h (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/focus_synch...
ui/aura/mus/focus_synchronizer.h:31: // and the mus server. FocusSynchronizer
may be configured in two distinct
On 2017/04/26 23:01:08, msw wrote:
> Should riajiang@ or perkj@ review this or be CC'ed? (I've never heard of
> focus_synchronizer before this CL)

I would have sent it to Ria, but she hasn't been around in a while. I would have
sent it to Sadrul, but he is OOO today too.. perkj@ was contributing a fix I
believe and doesn't work so much on this code.

If you would prefer I wait until Sadrul is back I can do that.

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.cc:738: // Tests may not set
|window_manager_client_|.
On 2017/04/26 23:01:08, msw wrote:
> Should this say "Some tests may not set |window_manager_client_|."? Otherwise
it
> sounds like tests are not allowed to set |window_manager_client_|, which
> contradict the header's comment.

I'm going to remove the comment given we don't have it everywhere and I'll
expand the comment in the header.

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
File ui/aura/test/mus/test_window_manager_client.h (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
ui/aura/test/mus/test_window_manager_client.h:31: struct Change {
On 2017/04/26 23:01:08, msw wrote:
> nit: remove struct if it won't be expanded imminently.

Done.

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
ui/aura/test/mus/test_window_manager_client.h:36: void
AddAccelerators(std::vector<ui::mojom::WmAcceleratorPtr> accelerators,
On 2017/04/26 23:01:09, msw wrote:
> nit: match order with interface in window_manager.mojom

Done.

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
File ui/aura/test/mus/test_window_tree_client_setup.cc (right):

https://codereview.chromium.org/2843193002/diff/20001/ui/aura/test/mus/test_w...
ui/aura/test/mus/test_window_tree_client_setup.cc:32: WindowTreeClientPrivate
wtc_private(window_tree_client_.get());
On 2017/04/26 23:01:09, msw wrote:
> optional nit: avoid acronyms

Ya, I thought the name was rather long and went with the acronym. But you're
right, we typically don't do that.

[commit-bot: I haz the power, 2017-04-26 23:31:27]

Dry run: CQ is trying da patch.

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

[msw, 2017-04-26 23:44:46]

lgtm with minor nits. I think I sufficiently understand the need for the
singleton config, sod feel free to land this sooner, but maybe encourage Sadrul
and/or Ria to take a look when they get back?

https://codereview.chromium.org/2843193002/diff/60001/ash/mus/window_manager_...
File ash/mus/window_manager_common_unittests.cc (right):

https://codereview.chromium.org/2843193002/diff/60001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:15: // window_manager_unittest.cc is
only run in Config::MASH.
aside/followup: maybe rename this to window_manager_unittests.cc and rename the
other file to be mash-specific? (I guess after shipping mash, both files should
be merged?)

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/focus_synch...
File ui/aura/mus/focus_synchronizer.cc (right):

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/focus_synch...
ui/aura/mus/focus_synchronizer.cc:46: DCHECK_EQ(active_focus_client_,
client::GetFocusClient(root));
q: this should work in release builds, right? (DCHECK comes out to an empty
statement in release, ie. ';', not a blank line without a statement, right?)

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.h (right):

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.h:585: // run without a Connector, which means
this (and
optional nit: service_manager::Connector

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.h:586: // |window_manager_internal_client_) are
null. Tests thata need to test
nit: '|' inside of ')', and s/thata/that/

[commit-bot: I haz the power, 2017-04-27 01:03:54]

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

[commit-bot: I haz the power, 2017-04-27 01:03:55]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sky, 2017-04-27 02:49:22]

The CQ bit was checked by sky@chromium.org

[sky, 2017-04-27 02:49:26]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2843193002/#ps80001
(title: "feedback")

[sky, 2017-04-27 02:49:40]

sky@chromium.org changed reviewers:
+ riajiang@chromium.org

[sky, 2017-04-27 02:49:41]

+ria to look at focus_synchronizer changes when you have a chance.

https://codereview.chromium.org/2843193002/diff/60001/ash/mus/window_manager_...
File ash/mus/window_manager_common_unittests.cc (right):

https://codereview.chromium.org/2843193002/diff/60001/ash/mus/window_manager_...
ash/mus/window_manager_common_unittests.cc:15: // window_manager_unittest.cc is
only run in Config::MASH.
On 2017/04/26 23:44:46, msw wrote:
> aside/followup: maybe rename this to window_manager_unittests.cc and rename
the
> other file to be mash-specific? (I guess after shipping mash, both files
should
> be merged?)

That is the plan. I'm going to leave this named common for now.

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/focus_synch...
File ui/aura/mus/focus_synchronizer.cc (right):

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/focus_synch...
ui/aura/mus/focus_synchronizer.cc:46: DCHECK_EQ(active_focus_client_,
client::GetFocusClient(root));
On 2017/04/26 23:44:46, msw wrote:
> q: this should work in release builds, right? (DCHECK comes out to an empty
> statement in release, ie. ';', not a blank line without a statement, right?)

If it were a blank line this wouldn't compile at all as the code would
effectively become if (...) no-statement else.

As to what DCHECK generates in release builds, it's rather subtle. See
https://chromium.googlesource.com/chromium/src/+/master/base/logging.h#823 .

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/window_tree...
File ui/aura/mus/window_tree_client.h (right):

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.h:585: // run without a Connector, which means
this (and
On 2017/04/26 23:44:46, msw wrote:
> optional nit: service_manager::Connector

Done.

https://codereview.chromium.org/2843193002/diff/60001/ui/aura/mus/window_tree...
ui/aura/mus/window_tree_client.h:586: // |window_manager_internal_client_) are
null. Tests thata need to test
On 2017/04/26 23:44:46, msw wrote:
> nit: '|' inside of ')', and s/thata/that/

Done.

[commit-bot: I haz the power, 2017-04-27 02:50:14]

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-04-27 04:45:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493261362858100,
"parent_rev": "c23fcf744de8b51e5936649cfe985cfa9c058398", "commit_rev":
"68612719a16f976f78ab504a98295dc120d14852"}

[commit-bot: I haz the power, 2017-04-27 04:45:15]

Description was changed from

==========
chromeos: fix focus for mushrome

There were two issues preventing focus from working correctly:
. In ash there is a single FocusClient used. FocusSynchronizer didn't
  work well with this, and as a result never told mus about focus
  changes.
. Mushrome mode was never configuring activation parents in mus. This
  meant even once the previous item was fixed mus would ignore the
  focus request. The fix is to move configuration of activation
  parents to the right place.

BUG=none
TEST=covered by tests
R=msw@chromium.org
==========

to

==========
chromeos: fix focus for mushrome

There were two issues preventing focus from working correctly:
. In ash there is a single FocusClient used. FocusSynchronizer didn't
  work well with this, and as a result never told mus about focus
  changes.
. Mushrome mode was never configuring activation parents in mus. This
  meant even once the previous item was fixed mus would ignore the
  focus request. The fix is to move configuration of activation
  parents to the right place.

BUG=none
TEST=covered by tests
R=msw@chromium.org

Review-Url: https://codereview.chromium.org/2843193002
Cr-Commit-Position: refs/heads/master@{#467581}
Committed:
https://chromium.googlesource.com/chromium/src/+/68612719a16f976f78ab504a9829...
==========

[commit-bot: I haz the power, 2017-04-27 04:45:17]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/68612719a16f976f78ab504a9829...

[riajiang, 2017-04-28 15:14:15]

LGTM