Update Windows UI on system color changes.

ui/native_theme/native_theme.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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;
 
 namespace gfx {
 class Rect;
 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
 // dialog box backgrounds, etc., which are obtained from the system theme where
 // possible.
 //
 // The supported control types are listed in the Part enum.  These parts can be
 // in any state given by the State enum, where the actual definition of the
 // state is part-specific. The supported colors are listed in the ColorId enum.
 //
 // Some parts require more information than simply the state in order to be
 // drawn correctly, and this information is given to the Paint() method via the
 // ExtraParams union.  Each part that requires more information has its own
 // field in the union.
 //
 // 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 {
     kCheckbox,
     kInnerSpinButton,
     kMenuList,
     kMenuCheck,
     kMenuCheckBackground,
     kMenuPopupArrow,
     kMenuPopupBackground,
@@ skipping 254 matching lines @@
   // Return a color from the system theme.
   virtual SkColor GetSystemColor(ColorId color_id) const = 0;
 
   // Returns a shared instance of the native theme.
   // The returned object should not be deleted by the caller.  This function
   // is not thread safe and should only be called from the UI thread.
   // Each port of NativeTheme should provide its own implementation of this
   // 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);
 };
 
 }  // namespace ui
 
 #endif  // UI_NATIVE_THEME_NATIVE_THEME_H_