Adds AshTest

Adds AshTest

AshTest is a new base class for tests that use ash/common. AshTest is-a
testing::Test and designed to be a mostly drop in replacement for
AshTestBase. AshTest calls through to AshTestImpl for the
implementation. An implementation of AshTestImpl is provided for aura
and mus, depending upon the target you link against.

One notable difference between AshTest and AndTestBase is CreateWindow
returns a WindowOwner, which has a WmWindow and in its destructor
calls WmWindow::destroy(). This is done to address the different lifetimes.

Future patches will make use of the AshTest.

BUG=none
TEST=test only change
R=jamescook@chromium.org,reveman@chromium.org

Committed: https://crrev.com/cd81e7cb288fb9f67310dc7ccc13a87c7e5d9b44
Cr-Commit-Position: refs/heads/master@{#421592}

[sky, 2016-09-27 23:27:52]

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

[commit-bot: I haz the power, 2016-09-27 23: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, 2016-09-28 00:24:27]

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

[commit-bot: I haz the power, 2016-09-28 00:24:28]

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_...)

[James Cook, 2016-09-28 01:19:42]

So nice to see this coming!

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
File ash/common/test/ash_test.cc (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.cc:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
super nit: 2016

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

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:28: // Wraps a WmWindow calling WmWindow::Destroy()
from the destructor.
nit: Can you add another line or two here explaining why it has to be this way?

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:48: // backend is not provided as a separate link
target.
For my knowledge: Is that because mash_unittests will link it in directly? Why
doesn't plain-old-ash_unittests do that? Because of dependencies in chrome's
tests?

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:61: // |bounds_is_screen| is empty the window is
added to the primary root window,
nit: bounds_in_screen

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:69: // Returns the Display for the secondary display.
It's assumed there is two
super nit: "there is" -> "there are"

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:82: void ConfigureWidgetInitParamsForDisplay(
It seems a little odd that this doesn't take a Display argument. Is this
expected to be a common calling pattern?

Edit: Hrm. Is the replacement for aura::Window* CurrentContext()? I wonder if we
could convert tests to use a CreateTestWidget() method.

If you don't need this function immediately, maybe hold off on it for now?

https://codereview.chromium.org/2374893002/diff/20001/ash/mus/test/ash_test_i...
File ash/mus/test/ash_test_impl_mus.cc (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/mus/test/ash_test_i...
ash/mus/test/ash_test_impl_mus.cc:19: class WmTestBaseImpl : public WmTestBase {
This is unusual, and testing::Test specifically warns against overriding
TestBody(). I think I see why you have to do it, but can you document it here?

https://codereview.chromium.org/2374893002/diff/20001/ash/mus/test/ash_test_i...
File ash/mus/test/ash_test_impl_mus.h (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/mus/test/ash_test_i...
ash/mus/test/ash_test_impl_mus.h:44: DISALLOW_COPY_AND_ASSIGN(AshTestImplMus);
#include macros.h

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_base.h
File ash/test/ash_test_base.h (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_base....
ash/test/ash_test_base.h:172: friend class ash::AshTestImplAura;
nit: no ash::

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_impl_...
File ash/test/ash_test_impl_aura.cc (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_impl_...
ash/test/ash_test_impl_aura.cc:21: class AshTestBaseImpl : public
test::AshTestBase {
ditto, as with the other one

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_impl_...
ash/test/ash_test_impl_aura.cc:73:
test::CreateDisplayLayout(display::DisplayPlacement::TOP, 0));
TOP seems like an odd default placement. Is that intentionally odd, to exercise
the code? If not, should it be RIGHT or LEFT?

[reveman, 2016-09-28 14:34:43]

components/exo lgtm

[sky, 2016-09-28 17:31:45]

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
File ash/common/test/ash_test.cc (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.cc:1: // Copyright 2015 The Chromium Authors. All
rights reserved.
On 2016/09/28 01:19:42, James Cook wrote:
> super nit: 2016

Done.

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

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:28: // Wraps a WmWindow calling WmWindow::Destroy()
from the destructor.
On 2016/09/28 01:19:42, James Cook wrote:
> nit: Can you add another line or two here explaining why it has to be this
way?

Done.

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:48: // backend is not provided as a separate link
target.
On 2016/09/28 01:19:42, James Cook wrote:
> For my knowledge: Is that because mash_unittests will link it in directly? Why
> doesn't plain-old-ash_unittests do that? Because of dependencies in chrome's
> tests?

The only place that needs the mus implementation is used is mash_unittests, so
there is no point in a separate target. OTOH the aura implementation is needed
by ash_unittests, exo_unittests and unit_tests. Thus a separate target.

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:61: // |bounds_is_screen| is empty the window is
added to the primary root window,
On 2016/09/28 01:19:42, James Cook wrote:
> nit: bounds_in_screen

Done.

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:69: // Returns the Display for the secondary display.
It's assumed there is two
On 2016/09/28 01:19:42, James Cook wrote:
> super nit: "there is" -> "there are"

Done.

https://codereview.chromium.org/2374893002/diff/20001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:82: void ConfigureWidgetInitParamsForDisplay(
On 2016/09/28 01:19:42, James Cook wrote:
> It seems a little odd that this doesn't take a Display argument. Is this
> expected to be a common calling pattern?
> 
> Edit: Hrm. Is the replacement for aura::Window* CurrentContext()? I wonder if
we
> could convert tests to use a CreateTestWidget() method.
> 
> If you don't need this function immediately, maybe hold off on it for now?

I'll need it shortly, but I can push it to the patch that actually uses it.

https://codereview.chromium.org/2374893002/diff/20001/ash/mus/test/ash_test_i...
File ash/mus/test/ash_test_impl_mus.cc (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/mus/test/ash_test_i...
ash/mus/test/ash_test_impl_mus.cc:19: class WmTestBaseImpl : public WmTestBase {
On 2016/09/28 01:19:42, James Cook wrote:
> This is unusual, and testing::Test specifically warns against overriding
> TestBody(). I think I see why you have to do it, but can you document it here?

Done.

https://codereview.chromium.org/2374893002/diff/20001/ash/mus/test/ash_test_i...
File ash/mus/test/ash_test_impl_mus.h (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/mus/test/ash_test_i...
ash/mus/test/ash_test_impl_mus.h:44: DISALLOW_COPY_AND_ASSIGN(AshTestImplMus);
On 2016/09/28 01:19:42, James Cook wrote:
> #include macros.h

Done.

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_base.h
File ash/test/ash_test_base.h (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_base....
ash/test/ash_test_base.h:172: friend class ash::AshTestImplAura;
On 2016/09/28 01:19:42, James Cook wrote:
> nit: no ash::

ash:: is necessary in this case because the current namespace is ash::test.

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_impl_...
File ash/test/ash_test_impl_aura.cc (right):

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_impl_...
ash/test/ash_test_impl_aura.cc:21: class AshTestBaseImpl : public
test::AshTestBase {
On 2016/09/28 01:19:42, James Cook wrote:
> ditto, as with the other one

Done.

https://codereview.chromium.org/2374893002/diff/20001/ash/test/ash_test_impl_...
ash/test/ash_test_impl_aura.cc:73:
test::CreateDisplayLayout(display::DisplayPlacement::TOP, 0));
On 2016/09/28 01:19:42, James Cook wrote:
> TOP seems like an odd default placement. Is that intentionally odd, to
exercise
> the code? If not, should it be RIGHT or LEFT?

Actually, it should be position.

[sky, 2016-09-28 17:32:15]

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

[commit-bot: I haz the power, 2016-09-28 17:32:53]

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-09-28 17:34:56]

Did you forget to commit/upload a patch? I'm not seeing some of the changes you
mentioned in comments.

[sky, 2016-09-28 18:04:08]

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

[commit-bot: I haz the power, 2016-09-28 18:04:29]

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

[sky, 2016-09-28 18:04:49]

On 2016/09/28 17:34:56, James Cook wrote:
> Did you forget to commit/upload a patch? I'm not seeing some of the changes
you
> mentioned in comments.

Sorry, not sure what happened. They should be there now.

[James Cook, 2016-09-28 18:13:53]

LGTM!

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

https://codereview.chromium.org/2374893002/diff/60001/ash/common/test/ash_tes...
ash/common/test/ash_test.h:31: // destructor.
Nice comment.

[sky, 2016-09-28 18:17:11]

The CQ bit was unchecked by sky@chromium.org

[sky, 2016-09-28 18:17:15]

The CQ bit was checked by sky@chromium.org

[sky, 2016-09-28 18:17:16]

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

[commit-bot: I haz the power, 2016-09-28 18:17:25]

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-09-28 18:56:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-28 18:58:32]

Description was changed from

==========
Adds AshTest

AshTest is a new base class for tests that use ash/common. AshTest is-a
testing::Test and designed to be a mostly drop in replacement for
AshTestBase. AshTest calls through to AshTestImpl for the
implementation. An implementation of AshTestImpl is provided for aura
and mus, depending upon the target you link against.

One notable difference between AshTest and AndTestBase is CreateWindow
returns a WindowOwner, which has a WmWindow and in its destructor
calls WmWindow::destroy(). This is done to address the different lifetimes.

Future patches will make use of the AshTest.

BUG=none
TEST=test only change
R=jamescook@chromium.org,reveman@chromium.org
==========

to

==========
Adds AshTest

AshTest is a new base class for tests that use ash/common. AshTest is-a
testing::Test and designed to be a mostly drop in replacement for
AshTestBase. AshTest calls through to AshTestImpl for the
implementation. An implementation of AshTestImpl is provided for aura
and mus, depending upon the target you link against.

One notable difference between AshTest and AndTestBase is CreateWindow
returns a WindowOwner, which has a WmWindow and in its destructor
calls WmWindow::destroy(). This is done to address the different lifetimes.

Future patches will make use of the AshTest.

BUG=none
TEST=test only change
R=jamescook@chromium.org,reveman@chromium.org

Committed: https://crrev.com/cd81e7cb288fb9f67310dc7ccc13a87c7e5d9b44
Cr-Commit-Position: refs/heads/master@{#421592}
==========

[commit-bot: I haz the power, 2016-09-28 18:58:33]

Patchset 4 (id:??) landed as
https://crrev.com/cd81e7cb288fb9f67310dc7ccc13a87c7e5d9b44
Cr-Commit-Position: refs/heads/master@{#421592}