Update Windows UI on system color changes.

ui/native_theme/native_theme.h

Index: ui/native_theme/native_theme.h
diff --git a/ui/native_theme/native_theme.h b/ui/native_theme/native_theme.h
index 67af252d7bd849273056b306ab233ceddf78bcbb..f5fd796c11285e2d6805d62fe2182cdbf2f06c7f 100644
--- a/ui/native_theme/native_theme.h
+++ b/ui/native_theme/native_theme.h
@@ -5,8 +5,10 @@
 #ifndef UI_NATIVE_THEME_NATIVE_THEME_H_
 #define UI_NATIVE_THEME_NATIVE_THEME_H_
 
+#include "base/observer_list.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/native_widget_types.h"
+#include "ui/gfx/sys_color_change_listener.h"
 #include "ui/native_theme/native_theme_export.h"
 
 class SkCanvas;
@@ -18,6 +20,15 @@ class Size;
 
 namespace ui {
 
+// Observers which are notified when the native theme changes.
+class NATIVE_THEME_EXPORT NativeThemeObserver {

[Evan Stade, 2014/04/22 00:32:32]

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:45]

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]

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);
}

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

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.

[Evan Stade, 2014/04/22 21:09:18]

Currently yes, but making this change provides a mechanism for other bits of
code to inform interested parties of changes.
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 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)

[msw, 2014/04/23 00:34:04]

Agreed, but I don't want to unnecessarily expand the scope of this CL.
 
That's fine.
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).

+ public:
+  virtual ~NativeThemeObserver();
+
+  // Called whenever the underlying platform's native theme changes.
+  virtual void OnNativeThemeChange() = 0;
+};
+
 // This class supports drawing UI controls (like buttons, text fields, lists,
 // comboboxes, etc) that look like the native UI controls of the underlying
 // platform, such as Windows or Linux. It also supplies default colors for
@@ -35,7 +46,7 @@ namespace ui {
 //
 // NativeTheme also supports getting the default size of a given part with
 // the GetPartSize() method.
-class NATIVE_THEME_EXPORT NativeTheme {
+class NATIVE_THEME_EXPORT NativeTheme : public gfx::SysColorChangeListener {
  public:
   // The part to be painted / sized.
   enum Part {
@@ -310,14 +321,32 @@ class NATIVE_THEME_EXPORT NativeTheme {
   // function, returning the port's subclass.
   static NativeTheme* instance();
 
+  // Add or remove observers to be notified when the native theme changes.
+  void AddObserver(NativeThemeObserver* observer);
+  void RemoveObserver(NativeThemeObserver* observer);
+
+  // Notify observers of native theme changes.
+  void OnNativeThemeChange();

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

I think this should be protected

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

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

[Evan Stade, 2014/04/22 21:09:18]

You can expose it on NativeThemeGtk2 but not here.

[msw, 2014/04/23 00:34:04]

Done (added NativeThemeGtk2::NotifyNativeThemeObservers).

+
  protected:
   NativeTheme();
   virtual ~NativeTheme();
 
+  // gfx::SysColorChangeListener implementation:
+  virtual void OnSysColorChange() OVERRIDE;
+
   unsigned int thumb_inactive_color_;
   unsigned int thumb_active_color_;
   unsigned int track_color_;
 
+ private:
+  // Observers to notify when the theme state changes.
+  ObserverList<NativeThemeObserver> native_theme_observers_;
+
+  // The system color change listener.
+  gfx::ScopedSysColorChangeListener color_change_listener_;
+
+
   DISALLOW_COPY_AND_ASSIGN(NativeTheme);
 };