[Favicon] Stop caching the default favicon

chrome/browser/ui/webui/favicon_source.cc

Index: chrome/browser/ui/webui/favicon_source.cc
diff --git a/chrome/browser/ui/webui/favicon_source.cc b/chrome/browser/ui/webui/favicon_source.cc
index 8ecedfbd5dfe03c668431458efc1ae3d7bd20189..9172f7d2688243b22fed2d77c9a0f18dab692daa 100644
--- a/chrome/browser/ui/webui/favicon_source.cc
+++ b/chrome/browser/ui/webui/favicon_source.cc
@@ -4,8 +4,6 @@
 
 #include "chrome/browser/ui/webui/favicon_source.h"
 
-#include <cmath>
-
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/strings/string_number_conversions.h"
@@ -23,8 +21,41 @@
 #include "ui/base/layout.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/webui/web_ui_util.h"
+#include "ui/gfx/geometry/safe_integer_conversions.h"
 #include "ui/resources/grit/ui_resources.h"
 
+namespace {
+
+struct DefaultFaviconResourceMap {
+  float scale;
+  int value;
+};
+
+const DefaultFaviconResourceMap kDefaultFaviconResources[] = {
+    {1.0f, IDR_DEFAULT_FAVICON},
+    {2.0f, IDR_DEFAULT_FAVICON_32},
+    {4.0f, IDR_DEFAULT_FAVICON_64}};
+
+// Return the resource id of default favicon, whose scale is closet to
+// |scale_factor|.
+int FindBestMatchDefaultFaviconResourceIdForScaleFactor(
+    ui::ScaleFactor scale_factor) {
+  float scale = ui::GetScaleForScaleFactor(scale_factor);
+  float smallest_diff = std::numeric_limits<float>::max();
+  int resource_id = IDR_DEFAULT_FAVICON;
+
+  for (size_t i = 0; i < arraysize(kDefaultFaviconResources); ++i) {
+    float diff = std::abs(kDefaultFaviconResources[i].scale - scale);
+    if (diff < smallest_diff) {
+      resource_id = kDefaultFaviconResources[i].value;
+      smallest_diff = diff;
+    }
+  }
+  return resource_id;
+}
+
+}  // namespace
+
 FaviconSource::IconRequest::IconRequest()
     : size_in_dip(gfx::kFaviconSize), device_scale_factor(1.0f) {
 }
@@ -81,7 +112,7 @@ void FaviconSource::StartDataRequest(
 
   GURL url(parsed.url);
   int desired_size_in_pixel =
-      std::ceil(parsed.size_in_dip * parsed.device_scale_factor);
+      gfx::ToCeiledInt(parsed.size_in_dip * parsed.device_scale_factor);
 
   if (parsed.is_icon_url) {
     // TODO(michaelbai): Change GetRawFavicon to support combination of
@@ -178,33 +209,33 @@ void FaviconSource::SendDefaultResponse(
 }
 
 void FaviconSource::SendDefaultResponse(const IconRequest& icon_request) {
-  int favicon_index;
   int resource_id;
-  switch (icon_request.size_in_dip) {
+  ui::ScaleFactor resource_scale_factor = ui::SCALE_FACTOR_NONE;
+
+  int desired_size_in_pixel = gfx::ToCeiledInt(
+      icon_request.size_in_dip * icon_request.device_scale_factor);
+
+  switch (desired_size_in_pixel) {
     case 64:
-      favicon_index = SIZE_64;
       resource_id = IDR_DEFAULT_FAVICON_64;
       break;
     case 32:
-      favicon_index = SIZE_32;
       resource_id = IDR_DEFAULT_FAVICON_32;
       break;
-    default:
-      favicon_index = SIZE_16;
+    case 16:
       resource_id = IDR_DEFAULT_FAVICON;
       break;
+    default:
+      resource_scale_factor =
+          ui::GetSupportedScaleFactor(icon_request.device_scale_factor);
+      resource_id = FindBestMatchDefaultFaviconResourceIdForScaleFactor(
+          resource_scale_factor);
+      break;
   }
+
   base::RefCountedMemory* default_favicon =
-      default_favicons_[favicon_index].get();

[Peter Kasting, 2016/12/01 07:17:46]

oshima suggested that just removing this cache might be sufficient to fix the
bug.  Is it?  If so, I'm leery about the other changes, since whether they are
correct or not, it seems like we should do them separately, with separate
justification for why they're useful.  (They seem like they're above improving
the quality of the resulting image in the case of an off-size scaled size by
using a closer origin image.)

[minggang, 2016/12/01 08:40:24]

By removing the cache, we can fix this bug apparently. The result is that we
only use the 16x16 favicon to scale up/down to any required icon size. This
behavior doesn't align with loading icons from local db. We know the root cause
here is that we select the icon size by |icon_request.size_in_dip|, not the size
in pixel, at least we shall change it along with the caching.
So, I propose 2 ways to fix it:
1. just like this patch set.
2. change "switch (icon_request.size_in_dip)" to "switch
(desired_size_in_pixel)", then remove the cache and we will only use the 16x16
favicon to scale, when the requested size is not 64x64/32x32/16x16.
@oshima what's your opinion? I am willing to correct once we align :)

-
-  if (!default_favicon) {
-    ui::ScaleFactor resource_scale_factor =
-        ui::GetSupportedScaleFactor(icon_request.device_scale_factor);
-    default_favicon =
-        ResourceBundle::GetSharedInstance().LoadDataResourceBytesForScale(
-            resource_id, resource_scale_factor);
-    default_favicons_[favicon_index] = default_favicon;
-  }
+      ResourceBundle::GetSharedInstance().LoadDataResourceBytesForScale(
+          resource_id, resource_scale_factor);
 
   icon_request.callback.Run(default_favicon);
 }