[Favicon] Stop caching the default favicon

Currently, we are caching the default favicon, but the scale factor is
ignored. So we need to remove this in order to take the required scale
factor into account.

BUG=668023
Committed: https://crrev.com/47fcdcbb580f9077c0b484ffc43a01f71960e06a
Cr-Commit-Position: refs/heads/master@{#436237}

[minggang, 2016-11-25 06:40:16]

minggang.wang@intel.com changed reviewers:
+ oshima@chromium.org, pkasting@chromium.org, xiyuan@chromium.org

[minggang, 2016-11-25 06:40:16]

PTAL, thanks!

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

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:186:
std::ceil(icon_request.size_in_dip * icon_request.device_scale_factor);
Why ceil, and not round?

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:201: default:
What other values are falling into here?  0?  If so why wouldn't we instead
detect 0 above directly?

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:202: desired_size_not_found = true;
It's very confusing that we set a temp here, and then key off it below, instead
of just handling this case here directly.

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:208: if (!desired_size_not_found) {
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.

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:223: IDR_DEFAULT_FAVICON,
resource_scale_factor);
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.

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

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:186:
std::ceil(icon_request.size_in_dip * icon_request.device_scale_factor);
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:
> Why ceil, and not round?

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:201: default:
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:
> What other values are falling into here?  0?  If so why wouldn't we instead
> detect 0 above directly?

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:202: desired_size_not_found = true;
Please see the comment above.
On 2016/11/26 06:22:30, Peter Kasting wrote:
> It's very confusing that we set a temp here, and then key off it below,
instead
> of just handling this case here directly.

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:208: if (!desired_size_not_found) {
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:
> 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.

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

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:186:
std::ceil(icon_request.size_in_dip * icon_request.device_scale_factor);
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:
> 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:
> > Why ceil, and not round?
>

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:201: default:
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:
> 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:
> > What other values are falling into here?  0?  If so why wouldn't we instead
> > detect 0 above directly?
>

https://codereview.chromium.org/2535463002/diff/1/chrome/browser/ui/webui/fav...
chrome/browser/ui/webui/favicon_source.cc:208: if (!desired_size_not_found) {
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:
> 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:
> > 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 07:35:33]

CL updates, PTAL thanks!

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

It's somewhat frustrating that you _still_ haven't answered the question I asked
earlier about what values fall into the default case, even after I pointed out
in my previous patch set that you hadn't answered it.

Please respond.

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:192: favicon_size = SIZE_64;
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]

If I don't misunderstand your question, you want me to point out values, which
will trigger to enter the default case. I added a comment just above the switch
block to make code clear.

If I misunderstood, please forgive.

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:192: favicon_size = SIZE_64;
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:
> 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.

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

Side note: When replying to comments, try to reply below the quoted text rather
than above, since the replies will be concatenated together, so it becomes
super-confusing to read sequential top-replies top-to-bottom.  I've violated
this in my replies to you so far, only to try to be consistent with the
top-replying you've been doing, but try not to do that :)

On 2016/11/28 08:40:47, minggang wrote:
> If I don't misunderstand your question, you want me to point out values, which
> will trigger to enter the default case. I added a comment just above the
switch
> block to make code clear.
> 
> If I misunderstood, please forgive.

The comment doesn't answer my question.  I don't understand what specific value
you're getting and why it wouldn't be handled by one of the distinct cases.  Is
this just for the case of value 0, because you need to handle an unset
size_in_dip?  Is this supposed to handle other arbitrary sizes?  If the former,
should we instead be guaranteeing on the caller side that the size in DIPs gets
set correctly?  If the latter, why IDR_DEFAULT_FAVICON instead of the "nearest
larger" size?

These questions are at the core of why you're writing the change.  Effectively,
you've written a code change, and your comments are motivating _what_ the code
does, when what matters to me is _why_ this is the correct fix for the problem
you're seeing.

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:192: favicon_size = SIZE_64;
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:
> 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:
> > 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 09:55:18]

