Makes MaterialDesignController initialization explicit

Makes MaterialDesignController initialization explicit

Also delays updating Material Design specific colors in GTK UI until after
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreadsImpl().
Updates unit tests to avoid lazy initialization.

BUG=595108
Committed: https://crrev.com/147afe55fdc0f5b25d3b33090b2355ec67ce04b0
Cr-Commit-Position: refs/heads/master@{#386582}

[varkha, 2016-04-01 00:40:03]

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

[commit-bot: I haz the power, 2016-04-01 00:40:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/1

[varkha, 2016-04-01 00:42:52]

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

[varkha, 2016-04-01 00:42:52]

sky@, notwithstanding the perils mentioned in the bug comment #7, does this make
sense to you?

[sky, 2016-04-01 02:13:07]

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

[sky, 2016-04-01 02:13:08]

No doubt this has subtle ramifications. I don't think I know what those might
be.
Adding +estade

[sky, 2016-04-01 02:13:36]

Description was changed from

==========
Delays LinuxUI initialization till PreCreateThreads

Doing this avoids kicking MaterialDesignController::GetMode() before
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreads()

BUG=595108
==========

to

==========
Delays LinuxUI initialization till PreCreateThreads

Doing this avoids kicking MaterialDesignController::GetMode() before
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreads()

BUG=595108
==========

[sky, 2016-04-01 02:13:36]

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

[sky, 2016-04-01 02:13:49]

+thestig know may know too

[sky, 2016-04-01 02:14:41]

That gibberish didn't make sense. I meant '+thestig who may know too'

On Thu, Mar 31, 2016 at 7:13 PM,  <sky@chromium.org> wrote:
> +thestig know may know too
>
> https://codereview.chromium.org/1845343005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2016-04-01 05:10:19]

It's a bit weird to move the call because:

a) ToolkitInitialized() is for doing toolkit initialization.
b) All the code that follows assume toolkit initialization has occurred. Though
I have not spotted any code that requires toolkit init.

Even with this CL, isn't ChromeMainDelegate::PreSandboxStartup() still going to
call ui::MaterialDesignController::InitializeMode() before
ChromeBrowserMainExtraPartsViewsLinux::PreCreateThreads() ?

[commit-bot: I haz the power, 2016-04-01 06:42:26]

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

[commit-bot: I haz the power, 2016-04-01 06:42:27]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2016-04-01 17:03:20]

I don't know what this breaks but I'm afraid it breaks something.

https://codereview.chromium.org/1845343005/diff/1/chrome/browser/ui/views/chr...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/1845343005/diff/1/chrome/browser/ui/views/chr...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:80:
ChromeBrowserMainExtraPartsViews::ToolkitInitialized();
do you still need this?

https://codereview.chromium.org/1845343005/diff/1/chrome/browser/ui/views/chr...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:84:
views::LinuxUI::instance()->Initialize();
This seems wrong. According to documentation, this should be part of
ToolkitInitialized. Also note this comment:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

[varkha, 2016-04-01 17:43:08]

On 2016/04/01 05:10:19, Lei Zhang wrote:
> It's a bit weird to move the call because:
> 
> a) ToolkitInitialized() is for doing toolkit initialization.
Acq but not sure what alternative is there.

> b) All the code that follows assume toolkit initialization has occurred.
Though
> I have not spotted any code that requires toolkit init.

I haven't either in linux path. I have placed DCHECKs that confirm that
LinuxUI::instance() is
not invoked before ChromeBrowserMainExtraPartsViewsLinux::PreCreateThreads() so
Initialize() is the first thing that happens to g_linux_ui instance.

> Even with this CL, isn't ChromeMainDelegate::PreSandboxStartup() still going
to
> call ui::MaterialDesignController::InitializeMode() before 
> ChromeBrowserMainExtraPartsViewsLinux::PreCreateThreads() ?

Yes, in zygote. Would this break anything though - MD-wise?

[varkha, 2016-04-01 18:36:23]

