Rejigger ThemeService: move exposure of ThemeProvider interface to a

chrome/browser/ui/webui/ntp/ntp_resource_cache.cc

 // 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.
 
 #include "chrome/browser/ui/webui/ntp/ntp_resource_cache.h"
 
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
@@ skipping 94 matching lines @@
 // Creates an rgb string for an SkColor, but leaves the alpha blank so that the
 // css can fill it in.
 std::string SkColorToRGBComponents(SkColor color) {
   return base::StringPrintf(
       "%d,%d,%d",
       SkColorGetR(color),
       SkColorGetG(color),
       SkColorGetB(color));
 }
 
-SkColor GetThemeColor(ui::ThemeProvider* tp, int id) {
-  SkColor color = tp->GetColor(id);
+SkColor GetThemeColor(const ui::ThemeProvider& tp, int id) {
+  SkColor color = tp.GetColor(id);
   // If web contents are being inverted because the system is in high-contrast
   // mode, any system theme colors we use must be inverted too to cancel out.
   return color_utils::IsInvertedColorScheme() ?
       color_utils::InvertColor(color) : color;
 }
 
 // Get the CSS string for the background position on the new tab page for the
 // states when the bar is attached or detached.
-std::string GetNewTabBackgroundCSS(const ui::ThemeProvider* theme_provider,
+std::string GetNewTabBackgroundCSS(const ui::ThemeProvider& theme_provider,
                                    bool bar_attached) {
   // TODO(glen): This is a quick workaround to hide the notused.png image when
   // no image is provided - we don't have time right now to figure out why
   // this is painting as white.
   // http://crbug.com/17593
-  if (!theme_provider->HasCustomImage(IDR_THEME_NTP_BACKGROUND)) {
+  if (!theme_provider.HasCustomImage(IDR_THEME_NTP_BACKGROUND)) {
     return "-64px";
   }
 
-  int alignment = theme_provider->GetDisplayProperty(
+  int alignment = theme_provider.GetDisplayProperty(
       ThemeProperties::NTP_BACKGROUND_ALIGNMENT);
 
   if (bar_attached)
     return ThemeProperties::AlignmentToString(alignment);
 
   if (alignment & ThemeProperties::ALIGN_TOP) {
     // The bar is detached, so we must offset the background by the bar size
     // if it's a top-aligned bar.
     int offset = chrome::kNTPBookmarkBarHeight;
 
     if (alignment & ThemeProperties::ALIGN_LEFT)
       return "left " + base::IntToString(-offset) + "px";
     else if (alignment & ThemeProperties::ALIGN_RIGHT)
       return "right " + base::IntToString(-offset) + "px";
     return "center " + base::IntToString(-offset) + "px";
   }
 
   return ThemeProperties::AlignmentToString(alignment);
 }
 
 // How the background image on the new tab page should be tiled (see tiling
 // masks in theme_service.h).
 std::string GetNewTabBackgroundTilingCSS(
-    const ui::ThemeProvider* theme_provider) {
-  int repeat_mode = theme_provider->GetDisplayProperty(
-      ThemeProperties::NTP_BACKGROUND_TILING);
+    const ui::ThemeProvider& theme_provider) {
+  int repeat_mode =
+      theme_provider.GetDisplayProperty(ThemeProperties::NTP_BACKGROUND_TILING);
   return ThemeProperties::TilingToString(repeat_mode);
 }
 
 }  // namespace
 
 NTPResourceCache::NTPResourceCache(Profile* profile)
     : profile_(profile), is_swipe_tracking_from_scroll_events_enabled_(false),
       should_show_apps_page_(NewTabUI::ShouldShowApps()),
       should_show_other_devices_menu_(true) {
   registrar_.Add(this, chrome::NOTIFICATION_BROWSER_THEME_CHANGED,
@@ skipping 159 matching lines @@
   }
 
   localized_strings.SetString("learnMore",
       l10n_util::GetStringUTF16(new_tab_link_ids));
   localized_strings.SetString("learnMoreLink", new_tab_link);
 
   bool bookmark_bar_attached =
       profile_->GetPrefs()->GetBoolean(bookmarks::prefs::kShowBookmarkBar);
   localized_strings.SetBoolean("bookmarkbarattached", bookmark_bar_attached);
 
-  ui::ThemeProvider* tp = ThemeServiceFactory::GetForProfile(profile_);
+  const ui::ThemeProvider& tp =
+      ThemeService::GetThemeProviderForProfile(profile_);

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

This looks like |profile_| is always the non-OTR profile. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

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

well, I suppose we might need to change that in the future, but for now there's
no incognito version of IDR_THEME_NTP_BACKGROUND so it doesn't matter.

[pkotwicz, 2015/12/09 02:08:29]

This class fetches colors as well using the ThemeProvider for the incorrect
profile. e.g. line 549

[Evan Stade, 2015/12/09 23:38:40]

are you asking me to fix that right now, or pointing out that this will prevent
the approach in this CL from ever working? There is a lot of follow up needed to
this CL, and there are many places that will need to adjust how they get colors.
I agree that L549 is one place that might change (although it technically works
as-is). Changes will be piecemeal, but will only affect MD, so e.g. normal
canary users won't see half-broken stuff.

[pkotwicz, 2015/12/10 01:02:02]

I am asking you to fix this right now or to commit to fixing this immediately
after this CL lands.

Most likely someone who changes ThemeProperties::GetDefaultColor() will assume
|otr| is correct on all OSes but Mac. This makes it almost guaranteed that if
someone modifies ThemeProperties::GetDefaultColor() that they will introduce
bugs.

If you want to fix this immediately after this CL lands:
1) Please only use the two argument version ThemeProperties::GetDefaultColor()
for COLOR_TOOLBAR
2) Add a comment that |otr| is not appropriate for deciding whether to use an
incognito color except in the very special case of COLOR_TOOLBAR.
Also please file a bug and include a bug number in the comment

[Evan Stade, 2015/12/10 01:31:17]

I don't understand the urgency. There are no special colors for OTR in pre-MD.
Why would someone besides me be adding a color that cared about OTR status?

   localized_strings.SetBoolean("hasCustomBackground",
-                               tp->HasCustomImage(IDR_THEME_NTP_BACKGROUND));
+                               tp.HasCustomImage(IDR_THEME_NTP_BACKGROUND));
 
   const std::string& app_locale = g_browser_process->GetApplicationLocale();
   webui::SetLoadTimeDataDefaults(app_locale, &localized_strings);
 
   static const base::StringPiece incognito_tab_html(
       ResourceBundle::GetSharedInstance().GetRawDataResource(
           new_tab_html_idr));
 
   std::string full_html = webui::GetI18nTemplateHtml(
       incognito_tab_html, &localized_strings);
@@ skipping 178 matching lines @@
 
   // Load the new tab page appropriate for this build.
   base::StringPiece new_tab_html(ResourceBundle::GetSharedInstance().
       GetRawDataResource(IDR_NEW_TAB_4_HTML));
   std::string full_html =
       webui::GetI18nTemplateHtml(new_tab_html, &load_time_data);
   new_tab_html_ = base::RefCountedString::TakeString(&full_html);
 }
 
 void NTPResourceCache::CreateNewTabIncognitoCSS() {
-  ui::ThemeProvider* tp = ThemeServiceFactory::GetForProfile(profile_);
-  DCHECK(tp);
+  const ui::ThemeProvider& tp =
+      ThemeService::GetThemeProviderForProfile(profile_);
 
   // Get our theme colors
   SkColor color_background =
-      tp->HasCustomImage(IDR_THEME_NTP_BACKGROUND)
+      tp.HasCustomImage(IDR_THEME_NTP_BACKGROUND)
           ? GetThemeColor(tp, ThemeProperties::COLOR_NTP_BACKGROUND)
           : SkColorSetRGB(0x32, 0x32, 0x32);
 
   // Generate the replacements.
   std::map<base::StringPiece, std::string> substitutions;
 
   // Cache-buster for background.
   substitutions["themeId"] =
       profile_->GetPrefs()->GetString(prefs::kCurrentThemeID);
 
   // Colors.
   substitutions["colorBackground"] = SkColorToRGBAString(color_background);
   substitutions["backgroundBarDetached"] = GetNewTabBackgroundCSS(tp, false);
   substitutions["backgroundBarAttached"] = GetNewTabBackgroundCSS(tp, true);
   substitutions["backgroundTiling"] = GetNewTabBackgroundTilingCSS(tp);
 
   // Get our template.
   static const base::StringPiece new_tab_theme_css(
       ResourceBundle::GetSharedInstance().GetRawDataResource(
           IDR_NEW_INCOGNITO_TAB_THEME_CSS));
 
   // Create the string from our template and the replacements.
   std::string full_css =
       ui::ReplaceTemplateExpressions(new_tab_theme_css, substitutions);
 
   new_tab_incognito_css_ = base::RefCountedString::TakeString(&full_css);
 }
 
 void NTPResourceCache::CreateNewTabCSS() {
-  ui::ThemeProvider* tp = ThemeServiceFactory::GetForProfile(profile_);
-  DCHECK(tp);
+  const ui::ThemeProvider& tp =
+      ThemeService::GetThemeProviderForProfile(profile_);
 
   // Get our theme colors
   SkColor color_background =
       GetThemeColor(tp, ThemeProperties::COLOR_NTP_BACKGROUND);
   SkColor color_text = GetThemeColor(tp, ThemeProperties::COLOR_NTP_TEXT);
   SkColor color_text_light =
       GetThemeColor(tp, ThemeProperties::COLOR_NTP_TEXT_LIGHT);
 
   SkColor color_header =
       GetThemeColor(tp, ThemeProperties::COLOR_NTP_HEADER);
@@ skipping 26 matching lines @@
   substitutions["colorTextRgba"] = SkColorToRGBAString(color_text);
   substitutions["colorSectionBorder"] =
       SkColorToRGBAString(color_section_border);
   substitutions["colorTextLight"] = SkColorToRGBAString(color_text_light);
   substitutions["colorSectionBorder"] =
       SkColorToRGBComponents(color_section_border);
   substitutions["colorText"] = SkColorToRGBComponents(color_text);
 
   // For themes that right-align the background, we flip the attribution to the
   // left to avoid conflicts.
-  int alignment = tp->GetDisplayProperty(
-      ThemeProperties::NTP_BACKGROUND_ALIGNMENT);
+  int alignment =
+      tp.GetDisplayProperty(ThemeProperties::NTP_BACKGROUND_ALIGNMENT);
   if (alignment & ThemeProperties::ALIGN_RIGHT) {
     substitutions["leftAlignAttribution"] = "0";
     substitutions["rightAlignAttribution"] = "auto";
     substitutions["textAlignAttribution"] = "right";
   } else {
     substitutions["leftAlignAttribution"] = "auto";
     substitutions["rightAlignAttribution"] = "0";
     substitutions["textAlignAttribution"] = "left";
   }
 
   substitutions["displayAttribution"] =
-      tp->HasCustomImage(IDR_THEME_NTP_ATTRIBUTION) ? "inline" : "none";
+      tp.HasCustomImage(IDR_THEME_NTP_ATTRIBUTION) ? "inline" : "none";
 
   // Get our template.
   static const base::StringPiece new_tab_theme_css(
       ResourceBundle::GetSharedInstance().GetRawDataResource(
           IDR_NEW_TAB_4_THEME_CSS));
 
   // Create the string from our template and the replacements.
   std::string css_string =
       ui::ReplaceTemplateExpressions(new_tab_theme_css, substitutions);
   new_tab_css_ = base::RefCountedString::TakeString(&css_string);
 }