Use default browser theming for hosted app windows

PWA windows are intended to appear stand alone from the main browser so they should not adopt the main browser's theme colors. This CL provides a default theme getter to be used by hosted app (and PWA) windows. This CL also removes the quick fix for this issue done by https://chromium-review.googlesource.com/c/chromium/src/+/1275466. Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=363145&signed_aid=TMzuMVlMKC2dm6ppdYp9PQ==&inline=1 After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=363146&signed_aid=Og46P66KeV7iP-mnBHO3cA==&inline=1 Bug: 891560 Change-Id: I2d94bff9f72bfca59fcf7d6cb940f42e4a25cc70 Reviewed-on: https://chromium-review.googlesource.com/c/1272896 Commit-Queue: Alan Cutter <alancutter@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#600643}
hylschwy · Oct 18, 2018 · fa6292a · fa6292a
1 parent 506ce45
commit fa6292a
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 28 deletions.
diff --git a/chrome/browser/themes/theme_service.cc b/chrome/browser/themes/theme_service.cc
@@ -144,39 +144,80 @@ bool IsColorGrayscale(SkColor color) {
 
 // ThemeService::BrowserThemeProvider -----------------------------------------
 
+// Creates a temporary scope where all |theme_service_| property getters return
+// uncustomized default values if |theme_provider_.use_default_| is enabled.
+class ThemeService::BrowserThemeProvider::DefaultScope {
+ public:
+  explicit DefaultScope(const BrowserThemeProvider& theme_provider)
+      : theme_provider_(theme_provider) {
+    if (theme_provider_.use_default_) {
+      // Mutations to |theme_provider_| are undone in the destructor making it
+      // effectively const over the entire duration of this object's scope.
+      theme_supplier_ =
+          std::move(const_cast<ThemeService&>(theme_provider_.theme_service_)
+                        .theme_supplier_);
+      DCHECK(!theme_provider_.theme_service_.theme_supplier_);
+    }
+  }
+
+  ~DefaultScope() {
+    if (theme_provider_.use_default_) {
+      const_cast<ThemeService&>(theme_provider_.theme_service_)
+          .theme_supplier_ = std::move(theme_supplier_);
+    }
+    DCHECK(!theme_supplier_);
+  }
+
+ private:
+  const BrowserThemeProvider& theme_provider_;
+  scoped_refptr<CustomThemeSupplier> theme_supplier_;
+
+  DISALLOW_COPY_AND_ASSIGN(DefaultScope);
+};
+
 ThemeService::BrowserThemeProvider::BrowserThemeProvider(
     const ThemeService& theme_service,
-    bool incognito)
-    : theme_service_(theme_service), incognito_(incognito) {}
+    bool incognito,
+    bool use_default)
+    : theme_service_(theme_service),
+      incognito_(incognito),
+      use_default_(use_default) {}
 
 ThemeService::BrowserThemeProvider::~BrowserThemeProvider() {}
 
 gfx::ImageSkia* ThemeService::BrowserThemeProvider::GetImageSkiaNamed(
     int id) const {
+  DefaultScope scope(*this);
   return theme_service_.GetImageSkiaNamed(id, incognito_);
 }
 
 SkColor ThemeService::BrowserThemeProvider::GetColor(int id) const {
+  DefaultScope scope(*this);
   return theme_service_.GetColor(id, incognito_);
 }
 
 color_utils::HSL ThemeService::BrowserThemeProvider::GetTint(int id) const {
+  DefaultScope scope(*this);
   return theme_service_.GetTint(id, incognito_);
 }
 
 int ThemeService::BrowserThemeProvider::GetDisplayProperty(int id) const {
+  DefaultScope scope(*this);
   return theme_service_.GetDisplayProperty(id);
 }
 
 bool ThemeService::BrowserThemeProvider::ShouldUseNativeFrame() const {
+  DefaultScope scope(*this);
   return theme_service_.ShouldUseNativeFrame();
 }
 
 bool ThemeService::BrowserThemeProvider::HasCustomImage(int id) const {
+  DefaultScope scope(*this);
   return theme_service_.HasCustomImage(id);
 }
 
 bool ThemeService::BrowserThemeProvider::HasCustomColor(int id) const {
+  DefaultScope scope(*this);
   bool has_custom_color = false;
   theme_service_.GetColor(id, incognito_, &has_custom_color);
   return has_custom_color;
@@ -185,6 +226,7 @@ bool ThemeService::BrowserThemeProvider::HasCustomColor(int id) const {
 base::RefCountedMemory* ThemeService::BrowserThemeProvider::GetRawData(
     int id,
     ui::ScaleFactor scale_factor) const {
+  DefaultScope scope(*this);
   return theme_service_.GetRawData(id, scale_factor);
 }
 
@@ -271,8 +313,9 @@ ThemeService::ThemeService()
       profile_(nullptr),
       installed_pending_load_id_(kDefaultThemeID),
       number_of_infobars_(0),
-      original_theme_provider_(*this, false),
-      incognito_theme_provider_(*this, true),
+      original_theme_provider_(*this, false, false),
+      incognito_theme_provider_(*this, true, false),
+      default_theme_provider_(*this, false, true),
       weak_ptr_factory_(this) {}
 
 ThemeService::~ThemeService() {
@@ -440,6 +483,14 @@ const ui::ThemeProvider& ThemeService::GetThemeProviderForProfile(
                    : service->original_theme_provider_;
 }
 
+// static
+const ui::ThemeProvider& ThemeService::GetDefaultThemeProviderForProfile(
+    Profile* profile) {
+  DCHECK_NE(profile->GetProfileType(), Profile::INCOGNITO_PROFILE)
+      << "Incognito default theme access not implemented, add if needed.";
+  return ThemeServiceFactory::GetForProfile(profile)->default_theme_provider_;
+}
+
 void ThemeService::SetCustomDefaultTheme(
     scoped_refptr<CustomThemeSupplier> theme_supplier) {
   ClearAllThemeData();

diff --git a/chrome/browser/themes/theme_service.h b/chrome/browser/themes/theme_service.h
@@ -125,6 +125,11 @@ class ThemeService : public content::NotificationObserver, public KeyedService {
   // the same ThemeService.
   static const ui::ThemeProvider& GetThemeProviderForProfile(Profile* profile);
 
+  // Gets the ThemeProvider for |profile| that represents the default colour
+  // scheme for the OS.
+  static const ui::ThemeProvider& GetDefaultThemeProviderForProfile(
+      Profile* profile);
+
  protected:
   // Set a custom default theme instead of the normal default theme.
   virtual void SetCustomDefaultTheme(
@@ -176,7 +181,9 @@ class ThemeService : public content::NotificationObserver, public KeyedService {
   // track of the incognito state of the calling code.
   class BrowserThemeProvider : public ui::ThemeProvider {
    public:
-    BrowserThemeProvider(const ThemeService& theme_service, bool incognito);
+    BrowserThemeProvider(const ThemeService& theme_service,
+                         bool incognito,
+                         bool use_default);
     ~BrowserThemeProvider() override;
 
     // Overridden from ui::ThemeProvider:
@@ -191,8 +198,12 @@ class ThemeService : public content::NotificationObserver, public KeyedService {
         const override;
 
    private:
+    class DefaultScope;
+    friend class DefaultScope;
+
     const ThemeService& theme_service_;
     bool incognito_;
+    bool use_default_;
 
     DISALLOW_COPY_AND_ASSIGN(BrowserThemeProvider);
   };
@@ -300,6 +311,7 @@ class ThemeService : public content::NotificationObserver, public KeyedService {
 
   BrowserThemeProvider original_theme_provider_;
   BrowserThemeProvider incognito_theme_provider_;
+  BrowserThemeProvider default_theme_provider_;
 
   SEQUENCE_CHECKER(sequence_checker_);
 

diff --git a/chrome/browser/themes/theme_service_unittest.cc b/chrome/browser/themes/theme_service_unittest.cc
@@ -295,6 +295,32 @@ TEST_F(ThemeServiceTest, IncognitoTest) {
 #endif
 }
 
+TEST_F(ThemeServiceTest, GetDefaultThemeProviderForProfile) {
+  ThemeService* theme_service =
+      ThemeServiceFactory::GetForProfile(profile_.get());
+  theme_service->UseDefaultTheme();
+  // Let the ThemeService uninstall unused themes.
+  base::RunLoop().RunUntilIdle();
+
+  SkColor default_toolbar_color =
+      ThemeService::GetThemeProviderForProfile(profile_.get())
+          .GetColor(ThemeProperties::COLOR_TOOLBAR);
+
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  LoadUnpackedThemeAt(temp_dir.GetPath());
+
+  // Should get a new color after installing a theme.
+  EXPECT_NE(ThemeService::GetThemeProviderForProfile(profile_.get())
+                .GetColor(ThemeProperties::COLOR_TOOLBAR),
+            default_toolbar_color);
+
+  // Should get the same color when requesting a default color.
+  EXPECT_EQ(ThemeService::GetDefaultThemeProviderForProfile(profile_.get())
+                .GetColor(ThemeProperties::COLOR_TOOLBAR),
+            default_toolbar_color);
+}
+
 namespace {
 
 // NotificationObserver which emulates an infobar getting destroyed when the

diff --git a/chrome/browser/ui/views/frame/browser_frame.cc b/chrome/browser/ui/views/frame/browser_frame.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_window_state.h"
+#include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
 #include "chrome/browser/ui/views/frame/browser_non_client_frame_view.h"
 #include "chrome/browser/ui/views/frame/browser_root_view.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
@@ -169,8 +170,14 @@ bool BrowserFrame::GetAccelerator(int command_id,
 }
 
 const ui::ThemeProvider* BrowserFrame::GetThemeProvider() const {
-  return &ThemeService::GetThemeProviderForProfile(
-      browser_view_->browser()->profile());
+  Browser* browser = browser_view_->browser();
+  Profile* profile = browser->profile();
+  // Hosted apps are meant to appear stand alone from the main browser so they
+  // do not use the normal browser's configured theme.
+  using HostedAppController = extensions::HostedAppBrowserController;
+  return HostedAppController::IsForExperimentalHostedAppBrowser(browser)
+             ? &ThemeService::GetDefaultThemeProviderForProfile(profile)
+             : &ThemeService::GetThemeProviderForProfile(profile);
 }
 
 const ui::NativeTheme* BrowserFrame::GetNativeTheme() const {

diff --git a/chrome/browser/ui/views/frame/windows_10_caption_button.cc b/chrome/browser/ui/views/frame/windows_10_caption_button.cc
@@ -5,7 +5,6 @@
 #include "chrome/browser/ui/views/frame/windows_10_caption_button.h"
 
 #include "chrome/browser/themes/theme_properties.h"
-#include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
 #include "chrome/browser/ui/frame/window_frame_util.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/glass_browser_frame_view.h"
@@ -52,9 +51,8 @@ SkColor Windows10CaptionButton::GetBaseColor() const {
       (frame_view_->ShouldPaintAsActive()
            ? ThemeProperties::COLOR_WINDOW_CONTROL_BUTTON_BACKGROUND_ACTIVE
            : ThemeProperties::COLOR_WINDOW_CONTROL_BUTTON_BACKGROUND_INACTIVE);
-  const ui::ThemeProvider* theme_provider = GetFrameThemeProvider();
+  const ui::ThemeProvider* theme_provider = GetThemeProvider();
   const bool has_custom_color =
-      theme_provider &&
       theme_provider->HasCustomColor(control_button_bg_color_id);
   const SkColor bg_color =
       (has_custom_color ? theme_provider->GetColor(control_button_bg_color_id)
@@ -66,11 +64,9 @@ SkColor Windows10CaptionButton::GetBaseColor() const {
 void Windows10CaptionButton::OnPaintBackground(gfx::Canvas* canvas) {
   // Paint the background of the button (the semi-transparent rectangle that
   // appears when you hover or press the button).
-  const ui::ThemeProvider* theme_provider = GetFrameThemeProvider();
+  const ui::ThemeProvider* theme_provider = GetThemeProvider();
   const SkColor bg_color =
-      theme_provider
-          ? theme_provider->GetColor(ThemeProperties::COLOR_BUTTON_BACKGROUND)
-          : SK_ColorTRANSPARENT;
+      theme_provider->GetColor(ThemeProperties::COLOR_BUTTON_BACKGROUND);
   const SkAlpha theme_alpha = SkColorGetA(bg_color);
   gfx::Rect bounds = GetContentsBounds();
   bounds.Inset(GetBetweenButtonSpacing(), 0, 0, 0);
@@ -83,8 +79,7 @@ void Windows10CaptionButton::OnPaintBackground(gfx::Canvas* canvas) {
                         CalculateWindows10GlassCaptionButtonBackgroundAlpha(
                             theme_alpha)));
   }
-  if (theme_provider &&
-      theme_provider->HasCustomImage(IDR_THEME_WINDOW_CONTROL_BACKGROUND)) {
+  if (theme_provider->HasCustomImage(IDR_THEME_WINDOW_CONTROL_BACKGROUND)) {
     // Figure out what portion of the background image to display
     const int button_display_order = GetButtonDisplayOrderIndex();
     const int base_button_width =
@@ -266,11 +261,3 @@ void Windows10CaptionButton::PaintSymbol(gfx::Canvas* canvas) {
       return;
   }
 }
-
-const ui::ThemeProvider* Windows10CaptionButton::GetFrameThemeProvider() const {
-  // TODO(https://crbug.com/891560): Move this check up into
-  // BrowserFrame::GetThemeProvider() and have it return the default theme.
-  if (frame_view_->browser_view()->IsBrowserTypeHostedApp())
-    return nullptr;
-  return GetThemeProvider();
-}
diff --git a/chrome/browser/ui/views/frame/windows_10_caption_button.h b/chrome/browser/ui/views/frame/windows_10_caption_button.h
@@ -40,10 +40,6 @@ class Windows10CaptionButton : public views::Button {
   // Paints the minimize/maximize/restore/close icon for the button.
   void PaintSymbol(gfx::Canvas* canvas);
 
-  // Returns the appropriate ThemeProvider to use for the frame if any, may
-  // return nullptr.
-  const ui::ThemeProvider* GetFrameThemeProvider() const;
-
   GlassBrowserFrameView* frame_view_;
   ViewID button_type_;