https://codereview.chromium.org/1845343005/diff/1/chrome/browser/ui/views/chr...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/1845343005/diff/1/chrome/browser/ui/views/chr...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:80:
ChromeBrowserMainExtraPartsViews::ToolkitInitialized();
On 2016/04/01 17:03:20, Evan Stade wrote:
> do you still need this?

No, it is a NOP.

https://codereview.chromium.org/1845343005/diff/1/chrome/browser/ui/views/chr...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:84:
views::LinuxUI::instance()->Initialize();
On 2016/04/01 17:03:20, Evan Stade wrote:
> This seems wrong. According to documentation, this should be part of
> ToolkitInitialized. Also note this comment:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

Yes, this is probably a non-starter although I could not find what it really
breaks (if anything) on Linux.

Do you think it would be promising to see if we can isolate generation of
colors_[ThemeProperties::COLOR_BACKGROUND_TAB_TEXT] in gtk2_ui.cc? That seems to
be the only thing that needs to know MD mode so maybe we could leave the code
as-is but add a call through that updates the colors to MD.

[varkha, 2016-04-01 19:05:04]

Patchset #2 (id:20001) has been deleted

[varkha, 2016-04-01 19:09:03]

I've uploaded a patch that no longer changes the order of initialization but
still kicks MD controller later. PTAL.

https://codereview.chromium.org/1845343005/diff/1/chrome/browser/ui/views/chr...
File chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc
(right):

https://codereview.chromium.org/1845343005/diff/1/chrome/browser/ui/views/chr...
chrome/browser/ui/views/chrome_browser_main_extra_parts_views_linux.cc:84:
views::LinuxUI::instance()->Initialize();
>> isolate generation of
>> colors_[ThemeProperties::COLOR_BACKGROUND_TAB_TEXT] in gtk2_ui.cc?

PTAL at the last patch set if you think that this could work.

[varkha, 2016-04-01 19:09:10]

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

[commit-bot: I haz the power, 2016-04-01 19:09:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/40001

[commit-bot: I haz the power, 2016-04-01 20:39:56]

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

[commit-bot: I haz the power, 2016-04-01 20:39:57]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-04-01 23:24:19]

https://codereview.chromium.org/1845343005/diff/40001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1845343005/diff/40001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1371: LoadGtkValues();
wouldn't you have to UpdateColors here to?

I'm not too fond of the name of UpdateColors (or UpdateWithLocalState).

In general, this approach seems fragile because it's incredibly subtle that in
some random parts of this file you can't call IsModeMaterial and some you can. I
don't know if this bug is worth trying to fix because on linux it's really easy
to pass a command line flag (which does work). I'd be in favor of just removing
the about:flags entry on Linux since it's broken.

[varkha, 2016-04-01 23:55:40]

https://codereview.chromium.org/1845343005/diff/40001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/1845343005/diff/40001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1371: LoadGtkValues();
On 2016/04/01 23:24:18, Evan Stade wrote:
> wouldn't you have to UpdateColors here to?

Yes I would.


> I'm not too fond of the name of UpdateColors (or UpdateWithLocalState).

I didn't want to bring MD into the names and couldn't think of anything better.
UpdateWithLocalState -> CommandLineReady ? Not sure if I like it any better
though.

> 
> In general, this approach seems fragile because it's incredibly subtle that in
> some random parts of this file you can't call IsModeMaterial and some you can.
I
> don't know if this bug is worth trying to fix because on linux it's really
easy
> to pass a command line flag (which does work). I'd be in favor of just
removing
> the about:flags entry on Linux since it's broken.

I think it is wrong to treat Linux any different as long as we support this
platform. Also it is currently just as fragile on Windows, we just haven't
broken it yet.
I think a better approach would be to land this and then refactor
MaterialDesignController to replace lazy initialization with explicit and assert
on any GetMode being called before the explicit init. Then we won't be breaking
it unknowingly.
I think LoadChromeResources or some place up the stack would be right to init
MD.

[Evan Stade, 2016-04-02 00:43:04]

