Set Ash material design mode in tests properly

Set Ash material design mode in tests properly

Currently, in Ash tests, material design mode is set after call to
AshTestBase::SetUp(). This is not right as some setup in AshTestBase
might depend on material mode. This CL allows setting material mode
before calling AshTestBase::SetUp().

BUG=620093
Committed: https://crrev.com/c66d687b9ffbd366ff3a1ca85acb6c0c4eab1ab1
Cr-Commit-Position: refs/heads/master@{#408837}

[mohsen, 2016-07-28 21:29:56]

Patchset #2 (id:20001) has been deleted

[mohsen, 2016-07-28 21:31:02]

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

[mohsen, 2016-07-28 21:31:33]

mohsen@chromium.org changed reviewers:
+ bruthig@chromium.org, tdanderson@chromium.org

[mohsen, 2016-07-28 21:31:33]

tdanderson@, bruthig@: Please take a look...

[commit-bot: I haz the power, 2016-07-28 21:31:46]

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-07-28 21:34:26]

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

[commit-bot: I haz the power, 2016-07-28 21:34:27]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[mohsen, 2016-07-28 21:34:50]

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

[commit-bot: I haz the power, 2016-07-28 21:35:39]

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-07-28 21:35:59]

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

[commit-bot: I haz the power, 2016-07-28 21:36:00]

Dry run: Try jobs failed on following builders:
  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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[bruthig, 2016-07-28 21:40:25]

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

https://codereview.chromium.org/2186363002/diff/40001/ash/test/ash_test_base....
ash/test/ash_test_base.h:145: material_mode_ = material_mode;
Can you add 'CHECK(setup_called_)' here?

[mohsen, 2016-07-28 21:40:30]

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

[commit-bot: I haz the power, 2016-07-28 21:40:46]

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

[mohsen, 2016-07-28 22:01:24]

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

[commit-bot: I haz the power, 2016-07-28 22:02:26]

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-07-28 22:15:40]

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

[commit-bot: I haz the power, 2016-07-28 22:15:41]

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

[mohsen, 2016-07-28 22:22:21]

Patchset #4 (id:80001) has been deleted

[tdanderson, 2016-07-29 15:16:50]

