Misc. trivial cleanup in advance of fixing bug 621004.

This is mostly to shorten code I was touching anyway or reduce diffs in the
later patch.

There is one behavior change here; the code to ignore custom theme NTP
background colors in incognito was also bypassing the GTK theme, which doesn't
appear to have been the intent.  Limited this case to just custom themes.

The custom tab bar code changes from NativeTheme::GetInstanceForNativeUi() to
GetNativeTheme(), but since this is happening in the constructor, those will
always return the same thing.

Bug: none
Change-Id: Ie9d490669c9bff474484294a630f0f6b37d25d1c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1961012Reviewed-by: Evan Stade <estade@chromium.org>
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#724074}

chrome/browser/themes/theme_service.cc

@@ -198,18 +198,18 @@ bool ThemeService::BrowserThemeProvider::HasCustomImage(int id) const {
 
 bool ThemeService::BrowserThemeProvider::HasCustomColor(int id) const {
   DefaultScope scope(*this);
-  bool has_custom_color = false;
 
   // COLOR_TOOLBAR_BUTTON_ICON has custom value if it is explicitly specified or
   // calclated from non {-1, -1, -1} tint (means "no change"). Note that, tint
   // can have a value other than {-1, -1, -1} even if it is not explicitly
   // specified (e.g incognito and dark mode).
   if (id == TP::COLOR_TOOLBAR_BUTTON_ICON) {
-    theme_service_.GetColor(id, incognito_, &has_custom_color);
     color_utils::HSL hsl = theme_service_.GetTint(TP::TINT_BUTTONS, incognito_);
-    return has_custom_color || (hsl.h != -1 || hsl.s != -1 || hsl.l != -1);
+    if (hsl.h != -1 || hsl.s != -1 || hsl.l != -1)
+      return true;
   }
 
+  bool has_custom_color = false;
   theme_service_.GetColor(id, incognito_, &has_custom_color);
   return has_custom_color;
 }
@@ -475,10 +475,6 @@ bool ThemeService::UsingAutogeneratedTheme() const {
   return autogenerated;
 }
 
-bool ThemeService::ForceLightDefaultColors() const {
-  return UsingExtensionTheme() || UsingAutogeneratedTheme();
-}
-
 std::string ThemeService::GetThemeID() const {
   return profile_->GetPrefs()->GetString(prefs::kCurrentThemeID);
 }
@@ -662,7 +658,9 @@ SkColor ThemeService::GetDefaultColor(int id, bool incognito) const {
     }
   }
 