On 2016/04/01 23:55:40, varkha wrote:
>
https://codereview.chromium.org/1845343005/diff/40001/chrome/browser/ui/libgt...
> File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):
> 
>
https://codereview.chromium.org/1845343005/diff/40001/chrome/browser/ui/libgt...
> chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1371: LoadGtkValues();
> On 2016/04/01 23:24:18, Evan Stade wrote:
> > wouldn't you have to UpdateColors here to?
> 
> Yes I would.
> 
> 
> > I'm not too fond of the name of UpdateColors (or UpdateWithLocalState).
> 
> I didn't want to bring MD into the names and couldn't think of anything
better.
> UpdateWithLocalState -> CommandLineReady ? Not sure if I like it any better
> though.
> 
> > 
> > In general, this approach seems fragile because it's incredibly subtle that
in
> > some random parts of this file you can't call IsModeMaterial and some you
can.
> I
> > don't know if this bug is worth trying to fix because on linux it's really
> easy
> > to pass a command line flag (which does work). I'd be in favor of just
> removing
> > the about:flags entry on Linux since it's broken.
> 
> I think it is wrong to treat Linux any different as long as we support this
> platform.

The only reason about:flags was invented to begin with was because it's hard to
pass CL flags on Windows and OSX.

> Also it is currently just as fragile on Windows, we just haven't
> broken it yet.
> I think a better approach would be to land this and then refactor
> MaterialDesignController to replace lazy initialization with explicit and
assert
> on any GetMode being called before the explicit init. Then we won't be
breaking
> it unknowingly.
> I think LoadChromeResources or some place up the stack would be right to init
> MD.

sure, if you stick something like

CHECK(!MDC::IsInitialized());
MDC::Initialize();

somewhere, that would go a ways towards addressing my fears.

[Evan Stade, 2016-04-02 00:44:45]

To be clear, part of my ambivalence towards this bug is based on the fact that
we are already defaulting to using MD on Linux, so the about:flags entry isn't
there to enable a new feature for testing, it's there to disable a new feature,
and it should go away pretty promptly one way or another.

[varkha, 2016-04-02 03:02:44]

On 2016/04/02 00:43:04, Evan Stade wrote:
> On 2016/04/01 23:55:40, varkha wrote:
> >
>
https://codereview.chromium.org/1845343005/diff/40001/chrome/browser/ui/libgt...
> > File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):
> > 
> >
>
https://codereview.chromium.org/1845343005/diff/40001/chrome/browser/ui/libgt...
> > chrome/browser/ui/libgtk2ui/gtk2_ui.cc:1371: LoadGtkValues();
> > On 2016/04/01 23:24:18, Evan Stade wrote:
> > > wouldn't you have to UpdateColors here to?
> > 
> > Yes I would.
> > 
> > 
> > > I'm not too fond of the name of UpdateColors (or UpdateWithLocalState).
> > 
> > I didn't want to bring MD into the names and couldn't think of anything
> better.
> > UpdateWithLocalState -> CommandLineReady ? Not sure if I like it any better
> > though.
> > 
> > > 
> > > In general, this approach seems fragile because it's incredibly subtle
that
> in
> > > some random parts of this file you can't call IsModeMaterial and some you
> can.
> > I
> > > don't know if this bug is worth trying to fix because on linux it's really
> > easy
> > > to pass a command line flag (which does work). I'd be in favor of just
> > removing
> > > the about:flags entry on Linux since it's broken.
> > 
> > I think it is wrong to treat Linux any different as long as we support this
> > platform.
> 
> The only reason about:flags was invented to begin with was because it's hard
to
> pass CL flags on Windows and OSX.
> 
> > Also it is currently just as fragile on Windows, we just haven't
> > broken it yet.
> > I think a better approach would be to land this and then refactor
> > MaterialDesignController to replace lazy initialization with explicit and
> assert
> > on any GetMode being called before the explicit init. Then we won't be
> breaking
> > it unknowingly.
> > I think LoadChromeResources or some place up the stack would be right to
init
> > MD.
> 
> sure, if you stick something like
> 
> CHECK(!MDC::IsInitialized());
> MDC::Initialize();
> 
> somewhere, that would go a ways towards addressing my fears.

