[top-chrome-md] Allows UpdateMaterialDesignColors to be called before MDC::Initialize()

[top-chrome-md] Allows UpdateMaterialDesignColors to be called before MDC::Initialize()

BUG=622234
Committed: https://crrev.com/69730710ade45f4cb7502f7b0ffff1b09c6a4ec5
Cr-Commit-Position: refs/heads/master@{#401707}

[varkha, 2016-06-22 16:55:14]

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

[commit-bot: I haz the power, 2016-06-22 16:55:57]

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

[varkha, 2016-06-22 16:57:31]

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

[varkha, 2016-06-22 16:57:32]

estade, Can you please take a look? Thanks!

[Evan Stade, 2016-06-22 17:17:34]

lgtm

https://codereview.chromium.org/2089263002/diff/1/chrome/browser/ui/libgtk2ui...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/2089263002/diff/1/chrome/browser/ui/libgtk2ui...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:999: if
(!ui::MaterialDesignController::is_mode_initialized())
nit, I'd combine this with the below check. Also, adding a link to the bug
wouldn't hurt.

[varkha, 2016-06-22 17:23:31]

https://codereview.chromium.org/2089263002/diff/1/chrome/browser/ui/libgtk2ui...
File chrome/browser/ui/libgtk2ui/gtk2_ui.cc (right):

https://codereview.chromium.org/2089263002/diff/1/chrome/browser/ui/libgtk2ui...
chrome/browser/ui/libgtk2ui/gtk2_ui.cc:999: if
(!ui::MaterialDesignController::is_mode_initialized())
On 2016/06/22 17:17:34, Evan Stade wrote:
> nit, I'd combine this with the below check. Also, adding a link to the bug
> wouldn't hurt.

Done.

[varkha, 2016-06-22 17:24:32]

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

[varkha, 2016-06-22 17:24:33]

+sadrul@ for an added accessor in
ui/base/material_design/material_design_controller.h.

[varkha, 2016-06-22 17:24:39]

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

[commit-bot: I haz the power, 2016-06-22 17:25:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089263002/20001

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

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

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

Dry run: This issue passed the CQ dry run.

[varkha, 2016-06-22 23:04:33]

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

[commit-bot: I haz the power, 2016-06-22 23:05:18]

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

[varkha, 2016-06-22 23:05:31]

estade@, do you agree that this look better?

[commit-bot: I haz the power, 2016-06-22 23:50:36]

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

[commit-bot: I haz the power, 2016-06-22 23:50:37]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-06-23 00:29:27]

On 2016/06/22 23:05:31, varkha wrote:
> estade@, do you agree that this look better?

I thought about this and it seems like it should work but it also seems less
full proof (I could see us failing to get the right GTK colors if we skip the
first theme change notification).

[varkha, 2016-06-23 00:38:12]

On 2016/06/23 00:29:27, Evan Stade wrote:
> On 2016/06/22 23:05:31, varkha wrote:
> > estade@, do you agree that this look better?
> 
> I thought about this and it seems like it should work but it also seems less
> full proof (I could see us failing to get the right GTK colors if we skip the
> first theme change notification).

Why would we miss the theme update once the signal is installed?

[Evan Stade, 2016-06-23 00:39:53]

On 2016/06/23 00:38:12, varkha wrote:
> On 2016/06/23 00:29:27, Evan Stade wrote:
> > On 2016/06/22 23:05:31, varkha wrote:
> > > estade@, do you agree that this look better?
> > 
> > I thought about this and it seems like it should work but it also seems less
> > full proof (I could see us failing to get the right GTK colors if we skip
the
> > first theme change notification).
> 
> Why would we miss the theme update once the signal is installed?

how do you know there's going to be another theme update after the initial one
that is being triggered by LoadGtkValues?

[sadrul, 2016-06-23 03:38:50]

On 2016/06/23 00:39:53, Evan Stade wrote:
> On 2016/06/23 00:38:12, varkha wrote:
> > On 2016/06/23 00:29:27, Evan Stade wrote:
> > > On 2016/06/22 23:05:31, varkha wrote:
> > > > estade@, do you agree that this look better?
> > > 
> > > I thought about this and it seems like it should work but it also seems
less
> > > full proof (I could see us failing to get the right GTK colors if we skip
> the
> > > first theme change notification).
> > 
> > Why would we miss the theme update once the signal is installed?
> 
> how do you know there's going to be another theme update after the initial one
> that is being triggered by LoadGtkValues?

If LoadGtkValues() triggers a theme-change signal (which probably happens if no
other gtk apps are currently running on the system?), then we should just be
getting the right values from the updated theme in LoadGtkValues(), right?

The thing that worries me a bit right now is that the call to LoadGtkValues()
apparently can trigger a theme-change signal, which means we re-enter
LoadGtkValues() before we have returned from the first call. It feels like that
could be problematic?

[varkha, 2016-06-23 14:56:24]

On 2016/06/23 00:39:53, Evan Stade wrote:
> how do you know there's going to be another theme update after the initial one
> that is being triggered by LoadGtkValues?

So I think what we need to make sure is that all things done in OnThemeChanged()
callback are accounted for during Gtk2UI::Initialize(). This way if theme change
is not sent during initialization we end up loading updated values anyway and
no harm is done.

OnThemeChanged() is only calling Gtk2UI::ResetStyle() which is:
- clearing them data (should not be a concern during initialization)
- calling LoadGtkValues() (is already being called during initialization)
- calling UpdateMaterialDesignColors (this is done later during initialization)
- notifying observers (should be no observers yet during initialization)

So effectively theme update from inside Gtk2UI::Initialize() is redundant.
Is this logic working? I still prefer the last patch set because it solves the
reentrant call to LoadGtkValues() and makes logic less fragile.

I don't object to just (or also) making UpdateMaterialDesignColors() no-op until
MDC is initialized but that seems redundant and should go away soon in any case.
estade@, what do you think is the best way here?

[Evan Stade, 2016-06-23 17:23:21]

> If LoadGtkValues() triggers a theme-change signal (which probably happens if
no
> other gtk apps are currently running on the system?), then we should just be
> getting the right values from the updated theme in LoadGtkValues(), right?

I dunno, the call stack shows that calling NativeThemeGtk2::GetWindow() causes
the theme change signal, but what if the first system color loaded by
LoadGtkValues doesn't use GetWindow() but rather GetLabel() and for some reason
the latter doesn't kickstart the theme system? Then we get some wrong color for
labels. Currently the label color would be wiped and reset after the
GetWindow()-triggered theme change.

> The thing that worries me a bit right now is that the call to LoadGtkValues()
> apparently can trigger a theme-change signal, which means we re-enter
> LoadGtkValues() before we have returned from the first call. It feels like
that
> could be problematic?

hypothetically, but the latest patchset doesn't fix that. If we get a theme
change callback and call LoadGtkValues and that causes another theme change
callback... we could get stuck. 

The current patch is imo more subtle; it's not at all obvious why we'd put
LoadGtkValues before the theme change signal hookup (if it were obvious, we'd
have already done it) and it fixes this particular bug, but doesn't guard
against similar ones in the future (i.e. we should think of the bug as "getting
a theme change callback at an unexpected time before MDC is ready causes a
crash").

[varkha, 2016-06-23 19:25:27]

Thanks estade@. sadrul@, I've reverted to Patch Set 2. Do you think this would
OK for now?

[sadrul, 2016-06-23 19:30:29]

sure. this lgtm too

https://codereview.chromium.org/2089263002/diff/60001/ui/base/material_design...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/2089263002/diff/60001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:54: static bool
is_mode_initialized_;
This is now being used in non-test case. Please update the doc.

[varkha, 2016-06-23 19:40:03]

https://codereview.chromium.org/2089263002/diff/60001/ui/base/material_design...
File ui/base/material_design/material_design_controller.h (right):

https://codereview.chromium.org/2089263002/diff/60001/ui/base/material_design...
ui/base/material_design/material_design_controller.h:54: static bool
is_mode_initialized_;
On 2016/06/23 19:30:29, sadrul wrote:
> This is now being used in non-test case. Please update the doc.

Done.

[varkha, 2016-06-23 19:40:07]

The CQ bit was checked by varkha@chromium.org

[varkha, 2016-06-23 19:40:08]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
sadrul@chromium.org
Link to the patchset: https://codereview.chromium.org/2089263002/#ps80001
(title: "[top-chrome-md] Allows UpdateMaterialDesignColors to be called before
MDC::Initialize() (doc)")

[commit-bot: I haz the power, 2016-06-23 19:40:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2089263002/80001

[commit-bot: I haz the power, 2016-06-23 20:37:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-23 20:41:29]

Description was changed from

==========
[top-chrome-md] Allows UpdateMaterialDesignColors to be called before
MDC::Initialize()

BUG=622234
==========

to

==========
[top-chrome-md] Allows UpdateMaterialDesignColors to be called before
MDC::Initialize()

BUG=622234

Committed: https://crrev.com/69730710ade45f4cb7502f7b0ffff1b09c6a4ec5
Cr-Commit-Position: refs/heads/master@{#401707}
==========

[commit-bot: I haz the power, 2016-06-23 20:41:30]

Patchset 5 (id:??) landed as
https://crrev.com/69730710ade45f4cb7502f7b0ffff1b09c6a4ec5
Cr-Commit-Position: refs/heads/master@{#401707}