On 2016/11/28 09:11:55, Peter Kasting wrote:
> Side note: When replying to comments, try to reply below the quoted text
rather
> than above, since the replies will be concatenated together, so it becomes
> super-confusing to read sequential top-replies top-to-bottom.  I've violated
> this in my replies to you so far, only to try to be consistent with the
> top-replying you've been doing, but try not to do that :)
> 
> On 2016/11/28 08:40:47, minggang wrote:
> > If I don't misunderstand your question, you want me to point out values,
which
> > will trigger to enter the default case. I added a comment just above the
> switch
> > block to make code clear.
> > 
> > If I misunderstood, please forgive.
> 
> The comment doesn't answer my question.  I don't understand what specific
value
> you're getting and why it wouldn't be handled by one of the distinct cases. 
Is
> this just for the case of value 0, because you need to handle an unset
> size_in_dip?  Is this supposed to handle other arbitrary sizes?  If the
former,
> should we instead be guaranteeing on the caller side that the size in DIPs
gets
> set correctly?  If the latter, why IDR_DEFAULT_FAVICON instead of the "nearest
> larger" size?
> 
> These questions are at the core of why you're writing the change. 
Effectively,
> you've written a code change, and your comments are motivating _what_ the code
> does, when what matters to me is _why_ this is the correct fix for the problem
> you're seeing.
> 
>
https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/favicon_source.cc (right):
> 
>
https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/favicon_source.cc:192: favicon_size = SIZE_64;
> 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:
> > 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:
> > > 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.
> >

Sorry for the inconvenience and thanks for the guidance.

What I want to express is any arbitrary size, other than 64/32/16, should be
handled in the default case. Indeed, I didn't consider the nearest larger size.
Shall we return the biggest size, 64x64, to simplify this, it is suited for
being scaled up or down.

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

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:192: favicon_size = SIZE_64;
On 2016/11/28 09:11:55, Peter Kasting wrote:
> 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:
> > 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:
> > > 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.
> > 
> 
Considering to eliminate the unnecessary variable and enum, it's right to remove
the enum, I agree.

[Peter Kasting, 2016-11-28 20:42:48]

On 2016/11/28 09:55:18, minggang wrote:
> What I want to express is any arbitrary size, other than 64/32/16, should be
> handled in the default case.

Right, that's what the comment in the code says.  But it doesn't say _why_. 
What "arbitrary sizes" reach here?  Why do they reach here?  Why is handling
them here correct, instead of (e.g.) fixing callers to not do that?  In the
specific case of this bug, why is this change the correct one?  That kind of
context is what I'm missing.

> Indeed, I didn't consider the nearest larger size.
> Shall we return the biggest size, 64x64, to simplify this, it is suited for
> being scaled up or down.

We probably would want the biggest possible size if we were scaling up, but if
we were somehow asking for a 15x15 icon, the 16x16 source would be a better
choice than the 64x64 one, since it's already been "pixel-optimized" for a
nearby size.

But will we be asking for such a size?  What sizes would we get, and why?  This
returns to my question above.

[minggang, 2016-11-29 08:08:34]

On 2016/11/28 20:42:48, Peter Kasting wrote:
> On 2016/11/28 09:55:18, minggang wrote:
> > What I want to express is any arbitrary size, other than 64/32/16, should be
> > handled in the default case.
> 
> Right, that's what the comment in the code says.  But it doesn't say _why_. 
> What "arbitrary sizes" reach here?  Why do they reach here?  Why is handling
> them here correct, instead of (e.g.) fixing callers to not do that?  In the
> specific case of this bug, why is this change the correct one?  That kind of
> context is what I'm missing.
> 
> > Indeed, I didn't consider the nearest larger size.
> > Shall we return the biggest size, 64x64, to simplify this, it is suited for
> > being scaled up or down.
> 
> We probably would want the biggest possible size if we were scaling up, but if
> we were somehow asking for a 15x15 icon, the 16x16 source would be a better
> choice than the 64x64 one, since it's already been "pixel-optimized" for a
> nearby size.
> 
> But will we be asking for such a size?  What sizes would we get, and why? 
This
> returns to my question above.

