Update Windows UI on system color changes.

Update Windows UI on system color changes.

Propagate system theme (high contrast, etc.) changes throughout Views.
(Win UI wasn't being notified; omnibox, find bar, etc. were broken, now aren't).
(Linux Aura didn't update on changes with "Use Classic Theme", now it does).

Make NativeTheme hold an observer list to be notified of changes.
NotifyObservers of system color changes on Win via SysColorChangeListener.
Nix NativeTheme duties for ThemeServiceAuraX11, and LinuxUI / Gtk2UI.

Make Widget a NativeThemeObserver to call PropagateNativeThemeChanged.
Start observing the relevant NativeTheme after native widget initialization.

Make View::AddChildViewAt only update child views, not itself and siblings.
(this was invoking redundant calls on itself as the parent and sibling views)

Implement the BrowserView::OnNativeThemeChanged override.
(update the frame and trigger a repaint with UserChangedTheme)
(call MaybeShowInvertBubbleView, nix SysColorChangeListener impl)

TODO(followup): Audit Views that need OnNativeThemeChanged overrides.

BUG=134766
TEST=Win/Linux UI updates better when applying high contrast black/inverse system themes.
R=erg@chromium.org, estade@chromium.org, sky@chromium.org

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

[msw, 2014-04-21 18:43:58]

Hey Elliot and Scott, please take a look; thanks!

[Elliot Glaysher, 2014-04-21 23:00:54]

Perhaps I'm misreading this, but this patch seems to conflict with estade's
patch (https://codereview.chromium.org/243633003/) that makes the NativeTheme
variable? Does your patch deal with the actual NativeTheme object changing to a
different instance?

[msw, 2014-04-21 23:35:52]

On 2014/04/21 23:00:54, Elliot Glaysher wrote:
> Perhaps I'm misreading this, but this patch seems to conflict with estade's
> patch (https://codereview.chromium.org/243633003/) that makes the NativeTheme
> variable? Does your patch deal with the actual NativeTheme object changing to
a
> different instance?

I don't think the patches actually conflict, but Evan should take a look.
(I guess Gtk2Border objects can just use NativeThemeGtk2::instance() directly)

This does not add handling for switching or using multiple NativeTheme objects.
(it presumes Widget::GetNativeTheme is correct upon RootView construction)

[Evan Stade, 2014-04-22 00:32:31]

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:24: class NATIVE_THEME_EXPORT NativeThemeObserver
{
yea, this does conflict with my patch in the sense that git would throw a fit.
But ideologically I think they can coexist. This should not be called
NativeThemeObserver but SystemThemeObserver.

[msw - DO NOT USE, 2014-04-22 01:16:44]

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:24: class NATIVE_THEME_EXPORT NativeThemeObserver
{
On 2014/04/22 00:32:32, Evan Stade wrote:
> yea, this does conflict with my patch in the sense that git would throw a fit.
> But ideologically I think they can coexist. This should not be called
> NativeThemeObserver but SystemThemeObserver.

I'm not so sure about that naming... Shouldn't observers of changes to
NativeTheme instances be called NativeThemeObserver? Sure, the NativeTheme
colors changing here are derived from system values, but that doesn't really
warrant a name like SystemThemeObserver nor SystemColorObserver.

[Evan Stade, 2014-04-22 17:43:44]

https://codereview.chromium.org/239093007/diff/250001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/invert_bubble_view.cc (right):

https://codereview.chromium.org/239093007/diff/250001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/invert_bubble_view.cc:18: #include
"ui/gfx/sys_color_change_listener.h"
why is this needed?

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.cc (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.cc:45: OnNativeThemeChange();
The native theme won't always change just because the system colors have changed
-- it depends on the theme. I don't think the base NativeTheme class should do
this; leave it up to subclasses.

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:24: class NATIVE_THEME_EXPORT NativeThemeObserver
{
On 2014/04/22 01:16:45, msw - DO NOT USE wrote:
> On 2014/04/22 00:32:32, Evan Stade wrote:
> > yea, this does conflict with my patch in the sense that git would throw a
fit.
> > But ideologically I think they can coexist. This should not be called
> > NativeThemeObserver but SystemThemeObserver.
> 
> I'm not so sure about that naming... Shouldn't observers of changes to
> NativeTheme instances be called NativeThemeObserver? Sure, the NativeTheme
> colors changing here are derived from system values, but that doesn't really
> warrant a name like SystemThemeObserver nor SystemColorObserver.

Now that I see this isn't just replacing the LinuxUI NativeThemeObserver
interface, I guess you're right, but I'd still like to avoid reusing
OnNativeThemeChange(d). Instead can we call it RefreshNativeTheme() and also
pass a pointer to the NativeTheme object? Also I think we can handle the case of
the actual NativeTheme object changing pretty easily:

void RootView::RefreshNativeTheme(NativeTheme* previous_theme) {
  NativeTheme* current_native_theme = GetNativeTheme();

  if (previous_theme != current_native_theme) {
    observing_.Remove(previous_theme);
    observing_.Add(current_native_theme);
  }

  PropagateNativeThemeChanged(current_native_theme);
}

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:329: void OnNativeThemeChange();
I think this should be protected

https://codereview.chromium.org/239093007/diff/250001/ui/views/widget/root_vi...
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/239093007/diff/250001/ui/views/widget/root_vi...
ui/views/widget/root_view.cc:123: GetNativeTheme()->AddObserver(this);
ScopedObserver?

[msw, 2014-04-22 19:48:58]

https://codereview.chromium.org/239093007/diff/250001/chrome/browser/ui/views...
File chrome/browser/ui/views/accessibility/invert_bubble_view.cc (right):

https://codereview.chromium.org/239093007/diff/250001/chrome/browser/ui/views...
chrome/browser/ui/views/accessibility/invert_bubble_view.cc:18: #include
"ui/gfx/sys_color_change_listener.h"
On 2014/04/22 17:43:44, Evan Stade wrote:
> why is this needed?

For gfx::IsInvertedColorScheme, perhaps not an ideal home.

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.cc (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.cc:45: OnNativeThemeChange();
On 2014/04/22 17:43:44, Evan Stade wrote:
> The native theme won't always change just because the system colors have
changed
> -- it depends on the theme. I don't think the base NativeTheme class should do
> this; leave it up to subclasses.

Sadly, I'd have to do this for NativeThemeWin and NativeThemeAuraWin (which uses
NativeThemeAura as its base class) if not here. Also, SysColorChangeListener
could possibly be extended in a followup CL to catch the GTK frame "style-set"
signal and then this would be applicable for NativeThemeGtk2. Thoughts?

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:24: class NATIVE_THEME_EXPORT NativeThemeObserver
{
On 2014/04/22 17:43:44, Evan Stade wrote:
> On 2014/04/22 01:16:45, msw - DO NOT USE wrote:
> > On 2014/04/22 00:32:32, Evan Stade wrote:
> > > yea, this does conflict with my patch in the sense that git would throw a
> fit.
> > > But ideologically I think they can coexist. This should not be called
> > > NativeThemeObserver but SystemThemeObserver.
> > 
> > I'm not so sure about that naming... Shouldn't observers of changes to
> > NativeTheme instances be called NativeThemeObserver? Sure, the NativeTheme
> > colors changing here are derived from system values, but that doesn't really
> > warrant a name like SystemThemeObserver nor SystemColorObserver.
> 
> Now that I see this isn't just replacing the LinuxUI NativeThemeObserver
> interface, I guess you're right, but I'd still like to avoid reusing
> OnNativeThemeChange(d). Instead can we call it RefreshNativeTheme() and also
> pass a pointer to the NativeTheme object? Also I think we can handle the case
of
> the actual NativeTheme object changing pretty easily:
> 
> void RootView::RefreshNativeTheme(NativeTheme* previous_theme) {
>   NativeTheme* current_native_theme = GetNativeTheme();
> 
>   if (previous_theme != current_native_theme) {
>     observing_.Remove(previous_theme);
>     observing_.Add(current_native_theme);
>   }
> 
>   PropagateNativeThemeChanged(current_native_theme);
> }

I renamed this to OnNativeThemeUpdate, but I'd like to avoid handling changes to
the NativeTheme instance in this CL for a couple reasons:

1) RootView isn't notified when its Widget's NativeTheme object is "changed"
because this isn't a well-defined event, it's merely the implicit result of
DesktopWindowTreeHost::GetNativeTheme returning a different value sometime after
RootView construction (perhaps when the aura::Window's WindowTreeHost is
set/initialized?). So the change of NativeTheme objects only crops up once the
NativeTheme's observers are notified due to a system color change. This code is
awful; suggestions are welcome, but I think 'changing' NativeTheme objects
should be addressed separately.

2) Modifying the observer list while notifying the observers seems ripe for
disaster; in fact, this proposed solution causes a crash for me in local
testing.

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:329: void OnNativeThemeChange();
On 2014/04/22 17:43:44, Evan Stade wrote:
> I think this should be protected

I agree, but Gtk2UI::OnStyleSet calls this. Perhaps that could be made to work
like SysColorChangeListener in a followup CL (see my native_theme.cc reply).

https://codereview.chromium.org/239093007/diff/250001/ui/views/widget/root_vi...
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/239093007/diff/250001/ui/views/widget/root_vi...
ui/views/widget/root_view.cc:123: GetNativeTheme()->AddObserver(this);
On 2014/04/22 17:43:44, Evan Stade wrote:
> ScopedObserver?

Eh, it doesn't seem worthwhile to add a class member to handle one
RemoveObserver call; what do you think?

[Evan Stade, 2014-04-22 21:09:17]

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.cc (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.cc:45: OnNativeThemeChange();
On 2014/04/22 19:48:58, msw wrote:
> On 2014/04/22 17:43:44, Evan Stade wrote:
> > The native theme won't always change just because the system colors have
> changed
> > -- it depends on the theme. I don't think the base NativeTheme class should
do
> > this; leave it up to subclasses.
> 
> Sadly, I'd have to do this for NativeThemeWin and NativeThemeAuraWin (which
uses
> NativeThemeAura as its base class) if not here. Also, SysColorChangeListener
> could possibly be extended in a followup CL to catch the GTK frame "style-set"
> signal and then this would be applicable for NativeThemeGtk2. Thoughts?

I don't mind that you'd have to do this in more than one place; that's expected.
The amount of duplicated code is minimal.

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:24: class NATIVE_THEME_EXPORT NativeThemeObserver
{
On 2014/04/22 19:48:58, msw wrote:
> On 2014/04/22 17:43:44, Evan Stade wrote:
> > On 2014/04/22 01:16:45, msw - DO NOT USE wrote:
> > > On 2014/04/22 00:32:32, Evan Stade wrote:
> > > > yea, this does conflict with my patch in the sense that git would throw
a
> > fit.
> > > > But ideologically I think they can coexist. This should not be called
> > > > NativeThemeObserver but SystemThemeObserver.
> > > 
> > > I'm not so sure about that naming... Shouldn't observers of changes to
> > > NativeTheme instances be called NativeThemeObserver? Sure, the NativeTheme
> > > colors changing here are derived from system values, but that doesn't
really
> > > warrant a name like SystemThemeObserver nor SystemColorObserver.
> > 
> > Now that I see this isn't just replacing the LinuxUI NativeThemeObserver
> > interface, I guess you're right, but I'd still like to avoid reusing
> > OnNativeThemeChange(d). Instead can we call it RefreshNativeTheme() and also
> > pass a pointer to the NativeTheme object? Also I think we can handle the
case
> of
> > the actual NativeTheme object changing pretty easily:
> > 
> > void RootView::RefreshNativeTheme(NativeTheme* previous_theme) {
> >   NativeTheme* current_native_theme = GetNativeTheme();
> > 
> >   if (previous_theme != current_native_theme) {
> >     observing_.Remove(previous_theme);
> >     observing_.Add(current_native_theme);
> >   }
> > 
> >   PropagateNativeThemeChanged(current_native_theme);
> > }
> 
> I renamed this to OnNativeThemeUpdate, but I'd like to avoid handling changes
to
> the NativeTheme instance in this CL for a couple reasons:
> 
> 1) RootView isn't notified when its Widget's NativeTheme object is "changed"
> because this isn't a well-defined event, it's merely the implicit result of
> DesktopWindowTreeHost::GetNativeTheme returning a different value sometime
after
> RootView construction (perhaps when the aura::Window's WindowTreeHost is
> set/initialized?). So the change of NativeTheme objects only crops up once the
> NativeTheme's observers are notified due to a system color change.

Currently yes, but making this change provides a mechanism for other bits of
code to inform interested parties of changes.

> This code is
> awful; suggestions are welcome, but I think 'changing' NativeTheme objects
> should be addressed separately.

OK, well I probably will make this change soon if it's not part of this CL. As
long as you know it's coming and we can agree on it.

> 
> 2) Modifying the observer list while notifying the observers seems ripe for
> disaster; in fact, this proposed solution causes a crash for me in local
> testing.

That's strange, in your local testing why is the NativeTheme* changing? I had
thought the old != new check would prevent crashes, and that removing from old
would be safe (because of all the range checks going on in ObserverList)

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:329: void OnNativeThemeChange();
On 2014/04/22 19:48:58, msw wrote:
> On 2014/04/22 17:43:44, Evan Stade wrote:
> > I think this should be protected
> 
> I agree, but Gtk2UI::OnStyleSet calls this. Perhaps that could be made to work
> like SysColorChangeListener in a followup CL (see my native_theme.cc reply).

You can expose it on NativeThemeGtk2 but not here.

https://codereview.chromium.org/239093007/diff/250001/ui/views/widget/root_vi...
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/239093007/diff/250001/ui/views/widget/root_vi...
ui/views/widget/root_view.cc:123: GetNativeTheme()->AddObserver(this);
On 2014/04/22 19:48:58, msw wrote:
> On 2014/04/22 17:43:44, Evan Stade wrote:
> > ScopedObserver?
> 
> Eh, it doesn't seem worthwhile to add a class member to handle one
> RemoveObserver call; what do you think?

I always prefer scoping variables over doing stuff manually, which is error
prone.

[msw, 2014-04-23 00:34:03]

Please take a fresh look; thanks.

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.cc (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.cc:45: OnNativeThemeChange();
On 2014/04/22 21:09:18, Evan Stade wrote:
> On 2014/04/22 19:48:58, msw wrote:
> > On 2014/04/22 17:43:44, Evan Stade wrote:
> > > The native theme won't always change just because the system colors have
> > changed
> > > -- it depends on the theme. I don't think the base NativeTheme class
should
> do
> > > this; leave it up to subclasses.
> > 
> > Sadly, I'd have to do this for NativeThemeWin and NativeThemeAuraWin (which
> uses
> > NativeThemeAura as its base class) if not here. Also, SysColorChangeListener
> > could possibly be extended in a followup CL to catch the GTK frame
"style-set"
> > signal and then this would be applicable for NativeThemeGtk2. Thoughts?
> 
> I don't mind that you'd have to do this in more than one place; that's
expected.
> The amount of duplicated code is minimal.

I moved this over to NativeThemeWin; the observation by NativeThemeAuraWin is no
longer needed in the latest patch set, where Widget is the NativeThemeObserver
and correctly observes only NativeThemeWin.

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:24: class NATIVE_THEME_EXPORT NativeThemeObserver
{
On 2014/04/22 21:09:18, Evan Stade wrote:
> On 2014/04/22 19:48:58, msw wrote:
> > On 2014/04/22 17:43:44, Evan Stade wrote:
> > > On 2014/04/22 01:16:45, msw - DO NOT USE wrote:
> > > > On 2014/04/22 00:32:32, Evan Stade wrote:
> > > > > yea, this does conflict with my patch in the sense that git would
throw
> a
> > > fit.
> > > > > But ideologically I think they can coexist. This should not be called
> > > > > NativeThemeObserver but SystemThemeObserver.
> > > > 
> > > > I'm not so sure about that naming... Shouldn't observers of changes to
> > > > NativeTheme instances be called NativeThemeObserver? Sure, the
NativeTheme
> > > > colors changing here are derived from system values, but that doesn't
> really
> > > > warrant a name like SystemThemeObserver nor SystemColorObserver.
> > > 
> > > Now that I see this isn't just replacing the LinuxUI NativeThemeObserver
> > > interface, I guess you're right, but I'd still like to avoid reusing
> > > OnNativeThemeChange(d). Instead can we call it RefreshNativeTheme() and
also
> > > pass a pointer to the NativeTheme object? Also I think we can handle the
> case
> > of
> > > the actual NativeTheme object changing pretty easily:
> > > 
> > > void RootView::RefreshNativeTheme(NativeTheme* previous_theme) {
> > >   NativeTheme* current_native_theme = GetNativeTheme();
> > > 
> > >   if (previous_theme != current_native_theme) {
> > >     observing_.Remove(previous_theme);
> > >     observing_.Add(current_native_theme);
> > >   }
> > > 
> > >   PropagateNativeThemeChanged(current_native_theme);
> > > }
> > 
> > I renamed this to OnNativeThemeUpdate, but I'd like to avoid handling
changes
> to
> > the NativeTheme instance in this CL for a couple reasons:
> > 
> > 1) RootView isn't notified when its Widget's NativeTheme object is "changed"
> > because this isn't a well-defined event, it's merely the implicit result of
> > DesktopWindowTreeHost::GetNativeTheme returning a different value sometime
> after
> > RootView construction (perhaps when the aura::Window's WindowTreeHost is
> > set/initialized?). So the change of NativeTheme objects only crops up once
the
> > NativeTheme's observers are notified due to a system color change.
> 
> Currently yes, but making this change provides a mechanism for other bits of
> code to inform interested parties of changes.

Agreed, but I don't want to unnecessarily expand the scope of this CL.
 
> > This code is
> > awful; suggestions are welcome, but I think 'changing' NativeTheme objects
> > should be addressed separately.
> 
> OK, well I probably will make this change soon if it's not part of this CL. As
> long as you know it's coming and we can agree on it.

That's fine.

> > 2) Modifying the observer list while notifying the observers seems ripe for
> > disaster; in fact, this proposed solution causes a crash for me in local
> > testing.
> 
> That's strange, in your local testing why is the NativeTheme* changing? I had
> thought the old != new check would prevent crashes, and that removing from old
> would be safe (because of all the range checks going on in ObserverList)

This was an effect of the implicit NativeTheme change during Widget::Init, which
I alluded to above. Widget::GetNativeTheme returned one NativeTheme value on
RootView construction and returned a different value later when the original
NativeTheme notified its observers on a system color change. This is moot in the
latest patch set, where Widget is the NativeThemeObserver and only begins
observation after its native widget has been initialized (and the correct
NativeTheme is returned).

https://codereview.chromium.org/239093007/diff/250001/ui/native_theme/native_...
ui/native_theme/native_theme.h:329: void OnNativeThemeChange();
On 2014/04/22 21:09:18, Evan Stade wrote:
> On 2014/04/22 19:48:58, msw wrote:
> > On 2014/04/22 17:43:44, Evan Stade wrote:
> > > I think this should be protected
> > 
> > I agree, but Gtk2UI::OnStyleSet calls this. Perhaps that could be made to
work
> > like SysColorChangeListener in a followup CL (see my native_theme.cc reply).
> 
> You can expose it on NativeThemeGtk2 but not here.

Done (added NativeThemeGtk2::NotifyNativeThemeObservers).

https://codereview.chromium.org/239093007/diff/250001/ui/views/widget/root_vi...
File ui/views/widget/root_view.cc (right):

https://codereview.chromium.org/239093007/diff/250001/ui/views/widget/root_vi...
ui/views/widget/root_view.cc:123: GetNativeTheme()->AddObserver(this);
On 2014/04/22 21:09:18, Evan Stade wrote:
> On 2014/04/22 19:48:58, msw wrote:
> > On 2014/04/22 17:43:44, Evan Stade wrote:
> > > ScopedObserver?
> > 
> > Eh, it doesn't seem worthwhile to add a class member to handle one
> > RemoveObserver call; what do you think?
> 
> I always prefer scoping variables over doing stuff manually, which is error
> prone.

Done (in Widget).

[Evan Stade, 2014-04-24 01:14:51]

https://codereview.chromium.org/239093007/diff/360001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/239093007/diff/360001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:80:
NativeThemeGtk2::instance()->AddObserver(this);
nit: ScopedObserver

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

https://codereview.chromium.org/239093007/diff/360001/ui/views/widget/widget....
ui/views/widget/widget.cc:1346: void Widget::OnNativeThemeUpdate() {
I still think this should pass the calling native theme. For now you could
DCHECK_EQ(theme, GetNativeTheme()).

[msw, 2014-04-24 18:32:00]

Comments addressed.

https://codereview.chromium.org/239093007/diff/360001/chrome/browser/ui/libgt...
File chrome/browser/ui/libgtk2ui/gtk2_border.cc (right):

https://codereview.chromium.org/239093007/diff/360001/chrome/browser/ui/libgt...
chrome/browser/ui/libgtk2ui/gtk2_border.cc:80:
NativeThemeGtk2::instance()->AddObserver(this);
On 2014/04/24 01:14:52, Evan Stade wrote:
> nit: ScopedObserver

Done.

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

https://codereview.chromium.org/239093007/diff/360001/ui/views/widget/widget....
ui/views/widget/widget.cc:1346: void Widget::OnNativeThemeUpdate() {
On 2014/04/24 01:14:52, Evan Stade wrote:
> I still think this should pass the calling native theme. For now you could
> DCHECK_EQ(theme, GetNativeTheme()).

Done; this is a good idea, thanks for pushing for it.

[Evan Stade, 2014-04-24 20:29:09]

lgtm

[msw, 2014-04-24 20:36:19]

Elliot and Scott, please take a look; thanks!

[Elliot Glaysher, 2014-04-24 21:08:56]

lgtm

[sky, 2014-04-24 22:52:01]

https://codereview.chromium.org/239093007/diff/380001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/239093007/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1847:
chrome::MaybeShowInvertBubbleView(this);
estade's patch made it show that you'll hit this on startup. Is that OK? By that
I mean we may end up triggering MaybeShowInvertBubbleView before we're visible
and during startup.

https://codereview.chromium.org/239093007/diff/380001/ui/native_theme/native_...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/239093007/diff/380001/ui/native_theme/native_...
ui/native_theme/native_theme.h:25: class NATIVE_THEME_EXPORT NativeThemeObserver
{
nit: move to own file.

https://codereview.chromium.org/239093007/diff/380001/ui/native_theme/native_...
ui/native_theme/native_theme.h:27: virtual ~NativeThemeObserver();
make protected (since not owned).

https://codereview.chromium.org/239093007/diff/380001/ui/native_theme/native_...
ui/native_theme/native_theme.h:31: virtual void
OnNativeThemeUpdate(ui::NativeTheme* observed_theme) = 0;
OnNativeThemeUpdated?

[msw, 2014-04-25 00:14:50]

https://codereview.chromium.org/239093007/diff/380001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/239093007/diff/380001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1847:
chrome::MaybeShowInvertBubbleView(this);
On 2014/04/24 22:52:01, sky wrote:
> estade's patch made it show that you'll hit this on startup. Is that OK? By
that
> I mean we may end up triggering MaybeShowInvertBubbleView before we're visible
> and during startup.

I just ran my local Chromium build with a clean profile dir and with Windows
already in the High Contrast Black theme. The bubble is shown when the window
appears window (as expected), with or without this patch applied, so this seems
okay.

Unfortunately, the first-run bubble can potentially show with and overlap the
high-contrast bubble, but this occurs with or without this patch applied. I
filed <http://crbug.com/366937> to track that problem, which should be fixed
separately.

https://codereview.chromium.org/239093007/diff/380001/ui/native_theme/native_...
File ui/native_theme/native_theme.h (right):

https://codereview.chromium.org/239093007/diff/380001/ui/native_theme/native_...
ui/native_theme/native_theme.h:25: class NATIVE_THEME_EXPORT NativeThemeObserver
{
On 2014/04/24 22:52:01, sky wrote:
> nit: move to own file.

Done.

https://codereview.chromium.org/239093007/diff/380001/ui/native_theme/native_...
ui/native_theme/native_theme.h:27: virtual ~NativeThemeObserver();
On 2014/04/24 22:52:01, sky wrote:
> make protected (since not owned).

Done.

https://codereview.chromium.org/239093007/diff/380001/ui/native_theme/native_...
ui/native_theme/native_theme.h:31: virtual void
OnNativeThemeUpdate(ui::NativeTheme* observed_theme) = 0;
On 2014/04/24 22:52:01, sky wrote:
> OnNativeThemeUpdated?

Done.

[msw, 2014-04-25 01:12:21]

Yeah, Evan's r266047 broke this patch; give me a moment...

[msw, 2014-04-25 01:23:57]

Okay please take a look at the latest patch set; thanks.

[sky, 2014-04-25 15:32:07]

LGTM - I'm going to morph the bug you filed into a more general one.

https://codereview.chromium.org/239093007/diff/500001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/239093007/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1839: if (!initialized_)
This is subtle enough to warrant a comment.

[msw, 2014-04-25 15:45:30]

https://codereview.chromium.org/239093007/diff/500001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/239093007/diff/500001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:1839: if (!initialized_)
On 2014/04/25 15:32:08, sky wrote:
> This is subtle enough to warrant a comment.

Done.

[msw, 2014-04-25 15:45:44]

The CQ bit was checked by msw@chromium.org

[msw, 2014-04-25 16:10:18]

The CQ bit was checked by msw@chromium.org

[msw, 2014-04-25 16:24:38]

The CQ bit was unchecked by msw@chromium.org

[msw, 2014-04-25 16:24:58]

The CQ bit was checked by msw@chromium.org

[msw, 2014-04-25 20:21:53]

Committed patchset #16 manually as r266232 (presubmit successful).