Fix the MD Incognito NTP background color regression

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

 // Copyright 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 "base/feature_list.h"
 #include "base/memory/ref_counted_memory.h"
@@ skipping 478 matching lines @@
   new_tab_html_ = base::RefCountedString::TakeString(&full_html);
 }
 
 void NTPResourceCache::CreateNewTabIncognitoCSS() {
   // TODO(estade): this returns a subtly incorrect theme provider because
   // |profile_| is actually not the incognito profile. See crbug.com/568388
   const ui::ThemeProvider& tp =
       ThemeService::GetThemeProviderForProfile(profile_);
 
   // Get our theme colors
+  SkColor color_background_default =
+      base::FeatureList::IsEnabled(features::kMaterialDesignIncognitoNTP)
+          ? SkColorSetRGB(0x30, 0x30, 0x30)
+          : SkColorSetRGB(0x32, 0x32, 0x32);
+
   SkColor color_background =
       tp.HasCustomImage(IDR_THEME_NTP_BACKGROUND)
           ? GetThemeColor(tp, ThemeProperties::COLOR_NTP_BACKGROUND)
-          : SkColorSetRGB(0x32, 0x32, 0x32);

[Evan Stade, 2017/05/24 00:23:43]

I'm confused. Can you just remove this ternary and always use GetThemeColor(tp,
ThemeProperties::COLOR_NTP_BACKGROUND)?

[msramek, 2017/05/24 01:05:21]

Nope.

If ThemeService routes that to ThemeProperties, which  was patched in
https://codereview.chromium.org/2891983003/ to return 0x30, it's fine.

But if it routes to a CustomThemeSupplier subclass (e.g. when you select the GTK
theme on Linux), it will incorrectly return *white*. That's because
CustomThemeSupplier doesn't have a concept of incognito variants, and the
non-incognito variant of COLOR_NTP_BACKGROUND is white.

This seems to be the origin of the white flash bug (crbug.com/21798).

I could move this override one level lower into ThemeService, but I don't think
it's that much cleaner solution.

The cleanest solution is probably to define a new constant
COLOR_NTP_BACKGROUND_INCOGNITO and set it to 0x32 in all CustomThemeSupplier
subclasses. I can do that if you think it makes sense, although I'm not familiar
with the themes code.

In this CL, I'm simply keeping the override of the background color which was
always here, just change it to 0x30, since we have slightly changed the color
for the |features::kMaterialDesignIncognitoNTP| redesign.

[Evan Stade, 2017/05/24 02:00:25]

it seems like the new ternary (the feature flag one) should go in
theme_properties.cc.

The last branch of this HasCustomImage ternary can change to
ThemeProperties::GetDefaultColor(), I think.

[msramek, 2017/05/24 09:32:18]

Done. Thanks for the suggestion, I agree that that's a bit cleaner.

+          : color_background_default;
 
   // Generate the replacements.
   ui::TemplateReplacements substitutions;
 
   // Cache-buster for background.
   substitutions["themeId"] =
       profile_->GetPrefs()->GetString(prefs::kCurrentThemeID);
 
   // Colors.
   substitutions["colorBackground"] =
@@ skipping 81 matching lines @@
   // 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);
 }