I look into the codes to find the possible sizes used by the default favicon.

1. default favicons used in webui: the default size is 16, and @2x is 32,
defined through "-webkit-image-set"
2. used in native side (desired_size = dip_size *
scale_factor)(https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?sq=package:chromium&dr=C&rcl=1480378899&l=685):
    iOS platform scale factors can be {1.0, 2.0, 3.0}
    Android platform scale factor will be selected a closet value from array
https://cs.chromium.org/chromium/src/ui/base/resource/scale_factor.cc?q=NUM_S...
    Other platforms scale factors can be {1.0, 2.0}

So, platforms excluded Android, sizes of 16/32/48 may be requested. On Android,
the scale factor depends on display density of the device. I checked the code
path of loading a favicon from local db, the icon is just scaled to the
requested size if the proper size is not found. However, we have 3 different
resolutions of images stored in the bundle (64/32/16), which makes different. To
find the most proper size of the favicon, we have to afford to the overhead used
to find the closet size if the requested size in not one of 64/32/16.

[Peter Kasting, 2016-11-29 10:28:28]

Maybe I did not ask my question in the right way.

Please be specific about why this solves bug 668023.  I don't just want generic
background.  Why write _this_ specific change for this bug?

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

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (left):

https://codereview.chromium.org/2535463002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:198:
default_favicons_[favicon_index].get();
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.

[minggang, 2016-12-01 06:30:12]

Based on the previous comments, I change the code and give a detailed
description of what leads this bug, and why I change this and how to fix it in
the commit log. Please Peter&oshima review, thanks.

[minggang, 2016-12-01 06:31:34]

Description was changed from

==========
[Favicon] Get the default favicon size in pixel

Currently, when the default favicon is required, we use the size in DIP
to load the data from resource bundle.

But the DIP size will not change when we zoom in/out. Instead, we should
use the size in pixel. If the desired size in pixel is found, just load
it directly. Otherwise, scale the favicon to the desired size.

BUG=668023
==========

to

==========
[Favicon] Get the requested default favicon size in pixel

Currently, when the default favicon is required, we will load the
resource into memory by the requested size in DIP, later cache it
through the index of its size. The problem is that when the web page is
zoomed in/out, the scale factor will change instead of the DIP size of
the requested favicon. Thus, we always get the favicon size in
16x16/32x32. What's worse is that the icon will be cached in an array, when
the web page is scaled, the cached icon is just found by the size index
and returned and the process of scaling the icon is ignored.

After this patch, we will load the corresponding default favicon in
pixel size. If the requested size is one of 64x64/32x32/16x16, the right
favicon will be loaded from resource bundle without being scaled. Other
sizes, we will find the closed size to the requested one, which will be
used to scale later. In this way, we will always get the best match size
of default favicon.

BUG=668023
==========

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

https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (left):

https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:198:
default_favicons_[favicon_index].get();
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]

https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (left):

https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:198:
default_favicons_[favicon_index].get();
On 2016/12/01 07:17:46, Peter Kasting wrote:
> 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.)
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 :)

[Peter Kasting, 2016-12-01 09:09:59]

On 2016/12/01 08:40:24, minggang wrote:
>
https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/favicon_source.cc (left):
> 
>
https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/favicon_source.cc:198:
> default_favicons_[favicon_index].get();
> On 2016/12/01 07:17:46, Peter Kasting wrote:
> > 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.)
> 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.

I finally went and looked at the actual resources in the theme pak, and I
believe I understand this much better.

The _32 and _64 signifiers on the name are separate from the scale factor.  For
example, IDR_DEFAULT_FAVICON_64 at 1x and IDR_DEFAULT_FAVICON at 4x are both
64x64, but they have different appearances.  The approach in this patch is
confused; it's fundamentally not valid to compute a "requested pixel size" and
use that to pick a resource ID, because it conflates these two different factors
that both increase image dimension, but do so with different effects.

