Rejigger ThemeService: move exposure of ThemeProvider interface to a

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

Index: chrome/browser/ui/webui/ntp/ntp_resource_cache.cc
diff --git a/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc b/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc
index 4623939553293c05b0d540b03bc7fdbebe9c0307..98ca65534a76ef4647948b2f3b73b73462672c66 100644
--- a/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc
+++ b/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc
@@ -112,8 +112,8 @@ std::string SkColorToRGBComponents(SkColor 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() ?
@@ -122,17 +122,17 @@ SkColor GetThemeColor(ui::ThemeProvider* tp, int id) {
 
 // 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)
@@ -156,9 +156,9 @@ std::string GetNewTabBackgroundCSS(const ui::ThemeProvider* theme_provider,
 // 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);
 }
 
@@ -338,9 +338,10 @@ void NTPResourceCache::CreateNewTabIncognitoHTML() {
       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);
@@ -539,12 +540,12 @@ void NTPResourceCache::CreateNewTabHTML() {
 }
 
 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);
 
@@ -574,8 +575,8 @@ void NTPResourceCache::CreateNewTabIncognitoCSS() {
 }
 
 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 =
@@ -622,8 +623,8 @@ void NTPResourceCache::CreateNewTabCSS() {
 
   // 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";
@@ -635,7 +636,7 @@ void NTPResourceCache::CreateNewTabCSS() {
   }
 
   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(