[Favicon] Stop caching the default favicon

chrome/browser/ui/webui/favicon_source.cc

 // Copyright (c) 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/favicon_source.h"
 
-#include <cmath>
-
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/top_sites_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/instant_io_context.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/common/url_constants.h"
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/favicon_base/favicon_url_parser.h"
 #include "components/history/core/browser/top_sites.h"
 #include "components/sync_sessions/open_tabs_ui_delegate.h"
 #include "net/url_request/url_request.h"
 #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()
     : size_in_dip(gfx::kFaviconSize), device_scale_factor(1.0f) {
 }
 
 FaviconSource::IconRequest::IconRequest(
     const content::URLDataSource::GotDataCallback& cb,
     const GURL& path,
     int size,
@@ skipping 38 matching lines @@
 
   chrome::ParsedFaviconPath parsed;
   bool success = chrome::ParseFaviconPath(path, icon_types_, &parsed);
   if (!success) {
     SendDefaultResponse(callback);
     return;
   }
 
   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
     // IconType.
     favicon_service->GetRawFavicon(
         url,
         favicon_base::FAVICON,
         desired_size_in_pixel,
         base::Bind(
             &FaviconSource::OnFaviconDataAvailable,
@@ skipping 76 matching lines @@
     SendDefaultResponse(request);
   }
 }
 
 void FaviconSource::SendDefaultResponse(
     const content::URLDataSource::GotDataCallback& callback) {
   SendDefaultResponse(IconRequest(callback, GURL(), 16, 1.0f));
 }
 
 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);
 }