Instead, we need to use the request DIP size (not pixel size) to determine which
base resource to use, and the requested device scale factor to determine what
scale to pull it from.  That is basically what the code does today, except it
has this cache that makes no sense to me (sense the resource bundle caches
already AFAIK) and is wrong because it's only keyed off the resource ID, not the
scale factor.  So removing the cache or updating it to key off both factors
should fix this.  Given that I don't see the point of the cache, I'd do the
former.

If we don't get an exact match for the DIP size or scale factor, we could use
the "nearest" value; the resource bundle will do this automatically for scale
factors, while the current code doesn't implement this for the resource ID (and
doing so seems likely unnecessary, given that I'm not sure we'll ever request a
DIP size other than 16/32/64).

> The result is that we only use the 16x16 favicon to scale up/down to any
required icon size.

What do you mean by 16x16 favicon?  Do you mean IDR_DEFAULT_FAVICON?  If so,
then no, we use that when the requested _DIP_ size is not 32 or 64.  As
mentioned above, I think it's always 16 in this case.  Are you seeing something
else?

> This behavior doesn't align with loading icons from local db.

What do you mean by "local db", and how does this not align?

> We know the root cause here is that we select the icon size by
|icon_request.size_in_dip|, not the size in pixel

That's not a bug.  That's intentional and correct behavior.  The root cause here
is caching a value across a case where it doesn't apply.

[oshima, 2016-12-01 19:11:53]

On 2016/12/01 09:09:59, Peter Kasting wrote:
> On 2016/12/01 08:40:24, minggang wrote:
> >
>
https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
> > File chrome/browser/ui/webui/favicon_source.cc (left):
> > 
> >
>
https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
> > chrome/browser/ui/webui/favicon_source.cc:198:
> > default_favicons_[favicon_index].get();
> > On 2016/12/01 07:17:46, Peter Kasting wrote:
> > > 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.)
> > 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.
> 
> I finally went and looked at the actual resources in the theme pak, and I
> believe I understand this much better.
> 
> The _32 and _64 signifiers on the name are separate from the scale factor. 
For
> example, IDR_DEFAULT_FAVICON_64 at 1x and IDR_DEFAULT_FAVICON at 4x are both
> 64x64, but they have different appearances.  The approach in this patch is
> confused; it's fundamentally not valid to compute a "requested pixel size" and
> use that to pick a resource ID, because it conflates these two different
factors
> that both increase image dimension, but do so with different effects.
> 
> Instead, we need to use the request DIP size (not pixel size) to determine
which
> base resource to use, and the requested device scale factor to determine what
> scale to pull it from.  That is basically what the code does today, except it
> has this cache that makes no sense to me (sense the resource bundle caches
> already AFAIK) and is wrong because it's only keyed off the resource ID, not
the
> scale factor.  So removing the cache or updating it to key off both factors
> should fix this.  Given that I don't see the point of the cache, I'd do the
> former.
> 
> If we don't get an exact match for the DIP size or scale factor, we could use
> the "nearest" value; the resource bundle will do this automatically for scale
> factors, while the current code doesn't implement this for the resource ID
(and
> doing so seems likely unnecessary, given that I'm not sure we'll ever request
a
> DIP size other than 16/32/64).
> 
> > The result is that we only use the 16x16 favicon to scale up/down to any
> required icon size.
> 
> What do you mean by 16x16 favicon?  Do you mean IDR_DEFAULT_FAVICON?  If so,
> then no, we use that when the requested _DIP_ size is not 32 or 64.  As
> mentioned above, I think it's always 16 in this case.  Are you seeing
something
> else?
> 
> > This behavior doesn't align with loading icons from local db.
> 
> What do you mean by "local db", and how does this not align?
> 
> > We know the root cause here is that we select the icon size by
> |icon_request.size_in_dip|, not the size in pixel
> 
> That's not a bug.  That's intentional and correct behavior.  The root cause
here
> is caching a value across a case where it doesn't apply.

Peter is correct. There are separate assets for each scale factor,

