mash: Migrate ScopedTargetRootWindow to //ash/common

mash: Migrate ScopedTargetRootWindow to //ash/common

It's a dependency of ShelfView, which we're trying to move.

* Rename to ScopedRootWindowForNewWindows, the more-descriptive name we're
using in mus code.
* Move ownership of root_window_for_new_windows_ to WmShell.
* ash::Shell no longer needs to be an activation observer, since it was only
using that to set the target root window, and we can do that in WmShellAura
now.
* Fix some tests that were unnecessarily setting the target root window to
the primary display, which is the default.

BUG=633323
TEST=ash_unittests, unit_tests, create new browser windows by clicking on the
browser shortcut shelf item on different displays

Committed: https://crrev.com/4545062c75118e1a633987878b524277f33c9bbc
Cr-Commit-Position: refs/heads/master@{#409256}

[James Cook, 2016-08-01 21:20:53]

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

[James Cook, 2016-08-01 21:21:17]

jamescook@chromium.org changed reviewers:
+ msw@chromium.org

[James Cook, 2016-08-01 21:21:19]

msw, please take a look.

[commit-bot: I haz the power, 2016-08-01 21:21:39]

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

[msw, 2016-08-01 21:36:15]

https://codereview.chromium.org/2194353002/diff/40001/ash/aura/scoped_root_wi...
File ash/aura/scoped_root_window_for_new_windows_aura.cc (right):

https://codereview.chromium.org/2194353002/diff/40001/ash/aura/scoped_root_wi...
ash/aura/scoped_root_window_for_new_windows_aura.cc:13:
Shell::GetInstance()->scoped_target_root_window_ = root_window;
q: [D]CHECK that scoped_target_root_window_ is null?

https://codereview.chromium.org/2194353002/diff/40001/ash/aura/scoped_root_wi...
ash/aura/scoped_root_window_for_new_windows_aura.cc:17:
Shell::GetInstance()->scoped_target_root_window_ = nullptr;
q: [D]CHECK that the existing value was set by this instance?

https://codereview.chromium.org/2194353002/diff/40001/ash/aura/scoped_root_wi...
File ash/aura/scoped_root_window_for_new_windows_aura.h (right):

https://codereview.chromium.org/2194353002/diff/40001/ash/aura/scoped_root_wi...
ash/aura/scoped_root_window_for_new_windows_aura.h:23: class ASH_EXPORT
ScopedRootWindowForNewWindowsAura
q: Could we cache the WmWindow and make Shell::GetTargetRootWindow convert to an
aura window, instead of creating and vending aura/mus scoped* subclasses?

https://codereview.chromium.org/2194353002/diff/40001/ash/common/scoped_root_...
File ash/common/scoped_root_window_for_new_windows.h (right):

https://codereview.chromium.org/2194353002/diff/40001/ash/common/scoped_root_...
ash/common/scoped_root_window_for_new_windows.h:15: class ASH_EXPORT
ScopedRootWindowForNewWindows {
q: Could this just be a typedef for base::AutoReset<WmWindow>?

[commit-bot: I haz the power, 2016-08-01 22:19:19]

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

[commit-bot: I haz the power, 2016-08-01 22:19:20]

Dry run: This issue passed the CQ dry run.

[James Cook, 2016-08-02 00:22:03]

Description was changed from

==========
mash: Migrate ScopedTargetRootWindow to //ash/common

It's a dependency of ShelfView, which we're trying to move.

* Rename to ScopedRootWindowForNewWindows, the more-descriptive name we're
using in mus code.
* Convert existing aura implementation to ScopedRootWindowForNewWindowsAura
* Don't create one yet for mus, since mus only supports a single display.
* Delegate out creation via WmShell, similar to how we create
ScopedDisableInternalMouseAndKeyboard

BUG=633323
TEST=ash_unittests, unit_tests, create new browser windows by clicking on the
browser shortcut shelf item on different displays
==========

to

==========
mash: Migrate ScopedTargetRootWindow to //ash/common

It's a dependency of ShelfView, which we're trying to move.

* Rename to ScopedRootWindowForNewWindows, the more-descriptive name we're
using in mus code.
* Move ownership of root_window_for_new_windows_ to WmShell.
* ash::Shell no longer needs to be an activation observer, since it was only
using that to set the target root window, and we can do that in WmShellAura
now.
* Fix some tests that were unnecessarily setting the target root window to
the primary display, which is the default.

BUG=633323
TEST=ash_unittests, unit_tests, create new browser windows by clicking on the
browser shortcut shelf item on different displays
==========

[James Cook, 2016-08-02 00:26:54]

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

[commit-bot: I haz the power, 2016-08-02 00:27:08]

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

[James Cook, 2016-08-02 00:27:50]

msw, please take another look.

https://codereview.chromium.org/2194353002/diff/80001/ash/common/scoped_root_...
File ash/common/scoped_root_window_for_new_windows.h (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/common/scoped_root_...
ash/common/scoped_root_window_for_new_windows.h:26: WmWindow* old_root_;
I did this initially with base::AutoReset<> but the constructor looked a little
awkward to me, so I did it by hand.

[commit-bot: I haz the power, 2016-08-02 01:01:58]

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

[commit-bot: I haz the power, 2016-08-02 01:01:59]

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

[msw, 2016-08-02 01:04:51]

https://codereview.chromium.org/2194353002/diff/80001/ash/common/scoped_root_...
File ash/common/scoped_root_window_for_new_windows.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/common/scoped_root_...
ash/common/scoped_root_window_for_new_windows.cc:19:
WmShell::Get()->set_root_window_for_new_windows(old_root_);
I'm a little worried that this will break the value when multiple instances have
overlapping lifetimes, or when something calls set_root_window_for_new_windows()
directly during a ScopedRootWindowForNewWindows lifetime. Should WmShell use the
existing approach in ash::Shell? (keeping a separate member for scoped values,
setting that to null on scoped instance destruction, etc.) Otherwise, should
this [D]CHECK that the current value matches |new_root|?

https://codereview.chromium.org/2194353002/diff/80001/ash/common/scoped_root_...
File ash/common/scoped_root_window_for_new_windows.h (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/common/scoped_root_...
ash/common/scoped_root_window_for_new_windows.h:26: WmWindow* old_root_;
On 2016/08/02 00:27:50, James Cook wrote:
> I did this initially with base::AutoReset<> but the constructor looked a
little
> awkward to me, so I did it by hand.

Acknowledged.

https://codereview.chromium.org/2194353002/diff/80001/ash/drag_drop/drag_drop...
File ash/drag_drop/drag_drop_tracker_unittest.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/drag_drop/drag_drop...
ash/drag_drop/drag_drop_tracker_unittest.cc:73: ScopedRootWindowForNewWindows
root_for_new_windows(
optional nit: ScopedRootWindowForNewWindows for first root not needed?

https://codereview.chromium.org/2194353002/diff/80001/ash/drag_drop/drag_drop...
ash/drag_drop/drag_drop_tracker_unittest.cc:138: ScopedRootWindowForNewWindows
root_for_new_windows(
optional nit: ScopedRootWindowForNewWindows for first root not needed?

https://codereview.chromium.org/2194353002/diff/80001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:361:
static_cast<WmWindow*>(gained_active)->GetRootWindow());
q: why is this cast needed? WmWindowMus::GetRootWindow should work.

https://codereview.chromium.org/2194353002/diff/80001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1704: WmWindow* root_window =
optional nit: use a GetRootWindow pattern consistent with the one below:
  WmWindow* window = WmLookup::Get()->GetWindowForWidget(sender->GetWidget());
  scoped_root_window_for_new_windows_.reset(
      new ScopedRootWindowForNewWindows(window->GetRootWindow()));

https://codereview.chromium.org/2194353002/diff/80001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/shell.cc#newcode834
ash/shell.cc:834:
wm_shell_->set_root_window_for_new_windows(WmWindowAura::Get(root_window));
optional nit: if (!in_mus())? (it may be okay to do this regardless)

https://codereview.chromium.org/2194353002/diff/80001/chrome/browser/ui/windo...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/chrome/browser/ui/windo...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:824: ash::WmWindow*
first_root = ash::WmShell::Get()->GetAllRootWindows()[0];
optional nit: ScopedRootWindowForNewWindows for first root not needed?

[James Cook, 2016-08-02 16:15:38]

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

[commit-bot: I haz the power, 2016-08-02 16:16:01]

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

[James Cook, 2016-08-02 16:16:05]

msw, please take another look.

https://codereview.chromium.org/2194353002/diff/80001/ash/common/scoped_root_...
File ash/common/scoped_root_window_for_new_windows.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/common/scoped_root_...
ash/common/scoped_root_window_for_new_windows.cc:19:
WmShell::Get()->set_root_window_for_new_windows(old_root_);
On 2016/08/02 01:04:50, msw wrote:
> I'm a little worried that this will break the value when multiple instances
have
> overlapping lifetimes, or when something calls
set_root_window_for_new_windows()
> directly during a ScopedRootWindowForNewWindows lifetime. Should WmShell use
the
> existing approach in ash::Shell? (keeping a separate member for scoped values,
> setting that to null on scoped instance destruction, etc.) Otherwise, should
> this [D]CHECK that the current value matches |new_root|?

Good call. This was the source of my interactive_ui_tests failure on the bots.
Switched to the method used by ash::Shell.

https://codereview.chromium.org/2194353002/diff/80001/ash/drag_drop/drag_drop...
File ash/drag_drop/drag_drop_tracker_unittest.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/drag_drop/drag_drop...
ash/drag_drop/drag_drop_tracker_unittest.cc:73: ScopedRootWindowForNewWindows
root_for_new_windows(
On 2016/08/02 01:04:50, msw wrote:
> optional nit: ScopedRootWindowForNewWindows for first root not needed?

Done.

https://codereview.chromium.org/2194353002/diff/80001/ash/mus/bridge/wm_shell...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/mus/bridge/wm_shell...
ash/mus/bridge/wm_shell_mus.cc:361:
static_cast<WmWindow*>(gained_active)->GetRootWindow());
On 2016/08/02 01:04:50, msw wrote:
> q: why is this cast needed? WmWindowMus::GetRootWindow should work.

WmWindowMus::GetRootWindow calls the const version. Changed the pointers to
WmWindow*.

https://codereview.chromium.org/2194353002/diff/80001/ash/shelf/shelf_view.cc
File ash/shelf/shelf_view.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/shelf/shelf_view.cc...
ash/shelf/shelf_view.cc:1704: WmWindow* root_window =
On 2016/08/02 01:04:50, msw wrote:
> optional nit: use a GetRootWindow pattern consistent with the one below:
>   WmWindow* window = WmLookup::Get()->GetWindowForWidget(sender->GetWidget());
>   scoped_root_window_for_new_windows_.reset(
>       new ScopedRootWindowForNewWindows(window->GetRootWindow()));

Done.

https://codereview.chromium.org/2194353002/diff/80001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/ash/shell.cc#newcode834
ash/shell.cc:834:
wm_shell_->set_root_window_for_new_windows(WmWindowAura::Get(root_window));
On 2016/08/02 01:04:50, msw wrote:
> optional nit: if (!in_mus())? (it may be okay to do this regardless)

I think it's OK to do this regardless.

https://codereview.chromium.org/2194353002/diff/80001/chrome/browser/ui/windo...
File chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc (right):

https://codereview.chromium.org/2194353002/diff/80001/chrome/browser/ui/windo...
chrome/browser/ui/window_sizer/window_sizer_ash_unittest.cc:824: ash::WmWindow*
first_root = ash::WmShell::Get()->GetAllRootWindows()[0];
On 2016/08/02 01:04:51, msw wrote:
> optional nit: ScopedRootWindowForNewWindows for first root not needed?

Done.

[commit-bot: I haz the power, 2016-08-02 17:08:39]

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

[commit-bot: I haz the power, 2016-08-02 17:08:41]

Dry run: This issue passed the CQ dry run.

[msw, 2016-08-02 17:22:19]

lgtm with a nit

https://codereview.chromium.org/2194353002/diff/100001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2194353002/diff/100001/ash/common/wm_shell.h#...
ash/common/wm_shell.h:157: virtual WmWindow* GetRootWindowForNewWindows();
nit: virtual not needed (reorder decl&def if you make it non-virtual)

[James Cook, 2016-08-02 17:48:08]

https://codereview.chromium.org/2194353002/diff/100001/ash/common/wm_shell.h
File ash/common/wm_shell.h (right):

https://codereview.chromium.org/2194353002/diff/100001/ash/common/wm_shell.h#...
ash/common/wm_shell.h:157: virtual WmWindow* GetRootWindowForNewWindows();
On 2016/08/02 17:22:19, msw wrote:
> nit: virtual not needed (reorder decl&def if you make it non-virtual)

Oops. Done.

[James Cook, 2016-08-02 17:48:13]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2016-08-02 17:48:14]

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/2194353002/#ps140001
(title: "non-virtual")

[commit-bot: I haz the power, 2016-08-02 17:48:59]

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, 2016-08-02 18:48:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-02 18:51:42]

Description was changed from

==========
mash: Migrate ScopedTargetRootWindow to //ash/common

It's a dependency of ShelfView, which we're trying to move.

* Rename to ScopedRootWindowForNewWindows, the more-descriptive name we're
using in mus code.
* Move ownership of root_window_for_new_windows_ to WmShell.
* ash::Shell no longer needs to be an activation observer, since it was only
using that to set the target root window, and we can do that in WmShellAura
now.
* Fix some tests that were unnecessarily setting the target root window to
the primary display, which is the default.

BUG=633323
TEST=ash_unittests, unit_tests, create new browser windows by clicking on the
browser shortcut shelf item on different displays
==========

to

==========
mash: Migrate ScopedTargetRootWindow to //ash/common

It's a dependency of ShelfView, which we're trying to move.

* Rename to ScopedRootWindowForNewWindows, the more-descriptive name we're
using in mus code.
* Move ownership of root_window_for_new_windows_ to WmShell.
* ash::Shell no longer needs to be an activation observer, since it was only
using that to set the target root window, and we can do that in WmShellAura
now.
* Fix some tests that were unnecessarily setting the target root window to
the primary display, which is the default.

BUG=633323
TEST=ash_unittests, unit_tests, create new browser windows by clicking on the
browser shortcut shelf item on different displays

Committed: https://crrev.com/4545062c75118e1a633987878b524277f33c9bbc
Cr-Commit-Position: refs/heads/master@{#409256}
==========

[commit-bot: I haz the power, 2016-08-02 18:51:43]

Patchset 8 (id:??) landed as
https://crrev.com/4545062c75118e1a633987878b524277f33c9bbc
Cr-Commit-Position: refs/heads/master@{#409256}