Mohsen, thanks again for looking into this. The approach lgtm and I have left a
few comments. I'd value Ben's input on this too.

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2119: class ShelfViewInkDropTest : public
ShelfViewTest {
Can you add a comment here saying that this fixture forces MD to be enabled for
the test run (i.e., test fixture is only applicable with the MD feature work)

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2128:
set_material_mode(ash::MaterialDesignController::MATERIAL_EXPERIMENTAL);
Just a thought (not necessarily something to address in this CL): if there are
going to be lots of places where we will need to make calls like this, maybe we
could change MaterialDesignController to look something like:

Mode MDC::ModeForShelf() {
  return EXPERIMENTAL;  // (*)
}

bool MDC::IsShelfMaterial() {
  return mode() == ModeForShelf();
}

then line 2128 could be:

set_material_mode(MDC::ModeForShelf());

The advantage is that when the MD shelf needs to change mode (e.g., if we
promote from EXPERIMENTAL to NORMAL) then (*) would be the only line that needs
to change.

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

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_base...
ash/test/ash_test_base.h:144: void
set_material_mode(MaterialDesignController::Mode material_mode) {
optional nit: consider calling this set_material_mode_for_test() or similar to
make it extra clear at call sites. If you do that, perhaps change
|material_mode_| to |material_mode_for_test_|.

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
ash/test/ash_test_helper.cc:129: // If |material_mode| is not set, use the value
from command line switches.
I think it would be better to have this documented in the .h above
AshTestHelper::SetUp() instead of buried in the implementation. Maybe also
explicitly call out the fact that if the |material_mode| parameter is something
other than UNINITIALIZED, any command line value will be ignored.

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
File ash/test/ash_test_helper.h (right):

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
ash/test/ash_test_helper.h:113:
std::unique_ptr<test::MaterialDesignControllerTestAPI> material_design_state_;
Can't this be owned by AshTestBase instead?

[bruthig, 2016-07-29 16:13:04]

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2119: class ShelfViewInkDropTest : public
ShelfViewTest {
On 2016/07/29 15:16:50, tdanderson wrote:
> Can you add a comment here saying that this fixture forces MD to be enabled
for
> the test run (i.e., test fixture is only applicable with the MD feature work)

Are there any tests here that are NON_MATERIAL specific, or should there be? 
i.e. Should we be running these tests with more than just the one material
design mode?

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2128:
set_material_mode(ash::MaterialDesignController::MATERIAL_EXPERIMENTAL);
On 2016/07/29 15:16:50, tdanderson wrote:
> Just a thought (not necessarily something to address in this CL): if there are
> going to be lots of places where we will need to make calls like this, maybe
we
> could change MaterialDesignController to look something like:
> 
> Mode MDC::ModeForShelf() {
>   return EXPERIMENTAL;  // (*)
> }
> 
> bool MDC::IsShelfMaterial() {
>   return mode() == ModeForShelf();
> }
> 
> then line 2128 could be:
> 
> set_material_mode(MDC::ModeForShelf());
> 
> The advantage is that when the MD shelf needs to change mode (e.g., if we
> promote from EXPERIMENTAL to NORMAL) then (*) would be the only line that
needs
> to change.

+1 to something like this. Could "ModeForShelf()" be replaced with a constant?

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
File ash/test/ash_test_helper.h (right):

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
ash/test/ash_test_helper.h:113:
std::unique_ptr<test::MaterialDesignControllerTestAPI> material_design_state_;
On 2016/07/29 15:16:50, tdanderson wrote:
> Can't this be owned by AshTestBase instead?

The TestAPI Needs to be initialized after the MaterialDesignController, (or
instead of) which is currently done on line 128 in the ash_test_helper.cc and
before the Shell instance is constructed.  So if we are to move the TestAPI we
will also have to move this initialization.  I am not sure what the original
considerations were for the initializing the controller in the helper...

[mohsen, 2016-07-29 17:44:18]

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

[commit-bot: I haz the power, 2016-07-29 17:44:39]

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

[mohsen, 2016-07-29 17:50:52]

Patchset #6 (id:140001) has been deleted

[mohsen, 2016-07-29 17:54:20]

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

[commit-bot: I haz the power, 2016-07-29 17:54:50]

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

[mohsen, 2016-07-29 18:05:20]

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
File ash/shelf/shelf_view_unittest.cc (right):

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2119: class ShelfViewInkDropTest : public
ShelfViewTest {
On 2016/07/29 at 15:16:50, tdanderson wrote:
> Can you add a comment here saying that this fixture forces MD to be enabled
for the test run (i.e., test fixture is only applicable with the MD feature
work)

Done.

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2119: class ShelfViewInkDropTest : public
ShelfViewTest {
On 2016/07/29 16:13:03, bruthig wrote:
> On 2016/07/29 15:16:50, tdanderson wrote:
> > Can you add a comment here saying that this fixture forces MD to be enabled
> for
> > the test run (i.e., test fixture is only applicable with the MD feature
work)
> 
> Are there any tests here that are NON_MATERIAL specific, or should there be? 
> i.e. Should we be running these tests with more than just the one material
> design mode?

No, these are all ripple tests and only make sense in material mode.

https://codereview.chromium.org/2186363002/diff/100001/ash/shelf/shelf_view_u...
ash/shelf/shelf_view_unittest.cc:2128:
set_material_mode(ash::MaterialDesignController::MATERIAL_EXPERIMENTAL);
On 2016/07/29 16:13:04, bruthig wrote:
> On 2016/07/29 15:16:50, tdanderson wrote:
> > Just a thought (not necessarily something to address in this CL): if there
are
> > going to be lots of places where we will need to make calls like this, maybe
> we
> > could change MaterialDesignController to look something like:
> > 
> > Mode MDC::ModeForShelf() {
> >   return EXPERIMENTAL;  // (*)
> > }
> > 
> > bool MDC::IsShelfMaterial() {
> >   return mode() == ModeForShelf();
> > }
> > 
> > then line 2128 could be:
> > 
> > set_material_mode(MDC::ModeForShelf());
> > 
> > The advantage is that when the MD shelf needs to change mode (e.g., if we
> > promote from EXPERIMENTAL to NORMAL) then (*) would be the only line that
> needs
> > to change.
> 
> +1 to something like this. Could "ModeForShelf()" be replaced with a constant?

As discussed in person, this is not simple since EXPERIMENTAL also implies
NORMAL. For example, when shelf is promoted to NORMAL, then IsShelfMaterial()
should return true not just for NORMAL but also for EXPERIMENTAL. Maybe we need
to add some sort of ordering between modes. Anyways, better to address in
anotehr CL.

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

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_base...
ash/test/ash_test_base.h:144: void
set_material_mode(MaterialDesignController::Mode material_mode) {
On 2016/07/29 at 15:16:50, tdanderson wrote:
> optional nit: consider calling this set_material_mode_for_test() or similar to
make it extra clear at call sites. If you do that, perhaps change
|material_mode_| to |material_mode_for_test_|.

This whole class is for testing purposes. What would be the point of adding
for_test suffix to this one?

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
File ash/test/ash_test_helper.cc (right):

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
ash/test/ash_test_helper.cc:129: // If |material_mode| is not set, use the value
from command line switches.
On 2016/07/29 at 15:16:50, tdanderson wrote:
> I think it would be better to have this documented in the .h above
AshTestHelper::SetUp() instead of buried in the implementation. Maybe also
explicitly call out the fact that if the |material_mode| parameter is something
other than UNINITIALIZED, any command line value will be ignored.

Right. Done.

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
File ash/test/ash_test_helper.h (right):

https://codereview.chromium.org/2186363002/diff/100001/ash/test/ash_test_help...
ash/test/ash_test_helper.h:113:
std::unique_ptr<test::MaterialDesignControllerTestAPI> material_design_state_;
On 2016/07/29 16:13:04, bruthig wrote:
> On 2016/07/29 15:16:50, tdanderson wrote:
> > Can't this be owned by AshTestBase instead?
> 
> The TestAPI Needs to be initialized after the MaterialDesignController, (or
> instead of) which is currently done on line 128 in the ash_test_helper.cc and
> before the Shell instance is constructed.  So if we are to move the TestAPI we
> will also have to move this initialization.  I am not sure what the original
> considerations were for the initializing the controller in the helper...
I believe these initializations are in AshTestHelper rather than AshTestBase so
that they can be used in other tests that need to initialize Ash (e.g.
BrowserWithTestWindowTest).

[commit-bot: I haz the power, 2016-07-29 18:45:05]

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

[commit-bot: I haz the power, 2016-07-29 18:45:06]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-07-29 19:53:25]

lgtm with nit

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

https://codereview.chromium.org/2186363002/diff/160001/ash/test/ash_test_base...
ash/test/ash_test_base.h:145: material_mode_ = material_mode;
(In case this got lost)

Can you add 'CHECK(!setup_called_)' here?

[mohsen, 2016-07-29 20:03:51]

mohsen@chromium.org changed reviewers:
+ oshima@chromium.org

[mohsen, 2016-07-29 20:03:52]

oshima@: Can you take a look, please?

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

https://codereview.chromium.org/2186363002/diff/160001/ash/test/ash_test_base...
ash/test/ash_test_base.h:145: material_mode_ = material_mode;
On 2016/07/29 at 19:53:25, bruthig wrote:
> (In case this got lost)
> 
> Can you add 'CHECK(!setup_called_)' here?

Oops! Done.

[oshima, 2016-07-29 20:12:24]

lgtm

[mohsen, 2016-07-29 20:15:33]

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

[commit-bot: I haz the power, 2016-07-29 20:16:08]

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

[mohsen, 2016-07-29 20:16:24]

mohsen@chromium.org changed reviewers:
+ thestig@chromium.org

[mohsen, 2016-07-29 20:16:25]

thestig@: Can you please take an OWNERS look at
chrome/test/base/browser_with_test_window_test.cc?

[Lei Zhang, 2016-07-29 20:43:57]

lgtm if you IWYU.

Also please note chrome/test/OWNERS exists.

https://codereview.chromium.org/2186363002/diff/180001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2186363002/diff/180001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:59:
ash::MaterialDesignController::Mode::UNINITIALIZED);
Please IWYU and #include ash/common/material_design/material_design_controller.h
above in the right place.

[commit-bot: I haz the power, 2016-07-29 20:58:44]

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

[commit-bot: I haz the power, 2016-07-29 20:58:45]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-07-29 23:55:52]

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

[commit-bot: I haz the power, 2016-07-29 23:56:13]

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

[mohsen, 2016-07-29 23:59:50]

Yeah, sorry! I noticed chrome/test/OWNERS, but they were OOO and I wanted to get
the CL reviewed before the weekend to unblock some other changes. Thanks for the
review!

https://codereview.chromium.org/2186363002/diff/180001/chrome/test/base/brows...
File chrome/test/base/browser_with_test_window_test.cc (right):

https://codereview.chromium.org/2186363002/diff/180001/chrome/test/base/brows...
chrome/test/base/browser_with_test_window_test.cc:59:
ash::MaterialDesignController::Mode::UNINITIALIZED);
On 2016/07/29 at 20:43:57, Lei Zhang wrote:
> Please IWYU and #include
ash/common/material_design/material_design_controller.h above in the right
place.

Done.

[commit-bot: I haz the power, 2016-07-30 00:37:57]

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

[commit-bot: I haz the power, 2016-07-30 00:37:58]

Dry run: This issue passed the CQ dry run.

[mohsen, 2016-07-30 01:42:26]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2016-07-30 01:42:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, bruthig@chromium.org, thestig@chromium.org,
oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/2186363002/#ps200001
(title: "Addressed review comments")

[commit-bot: I haz the power, 2016-07-30 01:42:39]

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-07-30 01:46:32]

Committed patchset #8 (id:200001)

[commit-bot: I haz the power, 2016-07-30 01:48:03]

Description was changed from

==========
Set Ash material design mode in tests properly

Currently, in Ash tests, material design mode is set after call to
AshTestBase::SetUp(). This is not right as some setup in AshTestBase
might depend on material mode. This CL allows setting material mode
before calling AshTestBase::SetUp().

BUG=620093
==========

to

==========
Set Ash material design mode in tests properly

Currently, in Ash tests, material design mode is set after call to
AshTestBase::SetUp(). This is not right as some setup in AshTestBase
might depend on material mode. This CL allows setting material mode
before calling AshTestBase::SetUp().

BUG=620093

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

[commit-bot: I haz the power, 2016-07-30 01:48:05]

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