|
|
DescriptionCurrently, 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}
Patch Set 1 #
Total comments: 12
Patch Set 2 : [Favicon] Get the default favicon size in pixel #
Total comments: 5
Patch Set 3 : restructure the code #
Total comments: 2
Patch Set 4 : Remove the caching #
Total comments: 1
Patch Set 5 : Inline ui::GetSupportedScaleFactor #Patch Set 6 : Remove the unused favicon_index #
Messages
Total messages: 41 (12 generated)
minggang.wang@intel.com changed reviewers: + oshima@chromium.org, pkasting@chromium.org, xiyuan@chromium.org
PTAL, thanks!
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.
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.
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. >
CL updates, PTAL thanks!
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.
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.
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. >
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.
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.
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.
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.
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?
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.
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.
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 ==========
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.)
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 :)
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.
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.
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!
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 ==========
Peter, oshima PTAL
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.
lgtm
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
The CQ bit was checked by minggang.wang@intel.com
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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-...)
The CQ bit was unchecked by minggang.wang@intel.com
The CQ bit was checked by minggang.wang@intel.com
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1480917640701920, "parent_rev": "38ab3b6e2464c77908fbefed4485e278a49795f5", "commit_rev": "e4e2d0389b2da075785de64b87f4fc984c62f38c"}
Message was sent while issue was closed.
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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
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} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/47fcdcbb580f9077c0b484ffc43a01f71960e06a Cr-Commit-Position: refs/heads/master@{#436237} |