[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..29fe216184fb2af2f78cb39aa6f42748b35ab770 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,6 +21,7 @@
 #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"
 
 FaviconSource::IconRequest::IconRequest()
@@ -81,7 +80,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 +177,42 @@ void FaviconSource::SendDefaultResponse(
 }
 
 void FaviconSource::SendDefaultResponse(const IconRequest& icon_request) {
-  int favicon_index;
+  int favicon_size;
   int resource_id;
-  switch (icon_request.size_in_dip) {
-    case 64:
-      favicon_index = SIZE_64;
+  ui::ScaleFactor resource_scale_factor;
+
+  int desired_size_in_pixel = gfx::ToCeiledInt(
+      icon_request.size_in_dip * icon_request.device_scale_factor);
+
+  // Any desired size, which doesn't exist, will use the 16x16 default favicon
+  // to scale to that pixel size.
+  switch (desired_size_in_pixel) {
+    case SIZE_64:
       resource_id = IDR_DEFAULT_FAVICON_64;
+      favicon_size = SIZE_64;

[Peter Kasting, 2016/11/28 07:57:53]

It seems like the only point of this variable is to check whether it is set to
SIZE_NOT_EXISTS below.  Why not just initialize |resource_scale_factor| to
SCALE_FACTOR_NONE, eliminate this variable, and reset |resource_scale_factor| to
GetSupportedScaleFactor() in the default case?

Then you can eliminate the enum entirely.

[minggang, 2016/11/28 08:40:47]

Personally, I think that using enum is more meaningful than number, like
64/32/16, and |favicon_size| can be eliminated if by reseting the
|resource_scale_factor| in default as you said.
On 2016/11/28 07:57:53, Peter Kasting wrote:

[Peter Kasting, 2016/11/28 09:11:55]

I don't understand what you're saying.  Are you acknowledging that, regardless
of whether an enum is "more meaningful" than a number, you're planning on just
eliminating the variable and enum entirely since they're not actually needed? 
If not, please clarify.

On 2016/11/28 08:40:47, minggang wrote:

[minggang, 2016/11/28 09:56:19]

Considering to eliminate the unnecessary variable and enum, it's right to remove
the enum, I agree.

       break;
-    case 32:
-      favicon_index = SIZE_32;
+    case SIZE_32:
       resource_id = IDR_DEFAULT_FAVICON_32;
+      favicon_size = SIZE_32;
+      break;
+    case SIZE_16:
+      resource_id = IDR_DEFAULT_FAVICON;
+      favicon_size = SIZE_16;
       break;
     default:
-      favicon_index = SIZE_16;
       resource_id = IDR_DEFAULT_FAVICON;
+      favicon_size = SIZE_NOT_EXISTS;
       break;
   }
+
+  resource_scale_factor =
+      SIZE_NOT_EXISTS == favicon_size
+          ? ui::GetSupportedScaleFactor(icon_request.device_scale_factor)
+          : ui::SCALE_FACTOR_NONE;
+
   base::RefCountedMemory* default_favicon =
-      default_favicons_[favicon_index].get();

[oshima, 2016/11/30 18:04:38]

I believe the problem is that cache doesn't take the scale factor into account.
If you use the index + scale factor as a key (or just remove caching), it should
work.

-
-  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);
 }