[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"
 
+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) {
 }
 
 FaviconSource::IconRequest::IconRequest(
     const content::URLDataSource::GotDataCallback& cb,
     const GURL& path,
     int size,
     float scale)
     : callback(cb),
@@ skipping 36 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 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);
 }