ui/resources/default_100_percent/common/default_favicon.png
ui/resources/default_200_percent/common/default_favicon.png
ui/resources/default_300_percent/common/default_favicon.png

and the LoadDataResourceBytesForScale will fetch the correct one based on the
scale factor.

[minggang, 2016-12-02 02:20:22]

On 2016/12/01 19:11:53, oshima wrote:
> On 2016/12/01 09:09:59, Peter Kasting wrote:
> > On 2016/12/01 08:40:24, minggang wrote:
> > >
> >
>
https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
> > > File chrome/browser/ui/webui/favicon_source.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2535463002/diff/40001/chrome/browser/ui/webui...
> > > chrome/browser/ui/webui/favicon_source.cc:198:
> > > default_favicons_[favicon_index].get();
> > > On 2016/12/01 07:17:46, Peter Kasting wrote:
> > > > 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.)
> > > 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.
> > 
> > I finally went and looked at the actual resources in the theme pak, and I
> > believe I understand this much better.
> > 
> > The _32 and _64 signifiers on the name are separate from the scale factor. 
> For
> > example, IDR_DEFAULT_FAVICON_64 at 1x and IDR_DEFAULT_FAVICON at 4x are both
> > 64x64, but they have different appearances.  The approach in this patch is
> > confused; it's fundamentally not valid to compute a "requested pixel size"
and
> > use that to pick a resource ID, because it conflates these two different
> factors
> > that both increase image dimension, but do so with different effects.
> > 
> > Instead, we need to use the request DIP size (not pixel size) to determine
> which
> > base resource to use, and the requested device scale factor to determine
what
> > scale to pull it from.  That is basically what the code does today, except
it
> > has this cache that makes no sense to me (sense the resource bundle caches
> > already AFAIK) and is wrong because it's only keyed off the resource ID, not
> the
> > scale factor.  So removing the cache or updating it to key off both factors
> > should fix this.  Given that I don't see the point of the cache, I'd do the
> > former.
> > 
> > If we don't get an exact match for the DIP size or scale factor, we could
use
> > the "nearest" value; the resource bundle will do this automatically for
scale
> > factors, while the current code doesn't implement this for the resource ID
> (and
> > doing so seems likely unnecessary, given that I'm not sure we'll ever
request
> a
> > DIP size other than 16/32/64).
> > 
> > > The result is that we only use the 16x16 favicon to scale up/down to any
> > required icon size.
> > 
> > What do you mean by 16x16 favicon?  Do you mean IDR_DEFAULT_FAVICON?  If so,
> > then no, we use that when the requested _DIP_ size is not 32 or 64.  As
> > mentioned above, I think it's always 16 in this case.  Are you seeing
> something
> > else?
> > 
> > > This behavior doesn't align with loading icons from local db.
> > 
> > What do you mean by "local db", and how does this not align?
> > 
> > > We know the root cause here is that we select the icon size by
> > |icon_request.size_in_dip|, not the size in pixel
> > 
> > That's not a bug.  That's intentional and correct behavior.  The root cause
> here
> > is caching a value across a case where it doesn't apply.
> 
> Peter is correct. There are separate assets for each scale factor,
> 
> ui/resources/default_100_percent/common/default_favicon.png
> ui/resources/default_200_percent/common/default_favicon.png
> ui/resources/default_300_percent/common/default_favicon.png
> 
> and the LoadDataResourceBytesForScale will fetch the correct one based on the
> scale factor.

I see, that's clear now, I am going to update the patch then, thanks!

[minggang, 2016-12-02 05:29:21]

Description was changed from

==========
[Favicon] Get the requested default favicon size in pixel

Currently, when the default favicon is required, we will load the
resource into memory by the requested size in DIP, later cache it
through the index of its size. The problem is that when the web page is
zoomed in/out, the scale factor will change instead of the DIP size of
the requested favicon. Thus, we always get the favicon size in
16x16/32x32. What's worse is that the icon will be cached in an array, when
the web page is scaled, the cached icon is just found by the size index
and returned and the process of scaling the icon is ignored.

