Moves maintaining root_window_for_new_windows_ to Shell

Moves maintaining root_window_for_new_windows_ to Shell

There is no reason for this to be on WmShell anymore. This also
includes the following:
. Renames Shell::GetTargetRootWindow() to
  GetRootWindowForNewWindows(). This name better indicates what the
  function is for.
. Added version of the above that returns WmWindow. This will be
  removed once WmWindow is removed.
. Removes Shell::GetTargetDisplayId() as it was only used in one place
  (and it's name is rather confusing).
. Makes FocusController's implementation of ActivationClient and
  FocusClient public. FocusController's is intended to be the
  ActivationClient and the FocusClient, so making it protected just
  leads to awkwardness in using the class.

BUG=699231
TEST=covered by tests

Review-Url: https://codereview.chromium.org/2739553005
Cr-Commit-Position: refs/heads/master@{#455546}
Committed: https://chromium.googlesource.com/chromium/src/+/2734438cde42ce98227c4292b35a092fb75ee371

[sky, 2017-03-08 01:11:20]

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

[commit-bot: I haz the power, 2017-03-08 01:12:27]

Dry run: 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-03-08 02:46:01]

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

[commit-bot: I haz the power, 2017-03-08 02:46:02]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[James Cook, 2017-03-08 03:04:16]

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

https://codereview.chromium.org/2739553005/diff/20001/ash/shell.cc#newcode247
ash/shell.cc:247: aura::Window* Shell::GetTargetRootWindow() {
Hrm, though now it's confusing that we have both. Do you think these two methods
will eventually collapse?

https://codereview.chromium.org/2739553005/diff/20001/ash/shell.cc#newcode690
ash/shell.cc:690: activation_client_->AddObserver(this);
Where is this observer removed?

https://codereview.chromium.org/2739553005/diff/20001/ash/shell.cc#newcode971
ash/shell.cc:971: // TODO(sky): Shell should implement ActivationChangeObserver,
not WmShell.
This TODO looks TODONE.

https://codereview.chromium.org/2739553005/diff/20001/ash/shell.h
File ash/shell.h (right):

https://codereview.chromium.org/2739553005/diff/20001/ash/shell.h#newcode403
ash/shell.h:403: WmWindow* GetRootWindowForNewWindows();
Aside: I'm glad you choose this name for the WmWindow implementation. It's much
better than the old "target root window" name.

[sky, 2017-03-08 16:56:17]

Description was changed from

==========
Moves maintaining root_window_for_new_windows_ to Shell

There is no reason for this to be on WmShell anymore.

BUG=699231
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Moves maintaining root_window_for_new_windows_ to Shell

There is no reason for this to be on WmShell anymore. This also
includes the following:
. Renames Shell::GetTargetRootWindow() to
  GetRootWindowForNewWindows(). This name better indicates what the
  function is for.
. Added version of the above that returns WmWindow. This will be
  removed once WmWindow is removed.
. Removes Shell::GetTargetDisplayId() as it was only used in one place
  (and it's name is rather confusing).
. Makes FocusController's implementation of ActivationClient and
  FocusClient public. FocusController's is intended to be the
  ActivationClient and the FocusClient, so making it protected just
  leads to awkwardness in using the class.

BUG=699231
TEST=covered by tests
==========

[sky, 2017-03-08 17:01:38]

As part of making Shell remove itself from the FocusController I made
FocusController's implementation of ActivationClient and FocusClient public. The
whole reason FocusController exists is so that it implements
Activation/FocusClient. To make the implementations protected leads to casting
and using two separate variables, which is awkward.

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

https://codereview.chromium.org/2739553005/diff/20001/ash/shell.cc#newcode247
ash/shell.cc:247: aura::Window* Shell::GetTargetRootWindow() {
On 2017/03/08 03:04:15, James Cook wrote:
> Hrm, though now it's confusing that we have both. Do you think these two
methods
> will eventually collapse?

Indeed. I renamed this to GetRootWindowForNewWindows and added
GetWmRootWindowForNewWindows. The later is temporary while WmWindow exists.

https://codereview.chromium.org/2739553005/diff/20001/ash/shell.cc#newcode690
ash/shell.cc:690: activation_client_->AddObserver(this);
On 2017/03/08 03:04:15, James Cook wrote:
> Where is this observer removed?

Looking back at the history AFAICT the code never removed itself. I've added the
removal.

https://codereview.chromium.org/2739553005/diff/20001/ash/shell.cc#newcode971
ash/shell.cc:971: // TODO(sky): Shell should implement ActivationChangeObserver,
not WmShell.
On 2017/03/08 03:04:15, James Cook wrote:
> This TODO looks TODONE.

Done.

[sky, 2017-03-08 17:02:02]

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

[sky, 2017-03-08 17:02:20]

+reveman for exo

[reveman, 2017-03-08 17:09:57]

lgtm

[sky, 2017-03-08 17:27:26]

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

[commit-bot: I haz the power, 2017-03-08 17:28:06]

Dry run: 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-03-08 18:45:51]

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

[commit-bot: I haz the power, 2017-03-08 18:45:52]

Dry run: This issue passed the CQ dry run.

[James Cook, 2017-03-08 20:19:17]

LGTM! The new names are much clearer, it's nice to get rid of the two
focus/activation variables, and getting rid of GetTargetDisplayId() makes Shell
just a touch simpler. Nice.

https://codereview.chromium.org/2739553005/diff/60001/ash/wm/drag_window_resi...
File ash/wm/drag_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2739553005/diff/60001/ash/wm/drag_window_resi...
ash/wm/drag_window_resizer_unittest.cc:312: EXPECT_EQ(root_windows[0],
ash::Shell::GetRootWindowForNewWindows());
nit: I don't think ash:: is needed, here or below

[sky, 2017-03-08 20:28:22]

The CQ bit was checked by sky@chromium.org

[sky, 2017-03-08 20:28:22]

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

[commit-bot: I haz the power, 2017-03-08 20:28:51]

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

[sky, 2017-03-08 20:42:59]

https://codereview.chromium.org/2739553005/diff/60001/ash/wm/drag_window_resi...
File ash/wm/drag_window_resizer_unittest.cc (right):

https://codereview.chromium.org/2739553005/diff/60001/ash/wm/drag_window_resi...
ash/wm/drag_window_resizer_unittest.cc:312: EXPECT_EQ(root_windows[0],
ash::Shell::GetRootWindowForNewWindows());
On 2017/03/08 20:19:17, James Cook wrote:
> nit: I don't think ash:: is needed, here or below

Done.

[commit-bot: I haz the power, 2017-03-08 21:30:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489004902267710,
"parent_rev": "20fc04fae6337887e6b82d753cfac07325ed9820", "commit_rev":
"2734438cde42ce98227c4292b35a092fb75ee371"}

[commit-bot: I haz the power, 2017-03-08 21:31:20]

Description was changed from

==========
Moves maintaining root_window_for_new_windows_ to Shell

There is no reason for this to be on WmShell anymore. This also
includes the following:
. Renames Shell::GetTargetRootWindow() to
  GetRootWindowForNewWindows(). This name better indicates what the
  function is for.
. Added version of the above that returns WmWindow. This will be
  removed once WmWindow is removed.
. Removes Shell::GetTargetDisplayId() as it was only used in one place
  (and it's name is rather confusing).
. Makes FocusController's implementation of ActivationClient and
  FocusClient public. FocusController's is intended to be the
  ActivationClient and the FocusClient, so making it protected just
  leads to awkwardness in using the class.

BUG=699231
TEST=covered by tests
==========

to

==========
Moves maintaining root_window_for_new_windows_ to Shell

There is no reason for this to be on WmShell anymore. This also
includes the following:
. Renames Shell::GetTargetRootWindow() to
  GetRootWindowForNewWindows(). This name better indicates what the
  function is for.
. Added version of the above that returns WmWindow. This will be
  removed once WmWindow is removed.
. Removes Shell::GetTargetDisplayId() as it was only used in one place
  (and it's name is rather confusing).
. Makes FocusController's implementation of ActivationClient and
  FocusClient public. FocusController's is intended to be the
  ActivationClient and the FocusClient, so making it protected just
  leads to awkwardness in using the class.

BUG=699231
TEST=covered by tests

Review-Url: https://codereview.chromium.org/2739553005
Cr-Commit-Position: refs/heads/master@{#455546}
Committed:
https://chromium.googlesource.com/chromium/src/+/2734438cde42ce98227c4292b35a...
==========

[commit-bot: I haz the power, 2017-03-08 21:31:21]

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