Converts most of WorkspaceLayoutManager tests to use common code

Converts most of WorkspaceLayoutManager tests to use common code

A couple triggers crashes, which I've filed separately. The keyboard
ones won't work in mash. I plan to move those out of this file
separately.

BUG=624172, 652871
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/8335eeddd6a5bc06515eb7c577a9196112f4583c
Cr-Commit-Position: refs/heads/master@{#423732}

[sky, 2016-10-04 22:38:23]

My plan after this patch is to move everything except the keyboard tests to the
common directory, at which point I'll prune the includes. The keyboard tests
have to stay as the keyboard isn't working in mash yet.

[sky, 2016-10-04 22:38:25]

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

[commit-bot: I haz the power, 2016-10-04 22:38:54]

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, 2016-10-04 22:43:38]

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

[commit-bot: I haz the power, 2016-10-04 22:43:39]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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_compile_dbg_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_...)

[James Cook, 2016-10-04 23:16:05]

Nice to see more things being converted.

https://codereview.chromium.org/2391153002/diff/20001/ash/aura/wm_window_aura.cc
File ash/aura/wm_window_aura.cc (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/aura/wm_window_aura...
ash/aura/wm_window_aura.cc:198: bool WmWindowAura::GetLayerTargetVisibility() {
match header file order

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_test.h
File ash/common/test/ash_test.h (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:60: static bool IsMash();
We have WmShell::Get()->IsRunningInMash() now. How about renaming this function
IsRunningInMash() for consistency with the //chrome and //ash codebases, and
implementing it directly in ash_test.cc?

I find it confusing when static functions are implemented in multiple different
.cc files.

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:77: // Creates a visible top-level window. For aura a
top-level window is a Window
From the description it's a little hard to tell how this is different than the
above function, which also creates a top level visible window. Is there some
other way to explain the difference?

Could the two functions be collapsed together?

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:108: WmShelf* GetPrimaryShelf();
static and (optionally) move up

I think there are existing tests that assume they can call
AshTestBase::GetPrimaryShelf() in static helper methods.

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:116: void SetCanLockScreen(bool can_lock_screen);
Function comment? "Lock" means signout-related-lock, I presume.

https://codereview.chromium.org/2391153002/diff/20001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/common/wm_window.h#...
ash/common/wm_window.h:244: virtual void SetPinned(bool trusted) = 0;
Function docs? (I actually don't know what pinned means.)

https://codereview.chromium.org/2391153002/diff/20001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:249: bool
WmWindowMus::GetLayerTargetVisibility() {
match header order (after header matches parent interface order)

https://codereview.chromium.org/2391153002/diff/20001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.h (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.h:128: bool GetLayerTargetVisibility() override;
match parent interface order

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:328: //
aura::WindowObserver:
WmWindowObserver

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:338: gfx::Rect
shelf_bounds(ash_test_->GetPrimaryShelf()->GetIdealBounds());
GetPrimaryShelf() being static would help here

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:509:
std::unique_ptr<WindowOwner> window2_owner(
How about consistently using either |window2_owner| (like here) or
|window_owner2| (like above)?

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:576: class FocusDelegate :
public WmWindowObserver {
Rename to FocusObserver (or something other than Delegate, since it's not a
delegate).

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:925: for (int i =
static_cast<int>(children.size()) - 1; i >= 0; --i) {
nice. easier to read both the loop and the code below

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1333: }
Wow, this file looks like it was a lot of work to convert. It'll be a great
example for future conversions.

[sky, 2016-10-06 17:56:43]

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

[sky, 2016-10-06 17:57:06]

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

[sky, 2016-10-06 17:57:07]

jamescook->msw as James is OOO until Monday

https://codereview.chromium.org/2391153002/diff/20001/ash/aura/wm_window_aura.cc
File ash/aura/wm_window_aura.cc (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/aura/wm_window_aura...
ash/aura/wm_window_aura.cc:198: bool WmWindowAura::GetLayerTargetVisibility() {
On 2016/10/04 23:16:04, James Cook (OOO until Oct 10) wrote:
> match header file order

Done.

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_test.h
File ash/common/test/ash_test.h (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:60: static bool IsMash();
On 2016/10/04 23:16:04, James Cook (OOO until Oct 10) wrote:
> We have WmShell::Get()->IsRunningInMash() now. How about renaming this
function
> IsRunningInMash() for consistency with the //chrome and //ash codebases, and
> implementing it directly in ash_test.cc?

I forgot about that one. Done.

> I find it confusing when static functions are implemented in multiple
different
> .cc files.

Agreed, but it's a common pattern for some things.

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:77: // Creates a visible top-level window. For aura a
top-level window is a Window
On 2016/10/04 23:16:04, James Cook (OOO until Oct 10) wrote:
> From the description it's a little hard to tell how this is different than the
> above function, which also creates a top level visible window. Is there some
> other way to explain the difference?

I added a reference to aura::Window::GetToplevelWindow(). But I agree it's a bit
strange. AshTestBase uses a non-null delegate supplied to
CreateTestWindowInShellWithDelegate() to differentiate these. IMO that name is
not any clearer.

> 
> Could the two functions be collapsed together?

Indeed it could, but I think it would result in more optional args and most
likely a type to CreateTestWindow. I think it's arguably clearer and less code
with this approach.

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:108: WmShelf* GetPrimaryShelf();
On 2016/10/04 23:16:04, James Cook (OOO until Oct 10) wrote:
> static and (optionally) move up
> 
> I think there are existing tests that assume they can call
> AshTestBase::GetPrimaryShelf() in static helper methods.

Done.

https://codereview.chromium.org/2391153002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:116: void SetCanLockScreen(bool can_lock_screen);
On 2016/10/04 23:16:04, James Cook (OOO until Oct 10) wrote:
> Function comment? "Lock" means signout-related-lock, I presume.

Done.

https://codereview.chromium.org/2391153002/diff/20001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/common/wm_window.h#...
ash/common/wm_window.h:244: virtual void SetPinned(bool trusted) = 0;
On 2016/10/04 23:16:04, James Cook (OOO until Oct 10) wrote:
> Function docs? (I actually don't know what pinned means.)

Done.

https://codereview.chromium.org/2391153002/diff/20001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.cc (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.cc:249: bool
WmWindowMus::GetLayerTargetVisibility() {
On 2016/10/04 23:16:04, James Cook (OOO until Oct 10) wrote:
> match header order (after header matches parent interface order)

Done.

https://codereview.chromium.org/2391153002/diff/20001/ash/mus/bridge/wm_windo...
File ash/mus/bridge/wm_window_mus.h (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/mus/bridge/wm_windo...
ash/mus/bridge/wm_window_mus.h:128: bool GetLayerTargetVisibility() override;
On 2016/10/04 23:16:05, James Cook (OOO until Oct 10) wrote:
> match parent interface order

Done.

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:328: //
aura::WindowObserver:
On 2016/10/04 23:16:05, James Cook (OOO until Oct 10) wrote:
> WmWindowObserver

Done.

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:338: gfx::Rect
shelf_bounds(ash_test_->GetPrimaryShelf()->GetIdealBounds());
On 2016/10/04 23:16:05, James Cook (OOO until Oct 10) wrote:
> GetPrimaryShelf() being static would help here

Done.

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:509:
std::unique_ptr<WindowOwner> window2_owner(
On 2016/10/04 23:16:05, James Cook (OOO until Oct 10) wrote:
> How about consistently using either |window2_owner| (like here) or
> |window_owner2| (like above)?

Done.

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:576: class FocusDelegate :
public WmWindowObserver {
On 2016/10/04 23:16:05, James Cook (OOO until Oct 10) wrote:
> Rename to FocusObserver (or something other than Delegate, since it's not a
> delegate).

FocusOwner seemed like a confusing name to me, so I left it. But I agree
FocusDelegate isn't a good name. Renamed to the verbose to the more verbose but
specific FocusDuringUnminimizeWindowObserver.

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:925: for (int i =
static_cast<int>(children.size()) - 1; i >= 0; --i) {
On 2016/10/04 23:16:05, James Cook (OOO until Oct 10) wrote:
> nice. easier to read both the loop and the code below

Acknowledged.

https://codereview.chromium.org/2391153002/diff/20001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1333: }
On 2016/10/04 23:16:05, James Cook (OOO until Oct 10) wrote:
> Wow, this file looks like it was a lot of work to convert. It'll be a great
> example for future conversions.

Tedious, but mostly mechanical changes. Hopefully future ones go smoother as
AshTest gets more functionality.

[commit-bot: I haz the power, 2016-10-06 17:57:25]

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

[sky, 2016-10-06 17:59:16]

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

[commit-bot: I haz the power, 2016-10-06 17:59:40]

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, 2016-10-06 19:05:03]

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

[commit-bot: I haz the power, 2016-10-06 19:05:04]

Dry run: This issue passed the CQ dry run.

[msw, 2016-10-06 20:18:24]

Wow, it takes a lot of changes to migrate this test file!

https://codereview.chromium.org/2391153002/diff/60001/ash/common/test/ash_test.h
File ash/common/test/ash_test.h (right):

https://codereview.chromium.org/2391153002/diff/60001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:59: // Cover functions for
WmShell::Get()->IsRunningInMash().
nit: "Cover functions"? maybe just "Calls WmShell::IsRunningInMash()."?
(it's also short enough to just inline everywhere [+wm_shell.h include])

https://codereview.chromium.org/2391153002/diff/60001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:71: // Creates a top level visible window in the
appropriate container. If
I see that James already asked about this, but I need to ask. Does this not
actually/always create a top-level window? Otherwise, what's the practical
difference for users between this and CreateToplevelTestWindow? Should the aura
impl of CreateTestWindow just always provide a valid delegate for
ui::wm::WINDOW_TYPE_NORMAL? Should the function comment here mention the
conditions in which the window will be top-level?
also nit: 'top-level' to be consistent.

https://codereview.chromium.org/2391153002/diff/60001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:119: void SetCanLockScreen(bool can_lock_screen);
This seems a little unnecessary as an AshTest function; maybe it would be more
fitting as a static function on TestSessionStateDelegate, in some ash_test_utils
file or similar, if not simply inlined in the few places that need it.

https://codereview.chromium.org/2391153002/diff/60001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2391153002/diff/60001/ash/common/wm_window.h#...
ash/common/wm_window.h:244: // See ScreenPinningController::SetPinnedWindow()
for details.
aside: The comment there is: "Sets a pinned window. It is not allowed to call
this when there already is a pinned window." Something should actually explain
what it means for a window to be pinned. Maybe reference wm::PinWindow(), which
gives a better explanation: "Pins the window on top of other windows." (ie.
always-on-top [local to some container?])

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:106:
std::unique_ptr<WindowOwner> window_owner(
Why are so many WindowOwner instances held in std::unique_ptrs? Most of these
could be plain function-local instances allocated on the stack, afaict.

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:107:
CreateTestWindow(gfx::Rect(1, 2, 3, 4)));
It's unfortunate that we need a [temporary?] wrapper/helper (WindowOwner) for
another temporary wrapper/abstraction (WmWindow). Presumably, we'll want to
remove these when we switch production to mash and deprecate classic ash.

I was able to convert some AshTestBase tests to common types by using
CreateTestWidget instead of CreateTestWindow* (and using WmWindow as needed via
WmLookup::GetWindowForWidget). For example:
https://codereview.chromium.org/2211463002/diff/60001/ash/common/shelf/shelf_...

Converting to Widgets won't work everywhere, may require extra conversion work
now, and may add computational overhead, but I wonder if it would work well in
some cases and reduce the conversion/cleanup required later. WDYT?

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:188:
EXPECT_TRUE(WmShell::Get()->GetPrimaryRootWindow()->GetBounds().Intersects(
optional nit: cache the primary root bounds (4 calls like this)

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:355: WindowOwner
window_owner(WmShell::Get()->NewWindow(ui::wm::WINDOW_TYPE_NORMAL,
Why doesn't this use Create[Toplevel]TestWindow?

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:398: WindowOwner
window_owner(WmShell::Get()->NewWindow(ui::wm::WINDOW_TYPE_NORMAL,
Ditto: Why doesn't this use Create[Toplevel]TestWindow?

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:900: if (show) {
nit: remove curlies

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1145:
CreateToplevelTestWindow(work_area));
For my own curiosity, can you explain what goes wrong if these four
CreateToplevelTestWindow callers were changed to CreateTestWindow?

[sky, 2016-10-06 21:03:58]

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

[commit-bot: I haz the power, 2016-10-06 21:04:32]

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

[sky, 2016-10-06 21:05:59]

https://codereview.chromium.org/2391153002/diff/60001/ash/common/test/ash_test.h
File ash/common/test/ash_test.h (right):

https://codereview.chromium.org/2391153002/diff/60001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:59: // Cover functions for
WmShell::Get()->IsRunningInMash().
On 2016/10/06 20:18:24, msw wrote:
> nit: "Cover functions"? maybe just "Calls WmShell::IsRunningInMash()."?
> (it's also short enough to just inline everywhere [+wm_shell.h include])

Done.

https://codereview.chromium.org/2391153002/diff/60001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:71: // Creates a top level visible window in the
appropriate container. If
On 2016/10/06 20:18:24, msw wrote:
> I see that James already asked about this, but I need to ask. Does this not
> actually/always create a top-level window? Otherwise, what's the practical
> difference for users between this and CreateToplevelTestWindow? Should the
aura
> impl of CreateTestWindow just always provide a valid delegate for
> ui::wm::WINDOW_TYPE_NORMAL? Should the function comment here mention the
> conditions in which the window will be top-level?
> also nit: 'top-level' to be consistent.

I removed the mention to top-level here. I would rather keep these two functions
separate as it'll make the porting easier. To do otherwise means more time
porting each test, which is already painful.

https://codereview.chromium.org/2391153002/diff/60001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:119: void SetCanLockScreen(bool can_lock_screen);
On 2016/10/06 20:18:24, msw wrote:
> This seems a little unnecessary as an AshTest function; maybe it would be more
> fitting as a static function on TestSessionStateDelegate, in some
ash_test_utils
> file or similar, if not simply inlined in the few places that need it.

Agreed. Done.

https://codereview.chromium.org/2391153002/diff/60001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2391153002/diff/60001/ash/common/wm_window.h#...
ash/common/wm_window.h:244: // See ScreenPinningController::SetPinnedWindow()
for details.
On 2016/10/06 20:18:24, msw wrote:
> aside: The comment there is: "Sets a pinned window. It is not allowed to call
> this when there already is a pinned window." Something should actually explain
> what it means for a window to be pinned. Maybe reference wm::PinWindow(),
which
> gives a better explanation: "Pins the window on top of other windows." (ie.
> always-on-top [local to some container?])

I completely agree with on the lack of documentation for pinning, but I tend to
think that is out of scope of this patch (ScreenPinningController should be the
one describing all this). Even pins on top of other windows doesn't really
describe it in detail.

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:106:
std::unique_ptr<WindowOwner> window_owner(
On 2016/10/06 20:18:24, msw wrote:
> Why are so many WindowOwner instances held in std::unique_ptrs? Most of these
> could be plain function-local instances allocated on the stack, afaict.

Not sure I follow. CreateTestWIndow returns a unique_ptr<WindowOwner>. To get
what you are after I would have to make WindowOwner support move semantics.

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:107:
CreateTestWindow(gfx::Rect(1, 2, 3, 4)));
On 2016/10/06 20:18:24, msw wrote:
> It's unfortunate that we need a [temporary?] wrapper/helper (WindowOwner) for
> another temporary wrapper/abstraction (WmWindow). Presumably, we'll want to
> remove these when we switch production to mash and deprecate classic ash.
> 
> I was able to convert some AshTestBase tests to common types by using
> CreateTestWidget instead of CreateTestWindow* (and using WmWindow as needed
via
> WmLookup::GetWindowForWidget). For example:
>
https://codereview.chromium.org/2211463002/diff/60001/ash/common/shelf/shelf_...
> 
> Converting to Widgets won't work everywhere, may require extra conversion work
> now, and may add computational overhead, but I wonder if it would work well in
> some cases and reduce the conversion/cleanup required later. WDYT?

My concern is that it will make moving tests longer and more onerous. I would
like to make it as simple as possible to make tests work with both mus and ash.

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:188:
EXPECT_TRUE(WmShell::Get()->GetPrimaryRootWindow()->GetBounds().Intersects(
On 2016/10/06 20:18:24, msw wrote:
> optional nit: cache the primary root bounds (4 calls like this)

I'm going to leave as the test has always been doing this.

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:355: WindowOwner
window_owner(WmShell::Get()->NewWindow(ui::wm::WINDOW_TYPE_NORMAL,
On 2016/10/06 20:18:24, msw wrote:
> Why doesn't this use Create[Toplevel]TestWindow?

This test needs to do things in a certain order, such as no parent and hidden
(defaults) and then add it.

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:398: WindowOwner
window_owner(WmShell::Get()->NewWindow(ui::wm::WINDOW_TYPE_NORMAL,
On 2016/10/06 20:18:24, msw wrote:
> Ditto: Why doesn't this use Create[Toplevel]TestWindow?

To match old behavior (but for same reason as above).

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:900: if (show) {
On 2016/10/06 20:18:24, msw wrote:
> nit: remove curlies

Done.

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:1145:
CreateToplevelTestWindow(work_area));
On 2016/10/06 20:18:24, msw wrote:
> For my own curiosity, can you explain what goes wrong if these four
> CreateToplevelTestWindow callers were changed to CreateTestWindow?

Assertion on 1162/3 fails:

[ RUN      ] WorkspaceLayoutManagerKeyboardTest.ChildWindowFocused
../../ash/wm/workspace/workspace_layout_manager_unittest.cc:1164: Failure
Value of: parent_window->GetBounds().ToString()
  Actual: "50,50 100x324"
Expected: gfx::Rect(50, 0, 100, available_height).ToString()
Which is: "50,0 100x324"
../../ash/wm/workspace/workspace_layout_manager_unittest.cc:1167: Failure
Value of: parent_window->GetBounds().ToString()
  Actual: "50,50 100x324"
Expected: initial_window_bounds.ToString()
Which is: "50,50 100x500"
[  FAILED  ] WorkspaceLayoutManagerKeyboardTest.ChildWindowFocused (439 ms)

And that fails because WorkspaceLayoutManager::OnKeyboardBoundsChanging()
doesn't find a top-level window.

[sky, 2016-10-06 21:06:15]

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

[commit-bot: I haz the power, 2016-10-06 21:06:51]

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, 2016-10-06 21:11:21]

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

[commit-bot: I haz the power, 2016-10-06 21:11:22]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msw, 2016-10-06 21:19:16]

Okay, lgtm

https://codereview.chromium.org/2391153002/diff/60001/ash/common/wm_window.h
File ash/common/wm_window.h (right):

https://codereview.chromium.org/2391153002/diff/60001/ash/common/wm_window.h#...
ash/common/wm_window.h:244: // See ScreenPinningController::SetPinnedWindow()
for details.
On 2016/10/06 21:05:58, sky wrote:
> On 2016/10/06 20:18:24, msw wrote:
> > aside: The comment there is: "Sets a pinned window. It is not allowed to
call
> > this when there already is a pinned window." Something should actually
explain
> > what it means for a window to be pinned. Maybe reference wm::PinWindow(),
> which
> > gives a better explanation: "Pins the window on top of other windows." (ie.
> > always-on-top [local to some container?])
> 
> I completely agree with on the lack of documentation for pinning, but I tend
to
> think that is out of scope of this patch (ScreenPinningController should be
the
> one describing all this). Even pins on top of other windows doesn't really
> describe it in detail.

Acknowledged.

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
File ash/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:106:
std::unique_ptr<WindowOwner> window_owner(
On 2016/10/06 21:05:58, sky wrote:
> On 2016/10/06 20:18:24, msw wrote:
> > Why are so many WindowOwner instances held in std::unique_ptrs? Most of
these
> > could be plain function-local instances allocated on the stack, afaict.
> 
> Not sure I follow. CreateTestWIndow returns a unique_ptr<WindowOwner>. To get
> what you are after I would have to make WindowOwner support move semantics.

Ah, I was misreading this... I thought we could just do:
WindowOwner owner(AshTest::CreateTestWindow());
But I guess that'd facilitate callers mismanaging window lifetimes.

https://codereview.chromium.org/2391153002/diff/60001/ash/wm/workspace/worksp...
ash/wm/workspace/workspace_layout_manager_unittest.cc:107:
CreateTestWindow(gfx::Rect(1, 2, 3, 4)));
On 2016/10/06 21:05:58, sky wrote:
> On 2016/10/06 20:18:24, msw wrote:
> > It's unfortunate that we need a [temporary?] wrapper/helper (WindowOwner)
for
> > another temporary wrapper/abstraction (WmWindow). Presumably, we'll want to
> > remove these when we switch production to mash and deprecate classic ash.
> > 
> > I was able to convert some AshTestBase tests to common types by using
> > CreateTestWidget instead of CreateTestWindow* (and using WmWindow as needed
> via
> > WmLookup::GetWindowForWidget). For example:
> >
>
https://codereview.chromium.org/2211463002/diff/60001/ash/common/shelf/shelf_...
> > 
> > Converting to Widgets won't work everywhere, may require extra conversion
work
> > now, and may add computational overhead, but I wonder if it would work well
in
> > some cases and reduce the conversion/cleanup required later. WDYT?
> 
> My concern is that it will make moving tests longer and more onerous. I would
> like to make it as simple as possible to make tests work with both mus and
ash.

Fair enough, we'll need to remove WmWindow later anyway...

[sky, 2016-10-06 22:26:28]

The CQ bit was checked by sky@chromium.org

[sky, 2016-10-06 22:26:29]

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/2391153002/#ps120001
(title: "merge")

[commit-bot: I haz the power, 2016-10-06 22:27:16]

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-10-06 23:25:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-06 23:26:46]

Description was changed from

==========
Converts most of WorkspaceLayoutManager tests to use common code

A couple triggers crashes, which I've filed separately. The keyboard
ones won't work in mash. I plan to move those out of this file
separately.

BUG=624172,652871
TEST=covered by tests
R=jamescook@chromium.org
==========

to

==========
Converts most of WorkspaceLayoutManager tests to use common code

A couple triggers crashes, which I've filed separately. The keyboard
ones won't work in mash. I plan to move those out of this file
separately.

BUG=624172,652871
TEST=covered by tests
R=jamescook@chromium.org

Committed: https://crrev.com/8335eeddd6a5bc06515eb7c577a9196112f4583c
Cr-Commit-Position: refs/heads/master@{#423732}
==========

[commit-bot: I haz the power, 2016-10-06 23:26:48]

Patchset 7 (id:??) landed as
https://crrev.com/8335eeddd6a5bc06515eb7c577a9196112f4583c
Cr-Commit-Position: refs/heads/master@{#423732}