Rejigger ThemeService: move exposure of ThemeProvider interface to a

chrome/browser/themes/theme_service.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 CHROME_BROWSER_THEMES_THEME_SERVICE_H_
 #define CHROME_BROWSER_THEMES_THEME_SERVICE_H_
 
 #include <map>
 #include <set>
 #include <string>
 #include <utility>
 
 #include "base/compiler_specific.h"
+#include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "ui/base/theme_provider.h"
 
 class CustomThemeSupplier;
@@ skipping 27 matching lines @@
 
 #ifdef __OBJC__
 @class NSString;
 // Sent whenever the browser theme changes.  Object => NSValue wrapping the
 // ThemeService that changed.
 extern "C" NSString* const kBrowserThemeDidChangeNotification;
 #endif  // __OBJC__
 
 class ThemeService : public base::NonThreadSafe,
                      public content::NotificationObserver,
-                     public KeyedService,
-                     public ui::ThemeProvider {
+                     public KeyedService {
  public:
   // Public constants used in ThemeService and its subclasses:
   static const char* kDefaultThemeID;
 
   ThemeService();
   ~ThemeService() override;
 
   virtual void Init(Profile* profile);
 
-  // Returns a cross platform image for an id.
-  //
-  // TODO(erg): Make this part of the ui::ThemeProvider and the main way to get
-  // theme properties out of the theme provider since it's cross platform.
-  virtual gfx::Image GetImageNamed(int id) const;
-
-  // Overridden from ui::ThemeProvider:
-  bool UsingSystemTheme() const override;
-  gfx::ImageSkia* GetImageSkiaNamed(int id) const override;
-  SkColor GetColor(int id) const override;
-  int GetDisplayProperty(int id) const override;
-  bool ShouldUseNativeFrame() const override;
-  bool HasCustomImage(int id) const override;
-  base::RefCountedMemory* GetRawData(int id, ui::ScaleFactor scale_factor)
-      const override;
-#if defined(OS_MACOSX)
-  NSImage* GetNSImageNamed(int id) const override;
-  NSColor* GetNSImageColorNamed(int id) const override;
-  NSColor* GetNSColor(int id) const override;
-  NSColor* GetNSColorTint(int id) const override;
-  NSGradient* GetNSGradient(int id) const override;
-#endif
-
   // KeyedService:
   void Shutdown() override;
 
   // Overridden from content::NotificationObserver:
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
   // Set the current theme to the theme defined in |extension|.
   // |extension| must already be added to this profile's
   // ExtensionService.
   virtual void SetTheme(const extensions::Extension* extension);
 
   // Reset the theme to default.
   virtual void UseDefaultTheme();
 
   // Set the current theme to the system theme. On some platforms, the system
   // theme is the default theme.
   virtual void UseSystemTheme();
 
   // Returns true if the default theme and system theme are not the same on
   // this platform.
   virtual bool IsSystemThemeDistinctFromDefaultTheme() const;
 
   // Whether we're using the chrome default theme. Virtual so linux can check
   // if we're using the GTK theme.
   virtual bool UsingDefaultTheme() const;
 
+  // Also overridden for GTK.

[pkotwicz, 2015/12/04 23:16:49]

How about: "Whether we are using the system theme. On GTK, the system theme is
the GTK theme, not the "Classic" theme."

[Evan Stade, 2015/12/09 00:57:11]

Done.

+  virtual bool UsingSystemTheme() const;
+
   // Gets the id of the last installed theme. (The theme may have been further
   // locally customized.)
   virtual std::string GetThemeID() const;
 
   // This class needs to keep track of the number of theme infobars so that we
   // clean up unused themes.
   void OnInfobarDisplayed();
 
   // Decrements the number of theme infobars. If the last infobar has been
   // destroyed, uninstalls all themes that aren't the currently selected.
   void OnInfobarDestroyed();
 
   // Uninstall theme extensions which are no longer in use. |ignore_infobars| is
   // whether unused themes should be removed despite a theme infobar being
   // visible.
   void RemoveUnusedThemes(bool ignore_infobars);
 
   // Returns the syncable service for syncing theme. The returned service is
   // owned by |this| object.
   virtual ThemeSyncableService* GetThemeSyncableService() const;
 
-  // Save the images to be written to disk, mapping file path to id.
-  typedef std::map<base::FilePath, int> ImagesDiskCache;
+  static const ui::ThemeProvider& GetThemeProviderForProfile(Profile* profile);

[pkotwicz, 2015/12/04 23:16:50]

Is returning a const reference from a static method a new pattern?
Returning a const star seems much more prevalent in the code base.

[Evan Stade, 2015/12/09 00:57:11]

a new pattern? No, it has existed for a while. The only difference is that a
pointer can be null and a reference implies it will always be non-null. It's
true there are many places in the codebase where we are using const* but should
probably use const&, and places where we check null-ness when we don't need to,
or don't when we do. I don't think there are any places where we return const&
when we should return const* instead (it's hard to make the mistake in that
direction).

 
  protected:
   // Set a custom default theme instead of the normal default theme.
   virtual void SetCustomDefaultTheme(
       scoped_refptr<CustomThemeSupplier> theme_supplier);
 
   // Returns true if the ThemeService should use the system theme on startup.
   virtual bool ShouldInitWithSystemTheme() const;
 
   // Get the specified tint - |id| is one of the TINT_* enum values.
@@ skipping 26 matching lines @@
   }
 
   // True if the theme service is ready to be used.
   // TODO(pkotwicz): Add DCHECKS to the theme service's getters once
   // ThemeSource no longer uses the ThemeService when it is not ready.
   bool ready_;
 
  private:
   friend class theme_service_internal::ThemeServiceTest;
 
+  // This class serves to isolate the ThemeProvider methods that are implemented
+  // by ThemeService, and handles modifications based on whether the profile is
+  // incognito (otr) or normal.

[pkotwicz, 2015/12/04 23:16:50]

How about: "This class implements ui::ThemeProvider on behalf of ThemeService.
It also converts requested property ids to "off the record" (otr) property ids
for incognito profiles."

[Evan Stade, 2015/12/09 00:57:11]

adjusted

+  class BrowserThemeProvider : public ui::ThemeProvider {
+   public:
+    BrowserThemeProvider(const ThemeService& theme_service, bool otr);
+    ~BrowserThemeProvider() override;
+
+    // Overridden from ui::ThemeProvider:
+    gfx::ImageSkia* GetImageSkiaNamed(int id) const override;
+    SkColor GetColor(int id) const override;
+    int GetDisplayProperty(int id) const override;
+    bool ShouldUseNativeFrame() const override;
+    bool HasCustomImage(int id) const override;
+    base::RefCountedMemory* GetRawData(int id, ui::ScaleFactor scale_factor)
+        const override;
+#if defined(OS_MACOSX)
+    bool UsingSystemTheme() const override;
+    NSImage* GetNSImageNamed(int id) const override;
+    NSColor* GetNSImageColorNamed(int id) const override;
+    NSColor* GetNSColor(int id) const override;
+    NSColor* GetNSColorTint(int id) const override;
+    NSGradient* GetNSGradient(int id) const override;
+#endif
+
+   private:
+    const ThemeService& theme_service_;

[pkotwicz, 2015/12/04 23:16:49]

Nit: Make this a const pointer (const ThemeService*)

[Evan Stade, 2015/12/09 00:57:11]

why? It should never be null and this makes that clear.

+    bool otr_;

[pkotwicz, 2015/12/04 23:16:50]

Might as well name this variable |off_the_record_|.

[Evan Stade, 2015/12/09 00:57:11]

Done.

+
+    DISALLOW_COPY_AND_ASSIGN(BrowserThemeProvider);
+  };
+  friend class BrowserThemeProvider;
+
+  const ui::ThemeProvider& GetOrCreateThemeProviderForProfile(Profile* profile);
+
+  // These methods provide the implementation for ui::ThemeProvider (exposed
+  // via BrowserThemeProvider).
+  gfx::ImageSkia* GetImageSkiaNamed(int id) const;
+  SkColor GetColor(int id) const;
+  int GetDisplayProperty(int id) const;
+  bool ShouldUseNativeFrame() const;
+  bool HasCustomImage(int id) const;
+  base::RefCountedMemory* GetRawData(int id,
+                                     ui::ScaleFactor scale_factor) const;
+#if defined(OS_MACOSX)
+  bool UsingSystemTheme() const;

[pkotwicz, 2015/12/04 23:16:50]

UsingSystemTheme() should not be present twice in ThemeService.

[Evan Stade, 2015/12/09 00:57:11]

Done.

+  NSImage* GetNSImageNamed(int id) const;
+  NSColor* GetNSImageColorNamed(int id) const;
+  NSColor* GetNSColor(int id) const;
+  NSColor* GetNSColorTint(int id) const;
+  NSGradient* GetNSGradient(int id) const;
+#endif
+
+  // Returns a cross platform image for an id.
+  //
+  // TODO(erg): Make this part of the ui::ThemeProvider and the main way to get
+  // theme properties out of the theme provider since it's cross platform.
+  virtual gfx::Image GetImageNamed(int id) const;
+
   // Called when the extension service is ready.
   void OnExtensionServiceReady();
 
   // Migrate the theme to the new theme pack schema by recreating the data pack
   // from the extension.
   void MigrateTheme();
 
   // Replaces the current theme supplier with a new one and calls
   // StopUsingTheme() or StartUsingTheme() as appropriate.
   void SwapThemeSupplier(scoped_refptr<CustomThemeSupplier> theme_supplier);
@@ skipping 47 matching lines @@
 
   content::NotificationRegistrar registrar_;
 
   scoped_ptr<ThemeSyncableService> theme_syncable_service_;
 
 #if defined(ENABLE_EXTENSIONS)
   class ThemeObserver;
   scoped_ptr<ThemeObserver> theme_observer_;
 #endif
 
+  BrowserThemeProvider original_theme_provider_;
+  BrowserThemeProvider otr_theme_provider_;
+
   base::WeakPtrFactory<ThemeService> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ThemeService);
 };
 
 #endif  // CHROME_BROWSER_THEMES_THEME_SERVICE_H_