[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"
@@ skipping 162 matching lines @@
 }
 
 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) {
+  bool desired_size_not_found = false;
+
+  int desired_size_in_pixel =
+      std::ceil(icon_request.size_in_dip * icon_request.device_scale_factor);

[Peter Kasting, 2016/11/26 06:22:30]

Why ceil, and not round?

[minggang, 2016/11/28 02:53:07]

1 pixel higher resolution is always better if the icon is scaled up in render
process later. In fact, I don't think this makes a difference. Please see
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.c...

On 2016/11/26 06:22:30, Peter Kasting wrote:

[Peter Kasting, 2016/11/28 03:42:42]

I'm not any more sure that code is correct than this code :)

I think it probably doesn't matter here because this value is only used if it's
16/32/64, and given that favicons have 16-px origin sizes, ceil vs. round isn't
going to turn one kind of case into the other.

But I'm beginning to convert DIPs to px in various places, and the default
conversion is probably going to be to round rather than ceiling.  If someone
does something else, it reads suspiciously; we should have a comment justifying
which behavior we choose.

In any case, you'd want to use the gfx::ToXXXInt() functions here.

On 2016/11/28 02:53:07, minggang wrote:

+
+  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:
+    case 16:
       favicon_index = SIZE_16;
       resource_id = IDR_DEFAULT_FAVICON;
       break;
+    default:

[Peter Kasting, 2016/11/26 06:22:30]

What other values are falling into here?  0?  If so why wouldn't we instead
detect 0 above directly?

[minggang, 2016/11/28 02:53:07]

To assign a value to |resource_id| here is not proper I think, as the required
resource doesn't exist. Considering the definition of IconSize
(https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/favicon_source.h...),
there is no proper value also. So, we need the flag |desired_size_not_found| to
indicate the result. With initializing it to false, we only have to assign it in
the "default". 
On 2016/11/26 06:22:30, Peter Kasting wrote:

[Peter Kasting, 2016/11/28 03:42:42]

You didn't answer my question: what values are falling into here?

Instead it looks like you answered my other comment.  But I'm not sure you
understood what I was proposing.  I think you're assuming we need to do the
handling below the switch, and are trying to set variables to do that.  I'm
suggesting restructuring the control flow to be simpler.

On 2016/11/28 02:53:07, minggang wrote:

+      desired_size_not_found = true;

[Peter Kasting, 2016/11/26 06:22:30]

It's very confusing that we set a temp here, and then key off it below, instead
of just handling this case here directly.

[minggang, 2016/11/28 02:53:07]

Please see the comment above.
On 2016/11/26 06:22:30, Peter Kasting wrote:

+      break;
   }
-  base::RefCountedMemory* default_favicon =
-      default_favicons_[favicon_index].get();
 
-  if (!default_favicon) {
+  base::RefCountedMemory* default_favicon = nullptr;
+
+  if (!desired_size_not_found) {

[Peter Kasting, 2016/11/26 06:22:30]

This is super-confusing to read, because you have a negation of a string with
"not" in it, and then an "else" case, so the else case is basically a
triple-negative.

[minggang, 2016/11/28 02:53:07]

Yes, |desired_size_not_found| with the if...else.. makes confusing. When I wrote
these codes, I was trying not to break the original functionality. On my
opinion, we benefit a little from caching these resources, and I'm not sure to
remove it directly, suggestion is needed from owners. If the caching will be
removed, the code becomes unified and resolve your doubt below. 
On 2016/11/26 06:22:30, Peter Kasting wrote:

[Peter Kasting, 2016/11/28 03:42:43]

Aim to leave the code in the best state possible, rather than aiming to make as
minimal a change as possible.

On 2016/11/28 02:53:07, minggang wrote:

+    default_favicon = default_favicons_[favicon_index].get();
+
+    if (!default_favicon) {
+      default_favicon =
+          ResourceBundle::GetSharedInstance().LoadDataResourceBytes(
+              resource_id);
+      default_favicons_[favicon_index] = default_favicon;
+    }
+  } else {
     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;
+            IDR_DEFAULT_FAVICON, resource_scale_factor);

[Peter Kasting, 2016/11/26 06:22:30]

We go to the trouble of caching the favicons above, but not down here.  Does it
matter?  How often do we hit both arms?  How much caching does the resource
bundle do underneath us?  It feels sort of like we should cache nothing or
everything.

   }
 
   icon_request.callback.Run(default_favicon);
 }