I'll do that and since I agree this is not critical I'll do it in this CL. I'm
more worried about how the initialization path is obscure and subtly different
on different platforms so making it explicit seems like a win.

[varkha, 2016-04-04 17:15:05]

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

[commit-bot: I haz the power, 2016-04-04 17:15:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/60001

[commit-bot: I haz the power, 2016-04-04 17:37:47]

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

[commit-bot: I haz the power, 2016-04-04 17:37:48]

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

[varkha, 2016-04-04 18:30:06]

Patchset #4 (id:80001) has been deleted

[varkha, 2016-04-04 18:30:13]

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

[commit-bot: I haz the power, 2016-04-04 18:31:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/100001

[commit-bot: I haz the power, 2016-04-04 18:50:31]

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

[commit-bot: I haz the power, 2016-04-04 18:50:32]

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

[varkha, 2016-04-04 20:39:18]

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

[varkha, 2016-04-04 20:39:45]

Patchset #5 (id:120001) has been deleted

[commit-bot: I haz the power, 2016-04-04 20:40:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/140001

[commit-bot: I haz the power, 2016-04-04 20:48:40]

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

[commit-bot: I haz the power, 2016-04-04 20:48:41]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[varkha, 2016-04-04 21:07:36]

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

[commit-bot: I haz the power, 2016-04-04 21:08:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/160001

[commit-bot: I haz the power, 2016-04-04 21:48:28]

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

[commit-bot: I haz the power, 2016-04-04 21:48:30]

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

[varkha, 2016-04-04 22:03:34]

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

[commit-bot: I haz the power, 2016-04-04 22:04:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/180001

[varkha, 2016-04-04 22:17:52]

Patchset #6 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-04-04 22:58:07]

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

[commit-bot: I haz the power, 2016-04-04 22:58:08]

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

[varkha, 2016-04-04 23:26:05]

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

[commit-bot: I haz the power, 2016-04-04 23:26:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/200001

[commit-bot: I haz the power, 2016-04-04 23:59:26]

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

[commit-bot: I haz the power, 2016-04-04 23:59:27]

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

[varkha, 2016-04-05 00:15:21]

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

[commit-bot: I haz the power, 2016-04-05 00:15:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/220001

[commit-bot: I haz the power, 2016-04-05 00:20:58]

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

[commit-bot: I haz the power, 2016-04-05 00:20:59]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[varkha, 2016-04-05 00:56:53]

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

[commit-bot: I haz the power, 2016-04-05 00:57:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/240001

[commit-bot: I haz the power, 2016-04-05 01:33:04]

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

[commit-bot: I haz the power, 2016-04-05 01:33:05]

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

[varkha, 2016-04-05 01:43:50]

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

[commit-bot: I haz the power, 2016-04-05 01:44:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/260001

[varkha, 2016-04-05 01:58:48]

Description was changed from

==========
Delays LinuxUI initialization till PreCreateThreads

Doing this avoids kicking MaterialDesignController::GetMode() before
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreads()

BUG=595108
==========

to

==========
Makes MaterialDesignController initialization explicit

Also delays updating Material Design specific colors in GTK UI until after 
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreadsImpl()

BUG=595108
==========

[varkha, 2016-04-05 01:59:44]

Description was changed from

==========
Makes MaterialDesignController initialization explicit

Also delays updating Material Design specific colors in GTK UI until after 
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreadsImpl()

BUG=595108
==========

to

==========
Makes MaterialDesignController initialization explicit

Also delays updating Material Design specific colors in GTK UI until after 
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreadsImpl().
Updates unit tests to avoid lazy initialization.

BUG=595108
==========

[commit-bot: I haz the power, 2016-04-05 02:25:56]

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

[commit-bot: I haz the power, 2016-04-05 02:25:59]

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

[varkha, 2016-04-05 14:12:48]

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

[commit-bot: I haz the power, 2016-04-05 14:13:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/280001

[commit-bot: I haz the power, 2016-04-05 15:21:05]

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

[commit-bot: I haz the power, 2016-04-05 15:21:06]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-04-05 15:33:22]

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

[commit-bot: I haz the power, 2016-04-05 15:33:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/300001

[commit-bot: I haz the power, 2016-04-05 16:04:24]

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

[commit-bot: I haz the power, 2016-04-05 16:04:26]

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

[varkha, 2016-04-05 16:30:05]

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

[commit-bot: I haz the power, 2016-04-05 16:30:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/320001

[varkha, 2016-04-05 16:31:37]

Patchset #12 (id:300001) has been deleted

[commit-bot: I haz the power, 2016-04-05 17:40:51]

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

[commit-bot: I haz the power, 2016-04-05 17:40:53]

Dry run: This issue passed the CQ dry run.

[varkha, 2016-04-05 21:39:36]

Patchset #13 (id:340001) has been deleted

[varkha, 2016-04-05 21:41:13]

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

[commit-bot: I haz the power, 2016-04-05 21:41:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/360001

[varkha, 2016-04-05 22:00:38]

Patchset #14 (id:380001) has been deleted

[varkha, 2016-04-05 22:01:01]

Patchset #13 (id:360001) has been deleted

[varkha, 2016-04-05 22:01:16]

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

[commit-bot: I haz the power, 2016-04-05 22:01:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/400001

[commit-bot: I haz the power, 2016-04-05 23:21:28]

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

[commit-bot: I haz the power, 2016-04-05 23:21:30]

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

[varkha, 2016-04-06 04:48:35]

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

[commit-bot: I haz the power, 2016-04-06 04:49:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/400001

[varkha, 2016-04-06 04:49:33]

https://codereview.chromium.org/1845343005/diff/400001/ui/base/material_desig...
File ui/base/material_design/material_design_controller.cc (right):

https://codereview.chromium.org/1845343005/diff/400001/ui/base/material_desig...
ui/base/material_design/material_design_controller.cc:36:
CHECK(!is_mode_initialized_);
estade@, can you please take another look? I have reworked this CL to avoid lazy
initialization of the MDC and it should have CHECKs in place to enforce it. I
think this is less brittle than what we had before where we already had to work
around obscure initialization sequence on Mac. Of course it also fixes the
original linux bug.

[commit-bot: I haz the power, 2016-04-06 05:23:48]

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

[commit-bot: I haz the power, 2016-04-06 05:23:50]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-04-06 21:43:44]

I still question whether it's worth the changes to Linux. It's more or less a
non-goal to support the about:flags entry in Linux in M51.

https://codereview.chromium.org/1845343005/diff/400001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.h (right):

https://codereview.chromium.org/1845343005/diff/400001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.h:135: void UpdateColors();
can you mark all the changes that should be reverted when material design is
default as such? Some of these are not obvious. (The obvious ones like
MaterialDesignController::InitializeMode() don't need explicit docs)

[varkha, 2016-04-06 21:55:17]

I think the cost of supporting it is quite minimal and if it helps with being
more strict about initialization sequence it is for the better.

https://codereview.chromium.org/1845343005/diff/400001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.h (right):

https://codereview.chromium.org/1845343005/diff/400001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.h:135: void UpdateColors();
On 2016/04/06 21:43:44, Evan Stade wrote:
> can you mark all the changes that should be reverted when material design is
> default as such? Some of these are not obvious. (The obvious ones like
> MaterialDesignController::InitializeMode() don't need explicit docs)

Will do. MDC::Initialize might not go away, at least initially because it caches
hybrid mode.

[varkha, 2016-04-07 17:02:37]

PTAL.

https://codereview.chromium.org/1845343005/diff/400001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.h (right):

https://codereview.chromium.org/1845343005/diff/400001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.h:135: void UpdateColors();
On 2016/04/06 21:43:44, Evan Stade wrote:
> can you mark all the changes that should be reverted when material design is
> default as such? Some of these are not obvious. (The obvious ones like
> MaterialDesignController::InitializeMode() don't need explicit docs)

Done.

[Evan Stade, 2016-04-08 21:14:11]

linux parts LGTM with nits. I looked at the test changes but I don't feel
qualified to approve or disapprove.

https://codereview.chromium.org/1845343005/diff/420001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.h (right):

https://codereview.chromium.org/1845343005/diff/420001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.h:139: void UpdateColors();
s/UpdateColors/UpdateMaterialDesignColors/

https://codereview.chromium.org/1845343005/diff/420001/ui/views/linux_ui/linu...
File ui/views/linux_ui/linux_ui.h (right):

https://codereview.chromium.org/1845343005/diff/420001/ui/views/linux_ui/linu...
ui/views/linux_ui/linux_ui.h:85: virtual void UpdateWithLocalState() = 0;
perhaps then this should be called MaterialDesignControllerReady or something?

[varkha, 2016-04-08 23:27:06]

varkha@chromium.org changed reviewers:
+ mkwst@chromium.org

[varkha, 2016-04-08 23:27:07]

mkwst@ for OWNERS in content/shell/browser/shell_browser_main_parts.cc.

sky@ for OWNERS elsewhere.

Thanks!

https://codereview.chromium.org/1845343005/diff/420001/chrome/browser/ui/libg...
File chrome/browser/ui/libgtk2ui/gtk2_ui.h (right):

https://codereview.chromium.org/1845343005/diff/420001/chrome/browser/ui/libg...
chrome/browser/ui/libgtk2ui/gtk2_ui.h:139: void UpdateColors();
On 2016/04/08 21:14:11, Evan Stade wrote:
> s/UpdateColors/UpdateMaterialDesignColors/

Done.

https://codereview.chromium.org/1845343005/diff/420001/ui/views/linux_ui/linu...
File ui/views/linux_ui/linux_ui.h (right):

https://codereview.chromium.org/1845343005/diff/420001/ui/views/linux_ui/linu...
ui/views/linux_ui/linux_ui.h:85: virtual void UpdateWithLocalState() = 0;
On 2016/04/08 21:14:11, Evan Stade wrote:
> perhaps then this should be called MaterialDesignControllerReady or something?

Done.

[varkha, 2016-04-09 03:08:28]

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

[commit-bot: I haz the power, 2016-04-09 03:08:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/440001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/440001

[commit-bot: I haz the power, 2016-04-09 04:16:13]

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

[commit-bot: I haz the power, 2016-04-09 04:16:14]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-04-11 07:49:20]

content/shell LGTM

[sky, 2016-04-11 15:32:41]

https://codereview.chromium.org/1845343005/diff/440001/chrome/browser/ui/star...
File chrome/browser/ui/startup/bad_flags_prompt.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/chrome/browser/ui/star...
chrome/browser/ui/startup/bad_flags_prompt.cc:137: locale, NULL,
ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
Why do you need this change?

https://codereview.chromium.org/1845343005/diff/440001/chrome/service/service...
File chrome/service/service_process.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/chrome/service/service...
chrome/service/service_process.cc:184:
ui::MaterialDesignController::Initialize();
Why does the service process need material design?

https://codereview.chromium.org/1845343005/diff/440001/content/public/test/br...
File content/public/test/browser_test_base.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/content/public/test/br...
content/public/test/browser_test_base.cc:186:
ui::test::MaterialDesignControllerTestAPI::UninitializeMode();
Is it possible to supply a flag to ContentTestSuiteBase so you don't need the
uninitialize here?

https://codereview.chromium.org/1845343005/diff/440001/content/public/test/te...
File content/public/test/test_renderer_host.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/content/public/test/te...
content/public/test/test_renderer_host.cc:184: // ContentTestSuiteBase might
have already initialized
Similar comment here.

https://codereview.chromium.org/1845343005/diff/440001/ui/aura/test/aura_test...
File ui/aura/test/aura_test_base.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/ui/aura/test/aura_test...
ui/aura/test/aura_test_base.cc:37: // ContentTestSuiteBase might have already
initialized
And here.

https://codereview.chromium.org/1845343005/diff/440001/ui/base/material_desig...
File ui/base/material_design/material_design_controller_unittest.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/ui/base/material_desig...
ui/base/material_design/material_design_controller_unittest.cc:63: };
private: DISALLOW... for these.

https://codereview.chromium.org/1845343005/diff/440001/ui/base/material_desig...
ui/base/material_design/material_design_controller_unittest.cc:76:
SetCommandLineSwitch("");
"" -> std::string()

https://codereview.chromium.org/1845343005/diff/440001/ui/views/test/views_te...
File ui/views/test/views_test_base.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/ui/views/test/views_te...
ui/views/test/views_test_base.cc:30: // ContentTestSuiteBase might have already
initialized
and here

[varkha, 2016-04-11 19:30:58]

sky@, See an explanation of sorts. Maybe I misunderstood what you meant with
"supply a flag to ContentTestSuiteBase".

https://codereview.chromium.org/1845343005/diff/440001/chrome/browser/ui/star...
File chrome/browser/ui/startup/bad_flags_prompt.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/chrome/browser/ui/star...
chrome/browser/ui/startup/bad_flags_prompt.cc:137: locale, NULL,
ui::ResourceBundle::DO_NOT_LOAD_COMMON_RESOURCES);
On 2016/04/11 15:32:40, sky wrote:
> Why do you need this change?

We only load resources here to get the 2 strings and then cleanup immediately in
line 147 below. LOAD_COMMON_RESOURCES would load other resources as well and
would need to have MDC initialized before that (and with this CL that would
assert).
MaybeShowInvalidUserDataDirWarningDialog is invoked before the local state is
loaded so MDC would not yet be initialized.
An alternative would be to init and uninit MDC here but since that would be the
only place in our code base (other than tests) to require MDC::Uninitialize I
prefer to just cdo this change here.

https://codereview.chromium.org/1845343005/diff/440001/chrome/service/service...
File chrome/service/service_process.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/chrome/service/service...
chrome/service/service_process.cc:184:
ui::MaterialDesignController::Initialize();
On 2016/04/11 15:32:40, sky wrote:
> Why does the service process need material design?

InitSharedInstanceWithLocale below (inside ResourceBundle::LoadChromeResources)
checks MDC::Mode to load the right resource data pack.

https://codereview.chromium.org/1845343005/diff/440001/content/public/test/br...
File content/public/test/browser_test_base.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/content/public/test/br...
content/public/test/browser_test_base.cc:186:
ui::test::MaterialDesignControllerTestAPI::UninitializeMode();
On 2016/04/11 15:32:40, sky wrote:
> Is it possible to supply a flag to ContentTestSuiteBase so you don't need the
> uninitialize here?

Not sure how this would be possible.
The call stack I am dealing here is doing something like this:
base::TestSuite::Run {
  (first calls:)
  -> TestSuite::Initialize() -> MDC::Initialize()

  (and then, still in TestSuite::Run())
  -> testing::UnitTest::Run()
   -> UnitTestImpl::RunAllTests()
    -> ... (browser tests init)
     -> content::BrowserTestBase::SetUp()
      -> ... (BrowserMain initalization)
       -> ChromeBrowserMainParts::PreCreateThreadsImpl()
        -> MDC::Initialize()
That second call to MDC::Init we need in a production browser code. The first
call is in ContentTestSuiteBase and is necessary to make a lot of tests happy
that need to load resource packs first.

More tests have something similar - they load resource packs (and so we need to
kick MDC) and then proceed to use something like a child of ViewsTestBase or
AuraTestBase (which can also be run on their own in aura_unittests or
views_unittests and so need their own MDC::Init call).

I was trying to come up with the fewest number of (necessary and temporary evil)
calls to MaterialDesignControllerTestAPI::UninitializeMode() that would still
get all the bots happy.

https://codereview.chromium.org/1845343005/diff/440001/ui/base/material_desig...
File ui/base/material_design/material_design_controller_unittest.cc (right):

https://codereview.chromium.org/1845343005/diff/440001/ui/base/material_desig...
ui/base/material_design/material_design_controller_unittest.cc:63: };
On 2016/04/11 15:32:41, sky wrote:
> private: DISALLOW... for these.

Done.

https://codereview.chromium.org/1845343005/diff/440001/ui/base/material_desig...
ui/base/material_design/material_design_controller_unittest.cc:76:
SetCommandLineSwitch("");
On 2016/04/11 15:32:41, sky wrote:
> "" -> std::string()

Done.

[sky, 2016-04-11 20:58:55]

Ok, LGTM

[varkha, 2016-04-11 21:30:54]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-04-11 21:30:55]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1845343005/#ps460001
(title: "comments")

[commit-bot: I haz the power, 2016-04-11 21:32:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/460001

[commit-bot: I haz the power, 2016-04-12 01:19:12]

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

[commit-bot: I haz the power, 2016-04-12 01:19:14]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[varkha, 2016-04-12 02:03:20]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2016-04-12 02:04:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1845343005/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1845343005/460001

[commit-bot: I haz the power, 2016-04-12 04:24:48]

Description was changed from

==========
Makes MaterialDesignController initialization explicit

Also delays updating Material Design specific colors in GTK UI until after 
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreadsImpl().
Updates unit tests to avoid lazy initialization.

BUG=595108
==========

to

==========
Makes MaterialDesignController initialization explicit

Also delays updating Material Design specific colors in GTK UI until after 
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreadsImpl().
Updates unit tests to avoid lazy initialization.

BUG=595108
==========

[commit-bot: I haz the power, 2016-04-12 04:24:50]

Committed patchset #16 (id:460001)

[commit-bot: I haz the power, 2016-04-12 04:26:10]

Description was changed from

==========
Makes MaterialDesignController initialization explicit

Also delays updating Material Design specific colors in GTK UI until after 
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreadsImpl().
Updates unit tests to avoid lazy initialization.

BUG=595108
==========

to

==========
Makes MaterialDesignController initialization explicit

Also delays updating Material Design specific colors in GTK UI until after
about_flags::ConvertFlagsToSwitches() is called from
ChromeBrowserMainParts::PreCreateThreadsImpl().
Updates unit tests to avoid lazy initialization.

BUG=595108

Committed: https://crrev.com/147afe55fdc0f5b25d3b33090b2355ec67ce04b0
Cr-Commit-Position: refs/heads/master@{#386582}
==========

[commit-bot: I haz the power, 2016-04-12 04:26:12]

Patchset 16 (id:??) landed as
https://crrev.com/147afe55fdc0f5b25d3b33090b2355ec67ce04b0
Cr-Commit-Position: refs/heads/master@{#386582}

[kjellander_chromium, 2016-04-12 06:18:01]

A revert of this CL (patchset #16 id:460001) has been created in
https://codereview.chromium.org/1878943002/ by kjellander@chromium.org.

The reason for reverting is: Breaks message_center_unittests on  Linux Chromium
OS ASan LSan Tests (1):

NotifierSettingsViewTest.TestLearnMoreButton (run #1):
[ RUN      ] NotifierSettingsViewTest.TestLearnMoreButton
[7310:7310:0411/222127:19091916288:FATAL:material_design_controller.cc(63)]
Check failed: is_mode_initialized_.
#0 0x000000481ae1 __interceptor_backtrace
#1 0x0000006d28c3 base::debug::StackTrace::StackTrace()
#2 0x00000063f5ca logging::LogMessage::~LogMessage()
#3 0x0000008051c0 ui::MaterialDesignController::IsModeMaterial()
#4 0x000001e62411 ui::GetAuraColor()

The FindIt analysis seems correct:
https://findit-for-me.appspot.com/build-failure?url=https://build.chromium.or...
.