Track when fallback icon color is the default.

On Android (and soon iOS), we track what kind of tile we show on the NTP
so that we can compare different kinds of clicks in UMA. The results are
probably no surprise: click rate for colored tiles is about twice that
of default-colored tiles (though it's not necessarily clear how much to
attribute that to our presentation and how much to the fact that good
sites generally have favicons).

Right now we have the default background color hard-coded in a few
places and use that to determine when a tile is default-colored. This is
possibly incorrect (when #787878 is in fact the dominant color) and
generally bad (constants can get out of sync).

This CL adds `bool FallbackIconStyle::is_default_background_color`, so
we can check that instead of comparing colors, and threads it through to
Android. It also deletes a few copies of the color constant. Three
copies remain: the authoritative copy in fallback_icon_style.cc, one in
its test, and one in Android's colors.xml.

BUG=651056

Review-Url: https://codereview.chromium.org/2374753002
Cr-Commit-Position: refs/heads/master@{#422739}

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemRow.java

@@ -80,7 +80,8 @@ public class BookmarkItemRow extends BookmarkRow implements LargeIconCallback {
     // LargeIconCallback implementation.
 
     @Override
-    public void onLargeIconAvailable(Bitmap icon, int fallbackColor) {
+    public void onLargeIconAvailable(
+            Bitmap icon, int fallbackColor, boolean isFallbackColorDefault) {
         if (icon == null) {
             mIconGenerator.setBackgroundColor(fallbackColor);
             icon = mIconGenerator.generateIconForUrl(mUrl);

chrome/android/java/src/org/chromium/chrome/browser/bookmarkswidget/BookmarkWidgetService.java

@@ -240,7 +240,8 @@ public class BookmarkWidgetService extends RemoteViewsService {
             mRemainingTaskCount++;
             LargeIconCallback callback = new LargeIconCallback() {
                 @Override
-                public void onLargeIconAvailable(Bitmap icon, int fallbackColor) {
+                public void onLargeIconAvailable(
+                        Bitmap icon, int fallbackColor, boolean isFallbackColorDefault) {
                     if (icon == null) {
                         mIconGenerator.setBackgroundColor(fallbackColor);
                         icon = mIconGenerator.generateIconForUrl(bookmark.url);

chrome/android/java/src/org/chromium/chrome/browser/favicon/LargeIconBridge.java

@@ -6,7 +6,6 @@ package org.chromium.chrome.browser.favicon;
 
 import android.graphics.Bitmap;
 import android.util.LruCache;
-import android.util.Pair;
 
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.chrome.browser.profiles.Profile;
@@ -21,7 +20,19 @@ public class LargeIconBridge {
     private static final int CACHE_ENTRY_MIN_SIZE_BYTES = 1024;
     private long mNativeLargeIconBridge;
     private Profile mProfile;
-    private LruCache<String, Pair<Bitmap, Integer>> mFaviconCache;
+    private LruCache<String, CachedFavicon> mFaviconCache;
+
+    private static class CachedFavicon {
+        public Bitmap icon;
+        public int fallbackColor;
+        public boolean isFallbackColorDefault;
+
+        CachedFavicon(Bitmap newIcon, int newFallbackColor, boolean newIsFallbackColorDefault) {
+            icon = newIcon;
+            fallbackColor = newFallbackColor;
+            isFallbackColorDefault = newIsFallbackColorDefault;
+        }
+    }
 
     /**
      * Callback for use with GetLargeIconForUrl().
@@ -34,7 +45,7 @@ public class LargeIconBridge {
          * @param fallbackColor The fallback color to use if icon is null.
          */
         @CalledByNative("LargeIconCallback")
-        void onLargeIconAvailable(Bitmap icon, int fallbackColor);
+        void onLargeIconAvailable(Bitmap icon, int fallbackColor, boolean isFallbackColorDefault);
     }
 
     /**
@@ -55,10 +66,10 @@ public class LargeIconBridge {
     public void createCache(int cacheSizeBytes) {
         assert cacheSizeBytes > 0;
 
-        mFaviconCache = new LruCache<String, Pair<Bitmap, Integer>>(cacheSizeBytes) {
+        mFaviconCache = new LruCache<String, CachedFavicon>(cacheSizeBytes) {
             @Override
-            protected int sizeOf(String key, Pair<Bitmap, Integer> icon) {
-                int iconBitmapSize = icon.first == null ? 0 : icon.first.getByteCount();
+            protected int sizeOf(String key, CachedFavicon favicon) {
+                int iconBitmapSize = favicon.icon == null ? 0 : favicon.icon.getByteCount();
                 return Math.max(CACHE_ENTRY_MIN_SIZE_BYTES, iconBitmapSize);
             }
         };
@@ -95,17 +106,20 @@ public class LargeIconBridge {
             return nativeGetLargeIconForURL(mNativeLargeIconBridge, mProfile, pageUrl,
                     desiredSizePx, callback);
         } else {
-            Pair<Bitmap, Integer> cached = mFaviconCache.get(pageUrl);
+            CachedFavicon cached = mFaviconCache.get(pageUrl);
             if (cached != null) {
-                callback.onLargeIconAvailable(cached.first, cached.second);
+                callback.onLargeIconAvailable(
+                        cached.icon, cached.fallbackColor, cached.isFallbackColorDefault);
                 return true;
             }
 
             LargeIconCallback callbackWrapper = new LargeIconCallback() {
                 @Override
-                public void onLargeIconAvailable(Bitmap icon, int fallbackColor) {
-                    mFaviconCache.put(pageUrl, new Pair<Bitmap, Integer>(icon, fallbackColor));
-                    callback.onLargeIconAvailable(icon, fallbackColor);
+                public void onLargeIconAvailable(
+                        Bitmap icon, int fallbackColor, boolean isFallbackColorDefault) {
+                    mFaviconCache.put(pageUrl,
+                            new CachedFavicon(icon, fallbackColor, isFallbackColorDefault));
+                    callback.onLargeIconAvailable(icon, fallbackColor, isFallbackColorDefault);
                 }
             };
             return nativeGetLargeIconForURL(mNativeLargeIconBridge, mProfile, pageUrl,

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java

@@ -37,6 +37,7 @@ import android.widget.FrameLayout;
 import android.widget.ImageView;
 import android.widget.TextView;
 
+import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.Callback;
 import org.chromium.base.Log;
 import org.chromium.base.VisibleForTesting;
@@ -1072,7 +1073,6 @@ public class NewTabPageView extends FrameLayout
 
         private static final int ICON_CORNER_RADIUS_DP = 4;
         private static final int ICON_TEXT_SIZE_DP = 20;
-        private static final int ICON_BACKGROUND_COLOR = 0xff787878;
         private static final int ICON_MIN_SIZE_PX = 48;
 
         private int mMinIconSize;
@@ -1086,9 +1086,10 @@ public class NewTabPageView extends FrameLayout
             mMinIconSize = Math.min(mDesiredIconSize, ICON_MIN_SIZE_PX);
             int desiredIconSizeDp = Math.round(
                     mDesiredIconSize / res.getDisplayMetrics().density);
-            mIconGenerator = new RoundedIconGenerator(
-                    context, desiredIconSizeDp, desiredIconSizeDp, ICON_CORNER_RADIUS_DP,
-                    ICON_BACKGROUND_COLOR, ICON_TEXT_SIZE_DP);
+            int iconColor = ApiCompatibilityUtils.getColor(
+                    getResources(), R.color.default_favicon_background_color);
+            mIconGenerator = new RoundedIconGenerator(context, desiredIconSizeDp, desiredIconSizeDp,
+                    ICON_CORNER_RADIUS_DP, iconColor, ICON_TEXT_SIZE_DP);
         }
 
         public int getNumberOfTiles(boolean searchProviderHasLogo) {
@@ -1119,13 +1120,14 @@ public class NewTabPageView extends FrameLayout
             }
 
             @Override
-            public void onLargeIconAvailable(Bitmap icon, int fallbackColor) {
+            public void onLargeIconAvailable(
+                    Bitmap icon, int fallbackColor, boolean isFallbackColorDefault) {
                 if (icon == null) {
                     mIconGenerator.setBackgroundColor(fallbackColor);
                     icon = mIconGenerator.generateIconForUrl(mItem.getUrl());
                     mItemView.setIcon(new BitmapDrawable(getResources(), icon));
-                    mItem.setTileType(fallbackColor == ICON_BACKGROUND_COLOR
-                            ? MostVisitedTileType.ICON_DEFAULT : MostVisitedTileType.ICON_COLOR);
+                    mItem.setTileType(isFallbackColorDefault ? MostVisitedTileType.ICON_DEFAULT
+                                                             : MostVisitedTileType.ICON_COLOR);
                 } else {
                     RoundedBitmapDrawable roundedIcon = RoundedBitmapDrawableFactory.create(
                             getResources(), icon);
@@ -1167,7 +1169,7 @@ public class NewTabPageView extends FrameLayout
                 Log.d(TAG, "Image decoding failed: %s", item.getWhitelistIconPath());
                 return false;
             }
-            iconCallback.onLargeIconAvailable(bitmap, Color.BLACK);
+            iconCallback.onLargeIconAvailable(bitmap, Color.BLACK, false);
             return true;
         }
 

chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ConfirmImportantSitesDialogFragment.java

@@ -120,7 +120,8 @@ public class ConfirmImportantSitesDialogFragment extends DialogFragment {
         private void loadFavicon(final ViewAndFaviconHolder viewHolder, final String url) {
             viewHolder.imageCallback = new LargeIconCallback() {
                 @Override
-                public void onLargeIconAvailable(Bitmap icon, int fallbackColor) {
+                public void onLargeIconAvailable(
+                        Bitmap icon, int fallbackColor, boolean isFallbackColorDefault) {
                     if (this != viewHolder.imageCallback) return;
                     Drawable image = getFaviconDrawable(icon, fallbackColor, url);
                     viewHolder.imageView.setImageDrawable(image);
@@ -340,4 +341,4 @@ public class ConfirmImportantSitesDialogFragment extends DialogFragment {
 
         return mDialog;
     }
-}
+}
\ No newline at end of file

chrome/browser/android/large_icon_bridge.cc

@@ -30,8 +30,6 @@ using base::android::ConvertJavaStringToUTF16;
 
 namespace {
 
-const SkColor kDefaultBackgroundColor = SkColorSetRGB(0x78, 0x78, 0x78);
-
 void OnLargeIconAvailable(
     ScopedJavaGlobalRef<jobject>* j_callback,
     const favicon_base::LargeIconResult& result) {
@@ -48,12 +46,13 @@ void OnLargeIconAvailable(
       j_bitmap = gfx::ConvertToJavaBitmap(&bitmap);
   }
 
-  jint background_color = kDefaultBackgroundColor;
+  favicon_base::FallbackIconStyle fallback;
   if (result.fallback_icon_style)
-    background_color = result.fallback_icon_style->background_color;
+    fallback = *result.fallback_icon_style;
 
-  Java_LargeIconCallback_onLargeIconAvailable(env, j_callback->obj(), j_bitmap,
-                                              background_color);
+  Java_LargeIconCallback_onLargeIconAvailable(
+      env, j_callback->obj(), j_bitmap, fallback.background_color,
+      fallback.is_default_background_color);
 }
 
 }  // namespace

components/favicon/core/large_icon_service_unittest.cc

@@ -213,6 +213,7 @@ TEST_F(LargeIconServiceTest, FallbackSinceIconTooSmall) {
   mock_favicon_service_->InjectResult(CreateTestBitmap(16, 16, kTestColor));
   expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
   expected_fallback_icon_style_->background_color = kTestColor;
+  expected_fallback_icon_style_->is_default_background_color = false;
   large_icon_service_->GetLargeIconOrFallbackStyle(
       GURL(kDummyUrl),
       24,
@@ -227,6 +228,7 @@ TEST_F(LargeIconServiceTest, FallbackSinceIconNotSquare) {
   mock_favicon_service_->InjectResult(CreateTestBitmap(24, 32, kTestColor));
   expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
   expected_fallback_icon_style_->background_color = kTestColor;
+  expected_fallback_icon_style_->is_default_background_color = false;
   large_icon_service_->GetLargeIconOrFallbackStyle(
       GURL(kDummyUrl),
       24,
@@ -271,6 +273,7 @@ TEST_F(LargeIconServiceTest, FallbackSinceTooPicky) {
   mock_favicon_service_->InjectResult(CreateTestBitmap(24, 24, kTestColor));
   expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle);
   expected_fallback_icon_style_->background_color = kTestColor;
+  expected_fallback_icon_style_->is_default_background_color = false;
   large_icon_service_->GetLargeIconOrFallbackStyle(
       GURL(kDummyUrl),
       32,

components/favicon_base/fallback_icon_style.cc

@@ -34,19 +34,20 @@ const double kDefaultRoundness = 0;  // Square. Round corners are applied
 
 FallbackIconStyle::FallbackIconStyle()
     : background_color(kDefaultBackgroundColor),
+      is_default_background_color(true),
       text_color(kDefaultTextColorLight),
       font_size_ratio(kDefaultFontSizeRatio),
-      roundness(kDefaultRoundness) {
-}
+      roundness(kDefaultRoundness) {}
 
 FallbackIconStyle::~FallbackIconStyle() {
 }
 
 bool FallbackIconStyle::operator==(const FallbackIconStyle& other) const {
   return background_color == other.background_color &&
-      text_color == other.text_color &&
-      font_size_ratio == other.font_size_ratio &&
-      roundness == other.roundness;
+         is_default_background_color == other.is_default_background_color &&
+         text_color == other.text_color &&
+         font_size_ratio == other.font_size_ratio &&
+         roundness == other.roundness;
 }
 
 void MatchFallbackIconTextColorAgainstBackgroundColor(
@@ -80,6 +81,7 @@ void SetDominantColorAsBackground(
   color_hsl.l = std::min(color_hsl.l, kMaxBackgroundColorLightness);
   style->background_color =
       color_utils::HSLToSkColor(color_hsl, SK_AlphaOPAQUE);
+  style->is_default_background_color = false;
 }
 
 }  // namespace favicon_base

components/favicon_base/fallback_icon_style.h

@@ -21,6 +21,7 @@ struct FallbackIconStyle {
 
   // Icon background fill color.
   SkColor background_color;
+  bool is_default_background_color;
 
   // Icon text color.
   SkColor text_color;

components/favicon_base/fallback_icon_url_parser.cc

@@ -85,8 +85,13 @@ bool ParsedFallbackIconPath::ParseSpecs(
   if (*size <= 0)
     return false;
 
-  if (!tokens[1].empty() && !ParseColor(tokens[1], &style->background_color))
-    return false;
+  *style = favicon_base::FallbackIconStyle();
+
+  if (!tokens[1].empty()) {
+    style->is_default_background_color = false;
+    if (!ParseColor(tokens[1], &style->background_color))
+      return false;
+  }
 
   if (tokens[2].empty())
     favicon_base::MatchFallbackIconTextColorAgainstBackgroundColor(style);

components/favicon_base/fallback_icon_url_parser_unittest.cc

@@ -93,6 +93,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsEmpty) {
   EXPECT_TRUE(ParseSpecs(",,,,", &size, &style));
   EXPECT_EQ(16, size);
   EXPECT_EQ(kDefaultBackgroundColor, style.background_color);
+  EXPECT_TRUE(style.is_default_background_color);
   EXPECT_EQ(kDefaultTextColorLight, style.text_color);
   EXPECT_EQ(kDefaultFontSizeRatio, style.font_size_ratio);
   EXPECT_EQ(kDefaultRoundness, style.roundness);
@@ -104,6 +105,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsPartial) {
   EXPECT_TRUE(ParseSpecs(",,aCE,,0.1", &size, &style));
   EXPECT_EQ(16, size);
   EXPECT_EQ(kDefaultBackgroundColor, style.background_color);
+  EXPECT_TRUE(style.is_default_background_color);
   EXPECT_EQ(SkColorSetRGB(0xAA, 0xCC, 0xEE), style.text_color);
   EXPECT_EQ(kDefaultFontSizeRatio, style.font_size_ratio);
   EXPECT_EQ(0.1, style.roundness);
@@ -117,6 +119,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsFull) {
     EXPECT_TRUE(ParseSpecs("16,000,f01,0.75,0.25", &size, &style));
     EXPECT_EQ(16, size);
     EXPECT_EQ(SkColorSetRGB(0x00, 0x00, 0x00), style.background_color);
+    EXPECT_FALSE(style.is_default_background_color);
     EXPECT_EQ(SkColorSetRGB(0xff, 0x00, 0x11), style.text_color);
     EXPECT_EQ(0.75, style.font_size_ratio);
     EXPECT_EQ(0.25, style.roundness);
@@ -127,6 +130,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsFull) {
     EXPECT_TRUE(ParseSpecs("48,black,123456,0.5,0.3", &size, &style));
     EXPECT_EQ(48, size);
     EXPECT_EQ(SkColorSetRGB(0x00, 0x00, 0x00), style.background_color);
+    EXPECT_FALSE(style.is_default_background_color);
     EXPECT_EQ(SkColorSetRGB(0x12, 0x34, 0x56), style.text_color);
     EXPECT_EQ(0.5, style.font_size_ratio);
     EXPECT_EQ(0.3, style.roundness);
@@ -137,6 +141,7 @@ TEST_F(FallbackIconUrlParserTest, ParseSpecsFull) {
     EXPECT_TRUE(ParseSpecs("1,000,red,0,0", &size, &style));
     EXPECT_EQ(1, size);
     EXPECT_EQ(SkColorSetRGB(0x00, 0x00, 0x00), style.background_color);
+    EXPECT_FALSE(style.is_default_background_color);
     EXPECT_EQ(SkColorSetRGB(0xFF, 0x00, 0x00), style.text_color);
     EXPECT_EQ(0, style.font_size_ratio);
     EXPECT_EQ(0, style.roundness);
@@ -218,6 +223,7 @@ TEST_F(FallbackIconUrlParserTest, ParseFallbackIconPathSuccess) {
     EXPECT_EQ(31, parsed.size_in_pixels());
     const favicon_base::FallbackIconStyle& style = parsed.style();
     EXPECT_EQ(SkColorSetRGB(0x00, 0x00, 0x00), style.background_color);
+    EXPECT_FALSE(style.is_default_background_color);
     EXPECT_EQ(SkColorSetRGB(0xFF, 0xFF, 0xFF), style.text_color);
     EXPECT_EQ(0.75, style.font_size_ratio);
     EXPECT_EQ(0.25, style.roundness);
@@ -232,6 +238,7 @@ TEST_F(FallbackIconUrlParserTest, ParseFallbackIconPathSuccess) {
     EXPECT_EQ(31, parsed.size_in_pixels());
     const favicon_base::FallbackIconStyle& style = parsed.style();
     EXPECT_EQ(SkColorSetRGB(0x00, 0x00, 0x00), style.background_color);
+    EXPECT_FALSE(style.is_default_background_color);
     EXPECT_EQ(SkColorSetRGB(0xFF, 0xFF, 0xFF), style.text_color);
     EXPECT_EQ(0.75, style.font_size_ratio);
     EXPECT_EQ(0.25, style.roundness);
@@ -246,6 +253,7 @@ TEST_F(FallbackIconUrlParserTest, ParseFallbackIconPathSuccess) {
     EXPECT_EQ(31, parsed.size_in_pixels());
     const favicon_base::FallbackIconStyle& style = parsed.style();
     EXPECT_EQ(SkColorSetRGB(0x00, 0x00, 0x00), style.background_color);
+    EXPECT_FALSE(style.is_default_background_color);
     EXPECT_EQ(SkColorSetRGB(0xFF, 0xFF, 0xFF), style.text_color);
     EXPECT_EQ(0.75, style.font_size_ratio);
     EXPECT_EQ(0.25, style.roundness);
@@ -261,6 +269,7 @@ TEST_F(FallbackIconUrlParserTest, ParseFallbackIconPathSuccess) {
     EXPECT_EQ(gfx::kFaviconSize, parsed.size_in_pixels());
     const favicon_base::FallbackIconStyle& style = parsed.style();
     EXPECT_EQ(kDefaultBackgroundColor, style.background_color);
+    EXPECT_TRUE(style.is_default_background_color);
     EXPECT_EQ(kDefaultTextColorLight, style.text_color);
     EXPECT_EQ(kDefaultFontSizeRatio, style.font_size_ratio);
     EXPECT_EQ(kDefaultRoundness, style.roundness);

ios/chrome/browser/favicon/large_icon_cache_unittest.cc

@@ -43,6 +43,7 @@ class LargeIconCacheTest : public testing::Test {
   LargeIconCacheTest() {
     expected_fallback_icon_style_.reset(new favicon_base::FallbackIconStyle());
     expected_fallback_icon_style_->background_color = kTestColor;
+    expected_fallback_icon_style_->is_default_background_color = false;
     expected_bitmap_ = CreateTestBitmap(24, 24, kTestColor);
     large_icon_cache_.reset(new LargeIconCache);
   }
@@ -86,6 +87,7 @@ TEST_F(LargeIconCacheTest, RetreiveItem) {
   EXPECT_EQ(false, result2->bitmap.is_valid());
   EXPECT_EQ(expected_result2->fallback_icon_style->background_color,
             result2->fallback_icon_style->background_color);
+  EXPECT_FALSE(result2->fallback_icon_style->is_default_background_color);
 
   // Test overwriting kDummyUrl.
   large_icon_cache_->SetCachedResult(GURL(kDummyUrl), *expected_result2);
@@ -94,6 +96,7 @@ TEST_F(LargeIconCacheTest, RetreiveItem) {
   EXPECT_EQ(false, result3->bitmap.is_valid());
   EXPECT_EQ(expected_result2->fallback_icon_style->background_color,
             result3->fallback_icon_style->background_color);
+  EXPECT_FALSE(result2->fallback_icon_style->is_default_background_color);
 }
 
 }  // namespace