-  return TP::GetDefaultColor(id, incognito && !ForceLightDefaultColors());
+  // Incognito colors are ignored for custom themes so they apply atop a
+  // predictable state.
+  return TP::GetDefaultColor(id, incognito && !UsingCustomTheme());
 }
 
 color_utils::HSL ThemeService::GetTint(int id, bool incognito) const {
@@ -672,7 +670,9 @@ color_utils::HSL ThemeService::GetTint(int id, bool incognito) const {
   if (theme_supplier_ && theme_supplier_->GetTint(id, &hsl))
     return hsl;
 
-  return TP::GetDefaultTint(id, incognito && !ForceLightDefaultColors());
+  // Incognito tints are ignored for custom themes so they apply atop a
+  // predictable state.
+  return TP::GetDefaultTint(id, incognito && !UsingCustomTheme());
 }
 
 void ThemeService::ClearAllThemeData() {
@@ -825,24 +825,19 @@ SkColor ThemeService::GetColor(int id,
   if (has_custom_color)
     *has_custom_color = false;
 
-  // The incognito NTP always uses the default background color, unless there is
-  // a custom NTP background image. See also https://crbug.com/21798#c114.
-  if (id == TP::COLOR_NTP_BACKGROUND && incognito &&
-      !HasCustomImage(IDR_THEME_NTP_BACKGROUND)) {
-    return TP::GetDefaultColor(id, incognito);
-  }
-
   const base::Optional<SkColor> omnibox_color =
       GetOmniboxColor(id, incognito, has_custom_color);
   if (omnibox_color.has_value())
     return omnibox_color.value();
 
-  SkColor color;
-  const int theme_supplier_id = incognito ? GetIncognitoId(id) : id;
-  if (theme_supplier_ && theme_supplier_->GetColor(theme_supplier_id, &color)) {
-    if (has_custom_color)
-      *has_custom_color = true;
-    return color;
+  if (theme_supplier_ && !ShouldIgnoreThemeSupplier(id, incognito)) {
+    SkColor color;
+    const int theme_supplier_id = incognito ? GetIncognitoId(id) : id;
+    if (theme_supplier_->GetColor(theme_supplier_id, &color)) {
+      if (has_custom_color)
+        *has_custom_color = true;
+      return color;
+    }
   }
 
   return GetDefaultColor(id, incognito);
@@ -1093,6 +1088,17 @@ bool ThemeService::DisableExtension(const std::string& extension_id) {
   return false;
 }
 
+bool ThemeService::UsingCustomTheme() const {
+  return UsingExtensionTheme() || UsingAutogeneratedTheme();
+}
+
+bool ThemeService::ShouldIgnoreThemeSupplier(int id, bool incognito) const {
+  // The incognito NTP ignores custom theme background colors unless the theme
+  // also sets a custom background image.
+  return UsingCustomTheme() && incognito && (id == TP::COLOR_NTP_BACKGROUND) &&
+         !HasCustomImage(IDR_THEME_NTP_BACKGROUND);
+}
+
 base::Optional<SkColor> ThemeService::GetOmniboxColor(
     int id,
     bool incognito,

chrome/browser/themes/theme_service.h

@@ -128,10 +128,6 @@ class ThemeService : public content::NotificationObserver,
   // Whether we are using an autogenerated theme.
   virtual bool UsingAutogeneratedTheme() const;
 
-  // Whether to force the use of light colors/tints by default, i.e. ignore
-  // incognito state.  This is true for extension and autogenerated themes.
-  bool ForceLightDefaultColors() const;
-
   // Gets the id of the last installed theme. (The theme may have been further
   // locally customized.)
   virtual std::string GetThemeID() const;
@@ -320,6 +316,14 @@ class ThemeService : public content::NotificationObserver,
 
   bool DisableExtension(const std::string& extension_id);
 
+  // Whether what users would think of as a "custom theme" (that is, an
+  // extension or autogenerated theme) is in use.
+  bool UsingCustomTheme() const;
+
+  // Whether the color from the theme supplier (if any) should be ignored for
+  // the given |id| and |incognito| state.
+  bool ShouldIgnoreThemeSupplier(int id, bool incognito) const;
+
   // Given a theme property ID |id|, returns the corresponding omnibox color
   // overridden by the system theme.  Returns base::nullopt if the color is not
   // overridden, or if |id| does not correspond to an omnibox color.

chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc

@@ -53,19 +53,6 @@
 
 namespace {
 
-constexpr SkColor kDefaultCustomTabBarBackgroundColor = SK_ColorWHITE;
-
-// The frame color is different on ChromeOS and other platforms because Ash
-// specifies its own default frame color, which is not exposed through
-// BrowserNonClientFrameView::GetFrameColor.
-SkColor GetDefaultFrameColor() {
-#if defined(OS_CHROMEOS)
-  return ash::kDefaultFrameColor;
-#else
-  return ThemeProperties::GetDefaultColor(ThemeProperties::COLOR_FRAME, false);
-#endif
-}
-
 std::unique_ptr<views::ImageButton> CreateCloseButton(
     views::ButtonListener* listener,
     SkColor color) {
@@ -186,15 +173,18 @@ CustomTabBarView::CustomTabBarView(BrowserView* browser_view,
   base::Optional<SkColor> optional_theme_color =
       browser_->app_controller()->GetThemeColor();
 
-  // If we have a theme color, use that, otherwise fall back to the default
-  // frame color.
-  title_bar_color_ = optional_theme_color.value_or(GetDefaultFrameColor());
+  const SkColor default_frame_color =
+#if defined(OS_CHROMEOS)
+      // Ash system frames differ from ChromeOS browser frames.
+      ash::kDefaultFrameColor;
+#else
+      ThemeProperties::GetDefaultColor(ThemeProperties::COLOR_FRAME, false);
+#endif
+
+  title_bar_color_ = optional_theme_color.value_or(default_frame_color);
 
-  // Match the default frame colors if using dark colors.
-  background_color_ =
-      ui::NativeTheme::GetInstanceForNativeUi()->ShouldUseDarkColors()
-          ? GetDefaultFrameColor()
-          : kDefaultCustomTabBarBackgroundColor;
+  const bool dark_mode = GetNativeTheme()->ShouldUseDarkColors();
+  background_color_ = dark_mode ? default_frame_color : SK_ColorWHITE;
 
   SetBackground(views::CreateSolidBackground(background_color_));