Fix first time switching from classic to GTK theme.

Fix first time switching from classic to GTK theme.

Make sure Widget observes the correct NativeTheme. Wait until everything else is done initializing so GetNativeTheme() returns the right thing.

BUG=384492
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278334

[Elliot Glaysher, 2014-06-13 20:05:54]

lgtm++

https://codereview.chromium.org/330873003/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service_aurax11.cc (right):

https://codereview.chromium.org/330873003/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_aurax11.cc:46: // Both old and new themes'
observers should be notified of the change.
Could you please expand this comment to include more information that you put in
the commit message? This is hack, and should have a bigger explanation.

[Evan Stade, 2014-06-13 20:14:39]

https://codereview.chromium.org/330873003/diff/1/chrome/browser/themes/theme_...
File chrome/browser/themes/theme_service_aurax11.cc (right):

https://codereview.chromium.org/330873003/diff/1/chrome/browser/themes/theme_...
chrome/browser/themes/theme_service_aurax11.cc:46: // Both old and new themes'
observers should be notified of the change.
On 2014/06/13 20:05:54, Elliot Glaysher wrote:
> Could you please expand this comment to include more information that you put
in
> the commit message? This is hack, and should have a bigger explanation.

Done.

[Evan Stade, 2014-06-16 18:16:43]

pkotwicz, could you provide an OWNER review? thanks.

[pkotwicz, 2014-06-17 14:25:57]

Sorry for the delay, I am looking at the CL now

[pkotwicz, 2014-06-17 15:44:03]

Your CL looks good.

Based on investigation on my end, the reason the views::Widget initially adds an
observer to the wrong ui::NativeTheme is that when Widget::InitTheme() requests
the native theme, Profile::kProfileKey has not yet been set on the
DesktopNativeWidgetAura's aura::Window. I am probably naive but delaying
"observer_manager_.Add(GetNativeTheme())" to the end of Widget::Init() seems to
fix the bug.

I suspect that my suggestion is a bad one. For the sake of the next guy who
comes along and tries to fix this properly can you list the things which you
have tried and which have failed as a response to my review comments? (I often
go digging through old CLs so this would be useful for the likes of me)

[Evan Stade, 2014-06-17 20:04:22]

On 2014/06/17 15:44:03, pkotwicz wrote:
> Your CL looks good.
> 
> Based on investigation on my end, the reason the views::Widget initially adds
an
> observer to the wrong ui::NativeTheme is that when Widget::InitTheme()
requests
> the native theme, Profile::kProfileKey has not yet been set on the
> DesktopNativeWidgetAura's aura::Window. I am probably naive but delaying
> "observer_manager_.Add(GetNativeTheme())" to the end of Widget::Init() seems
to
> fix the bug.

I'll have to test this, but do you know why that works? What part of the block
of code after observer_manager_.Add(GetNativeTheme()) makes a difference?

ViewsDelegate::views_delegate->OnBeforeWidgetInit() already comes before
observer_manager_.Add(GetNativeTheme()).

> 
> I suspect that my suggestion is a bad one. For the sake of the next guy who
> comes along and tries to fix this properly can you list the things which you
> have tried and which have failed as a response to my review comments? (I often
> go digging through old CLs so this would be useful for the likes of me)

[Evan Stade, 2014-06-17 23:51:38]

now that the fix is in widget.cc, +sky

[sky, 2014-06-18 03:24:10]

LGTM with better comment.

https://codereview.chromium.org/330873003/diff/40001/ui/views/widget/widget.cc
File ui/views/widget/widget.cc (right):

https://codereview.chromium.org/330873003/diff/40001/ui/views/widget/widget.c...
ui/views/widget/widget.cc:380: // This must come after SetContentsView(). See
http://crbug.com/375545
Document why, I shouldn't have to chase down a bug to figure that out. In other
words, the comment should stand on its own.

[pkotwicz, 2014-06-18 14:27:46]

LGTM, but can you please make your CL description more descriptive?

[Evan Stade, 2014-06-19 02:06:22]

tried to improve the cl desc, code comment, and fixed comment link

[Evan Stade, 2014-06-19 02:06:28]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-06-19 02:08:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/330873003/60001

[commit-bot: I haz the power, 2014-06-19 11:45:09]

Change committed as 278334