After this patch, we will load the corresponding default favicon in
pixel size. If the requested size is one of 64x64/32x32/16x16, the right
favicon will be loaded from resource bundle without being scaled. Other
sizes, we will find the closed size to the requested one, which will be
used to scale later. In this way, we will always get the best match size
of default favicon.

BUG=668023
==========

to

==========
Currently, we are caching the default favicon, but the scale factor is
ignored. So we need to remove this in order to take the required scale
factor into account.

BUG=668023
==========

[minggang, 2016-12-02 07:12:52]

Peter, oshima PTAL

[Peter Kasting, 2016-12-02 21:27:01]

LGTM

https://codereview.chromium.org/2535463002/diff/60001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/favicon_source.cc (right):

https://codereview.chromium.org/2535463002/diff/60001/chrome/browser/ui/webui...
chrome/browser/ui/webui/favicon_source.cc:199:
ui::GetSupportedScaleFactor(icon_request.device_scale_factor);
Nit: I'd just inline this below.

[xiyuan, 2016-12-02 21:27:59]

lgtm

[oshima, 2016-12-02 21:48:27]

lgtm

On 2016/12/02 21:27:01, Peter Kasting wrote:
> LGTM
> 
>
https://codereview.chromium.org/2535463002/diff/60001/chrome/browser/ui/webui...
> File chrome/browser/ui/webui/favicon_source.cc (right):
> 
>
https://codereview.chromium.org/2535463002/diff/60001/chrome/browser/ui/webui...
> chrome/browser/ui/webui/favicon_source.cc:199:
> ui::GetSupportedScaleFactor(icon_request.device_scale_factor);
> Nit: I'd just inline this below.

+1

[minggang, 2016-12-05 05:27:53]

The CQ bit was checked by minggang.wang@intel.com

[minggang, 2016-12-05 05:27:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, oshima@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2535463002/#ps80001
(title: "Inline ui::GetSupportedScaleFactor")

[commit-bot: I haz the power, 2016-12-05 05:28:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 05:46:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 05:46:19]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[minggang, 2016-12-05 06:00:05]

The CQ bit was unchecked by minggang.wang@intel.com

[minggang, 2016-12-05 06:00:40]

The CQ bit was checked by minggang.wang@intel.com

[minggang, 2016-12-05 06:00:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, oshima@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2535463002/#ps100001
(title: "Remove the unused favicon_index")

[commit-bot: I haz the power, 2016-12-05 06:00:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 07:12:02]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480917640701920,
"parent_rev": "38ab3b6e2464c77908fbefed4485e278a49795f5", "commit_rev":
"e4e2d0389b2da075785de64b87f4fc984c62f38c"}

[commit-bot: I haz the power, 2016-12-05 07:12:57]

Description was changed from

==========
Currently, we are caching the default favicon, but the scale factor is
ignored. So we need to remove this in order to take the required scale
factor into account.

BUG=668023
==========

to

==========
Currently, we are caching the default favicon, but the scale factor is
ignored. So we need to remove this in order to take the required scale
factor into account.

BUG=668023
==========

[commit-bot: I haz the power, 2016-12-05 07:12:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-05 07:14:34]

Description was changed from

==========
Currently, we are caching the default favicon, but the scale factor is
ignored. So we need to remove this in order to take the required scale
factor into account.

BUG=668023
==========

to

==========
Currently, we are caching the default favicon, but the scale factor is
ignored. So we need to remove this in order to take the required scale
factor into account.

BUG=668023

Committed: https://crrev.com/47fcdcbb580f9077c0b484ffc43a01f71960e06a
Cr-Commit-Position: refs/heads/master@{#436237}
==========

[commit-bot: I haz the power, 2016-12-05 07:14:35]

Patchset 6 (id:??) landed as
https://crrev.com/47fcdcbb580f9077c0b484ffc43a01f71960e06a
Cr-Commit-Position: refs/heads/master@{#436237}