Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9696057:
Prioritize smaller favicons over larger ones for tabs, etc. (Closed)
|
Created:
8 years, 9 months ago by stevenjb Modified:
8 years, 9 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPrioritize smaller favicons over larger ones for tabs, etc.
Currently if multiple favicons are specified (through <link rel="icon"> tags), the first favicon specified will be used.
Unfortunately, this ordering is inconsistent, at least when using querySelector.
In order to support multiply sized icons for the Ash Launcher, we need to prioritize properly sized favicons (i.e. closest to 16x16) for normal favicons (type FaviconURL::FAVICON).
BUG=110143
TEST=Test favicon behavior thoroughly. On Ash with the latest GTalk, avatar icons should show in the launcher and status icons should show in the title bar.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127737
Patch Set 1 #Patch Set 2 : Appease clang #
Total comments: 6
Patch Set 3 : Add unit test for multiple favicons. #Patch Set 4 : Comment change. #
Total comments: 4
Patch Set 5 : Cache bitmap in FaviconCandidate. #Patch Set 6 : Store single FavionCandidate #
Total comments: 10
Patch Set 7 : Clear download requests if Fetch is called while processing candidates. #Patch Set 8 : Split OnUpdateFaviconURL into two parts #
Total comments: 8
Patch Set 9 : Address comments. #Patch Set 10 : Don't clear download_requests_ #Patch Set 11 : Rebase #
Total comments: 2
Messages
Total messages: 36 (0 generated)
It seems that you didn't fix the root cause. Note that, we only support 3 links a. <link rel="apple-touch-icon" href="somepath/image.png" /> b. <link rel="apple-touch-icon-precomposed" href="somepath/image.png" /> c. or apple-touch-icon-precomposed.png and apple-touch-icon.png (in order of priority) located in the web site's root. d. favicon Would you like to check whether gmail has these icons set? If gmail use other <link rel="">, you probably need to read it from webkit.
This looks good, but could you add some test coverage? http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.h (right): http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.h:173: FaviconCandidate() {} Don't inline this, and add a destructor (that isn't inlined either).
Ah, I think there is some confusion here. It is perfectly valid for a website to contain multiple links of the same type, e.g.: <link rel="shortcut icon" href="somepath/icon1.png" /> <link rel="shortcut icon" href="somepath/icon2.png" /> <link rel="shortcut icon" href="somepath/icon3.png" /> WebKit will pass us all of these icons (along with other icon types, e.g. touch, which we ignore), but not in a predictable order (at least, it is not predictable if you change a property of any of the nodes). Presumably few if any sites currently set more than one icon, however if they do, it seems reasonable to use the icon with the preferred size. I think as we enter the world of multiple resolution displays, this could be more generally useful (e.g. the preferred size for a normal favicon on a high resolution tablet might be 32x32 instead of 16x16), but for now it serves as a simple differentiator between tab icons and launcher icons. On 2012/03/13 23:38:11, michaelbai wrote: > It seems that you didn't fix the root cause. Note that, we only support 3 links > > a. <link rel="apple-touch-icon" href="somepath/image.png" /> > b. <link rel="apple-touch-icon-precomposed" href="somepath/image.png" /> > c. or apple-touch-icon-precomposed.png and apple-touch-icon.png (in order of > priority) located in the web site's root. > d. favicon > > Would you like to check whether gmail has these icons set? > If gmail use other <link rel="">, you probably need to read it from webkit.
On 2012/03/13 23:50:11, stevenjb (chromium) wrote: > Ah, I think there is some confusion here. > It is perfectly valid for a website to contain multiple links of the same type, > e.g.: > > <link rel="shortcut icon" href="somepath/icon1.png" /> > <link rel="shortcut icon" href="somepath/icon2.png" /> > <link rel="shortcut icon" href="somepath/icon3.png" /> > What's the icon type webkit returned? > WebKit will pass us all of these icons (along with other icon types, e.g. touch, > which we ignore), but not in a predictable order (at least, it is not > predictable if you change a property of any of the nodes). > > Presumably few if any sites currently set more than one icon, however if they > do, it seems reasonable to use the icon with the preferred size. I think as we > enter the world of multiple resolution displays, this could be more generally > useful (e.g. the preferred size for a normal favicon on a high resolution tablet > might be 32x32 instead of 16x16), but for now it serves as a simple > differentiator between tab icons and launcher icons. > > > > On 2012/03/13 23:38:11, michaelbai wrote: > > It seems that you didn't fix the root cause. Note that, we only support 3 > links > > > > a. <link rel="apple-touch-icon" href="somepath/image.png" /> > > b. <link rel="apple-touch-icon-precomposed" href="somepath/image.png" /> > > c. or apple-touch-icon-precomposed.png and apple-touch-icon.png (in order of > > priority) located in the web site's root. > > d. favicon > > > > Would you like to check whether gmail has these icons set? > > If gmail use other <link rel="">, you probably need to read it from webkit.
http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.h (right): http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.h:229: // that either the width and/or height is 16 pixels wide. Does nothing if the 16 -> preferred_size()? http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.h:244: return icon_types_ == history::FAVICON ? gfx::kFaviconSize : 0; How will you specified the icon size (32x32) you want?
Test added. PTAL. http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.h (right): http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.h:173: FaviconCandidate() {} On 2012/03/13 23:46:06, sky wrote: > Don't inline this, and add a destructor (that isn't inlined either). Done. http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.h:229: // that either the width and/or height is 16 pixels wide. Does nothing if the On 2012/03/14 05:01:03, michaelbai wrote: > 16 -> preferred_size()? Actually, we always scale to gfx::kFaviconSize. I changed the comment. http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.h:244: return icon_types_ == history::FAVICON ? gfx::kFaviconSize : 0; On 2012/03/14 05:01:03, michaelbai wrote: > How will you specified the icon size (32x32) you want? We handle launcher icons in a separate piece of code (I didn't want to interfere with the caching or NavigationEntry at this point). This CL is just to ensure that if more than one favicon is specified, we use the one closest to preferred_size() (16x16).
On 2012/03/14 21:35:26, stevenjb (chromium) wrote: > Test added. PTAL. > > http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... > File chrome/browser/favicon/favicon_handler.h (right): > > http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... > chrome/browser/favicon/favicon_handler.h:173: FaviconCandidate() {} > On 2012/03/13 23:46:06, sky wrote: > > Don't inline this, and add a destructor (that isn't inlined either). > > Done. > > http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... > chrome/browser/favicon/favicon_handler.h:229: // that either the width and/or > height is 16 pixels wide. Does nothing if the > On 2012/03/14 05:01:03, michaelbai wrote: > > 16 -> preferred_size()? > Actually, we always scale to gfx::kFaviconSize. I changed the comment. > > http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... > chrome/browser/favicon/favicon_handler.h:244: return icon_types_ == > history::FAVICON ? gfx::kFaviconSize : 0; > On 2012/03/14 05:01:03, michaelbai wrote: > > How will you specified the icon size (32x32) you want? > We handle launcher icons in a separate piece of code (I didn't want to interfere > with the caching or NavigationEntry at this point). This CL is just to ensure > that if more than one favicon is specified, we use the one closest to > preferred_size() (16x16). A question, for the link tag like below <link rel="shortcut icon" href="somepath/icon1.png" /> <link rel="shortcut icon" href="somepath/icon2.png" /> <link rel="shortcut icon" href="somepath/icon3.png" /> Do you know what's the icon type returned by webkit?
On 2012/03/14 21:41:38, michaelbai wrote: > On 2012/03/14 21:35:26, stevenjb (chromium) wrote: > > Test added. PTAL. > > > > > http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... > > File chrome/browser/favicon/favicon_handler.h (right): > > > > > http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... > > chrome/browser/favicon/favicon_handler.h:173: FaviconCandidate() {} > > On 2012/03/13 23:46:06, sky wrote: > > > Don't inline this, and add a destructor (that isn't inlined either). > > > > Done. > > > > > http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... > > chrome/browser/favicon/favicon_handler.h:229: // that either the width and/or > > height is 16 pixels wide. Does nothing if the > > On 2012/03/14 05:01:03, michaelbai wrote: > > > 16 -> preferred_size()? > > Actually, we always scale to gfx::kFaviconSize. I changed the comment. > > > > > http://codereview.chromium.org/9696057/diff/3001/chrome/browser/favicon/favic... > > chrome/browser/favicon/favicon_handler.h:244: return icon_types_ == > > history::FAVICON ? gfx::kFaviconSize : 0; > > On 2012/03/14 05:01:03, michaelbai wrote: > > > How will you specified the icon size (32x32) you want? > > We handle launcher icons in a separate piece of code (I didn't want to > interfere > > with the caching or NavigationEntry at this point). This CL is just to ensure > > that if more than one favicon is specified, we use the one closest to > > preferred_size() (16x16). > > A question, for the link tag like below > > <link rel="shortcut icon" href="somepath/icon1.png" /> > <link rel="shortcut icon" href="somepath/icon2.png" /> > <link rel="shortcut icon" href="somepath/icon3.png" /> > > Do you know what's the icon type returned by webkit? "icon" and "shortcut icon" are both type FaviconURL::FAVICON.
Thanks for the information http://codereview.chromium.org/9696057/diff/8003/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/8003/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:297: i->second.url, image_url, image, i->second.icon_type)); Would you like stop fetch the rest of icons if the preferred_size == bitmap_size? It will reduce the network access, and the right favicon could be stored before the next page loaded.
We already have code in the WebKit glue layer that tries to pick the right size image for a favicon (when provided with e.g. an ICO file that has multiple subimages). Are you familiar with it? Can we implement what you want in terms of that code? I would prefer to avoid code duplication if possible.
http://codereview.chromium.org/9696057/diff/8003/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/8003/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:297: i->second.url, image_url, image, i->second.icon_type)); On 2012/03/14 22:34:14, michaelbai wrote: > Would you like stop fetch the rest of icons if the preferred_size == > bitmap_size? > > It will reduce the network access, and the right favicon could be stored before > the next page loaded. Yes that would be better. I thought of this but didn't want to convert from gfx::Image yet again. I think what I really need to do is cache the SkBitmap in the Candidate. Done.
On 2012/03/14 22:37:28, Peter Kasting wrote: > We already have code in the WebKit glue layer that tries to pick the right size > image for a favicon (when provided with e.g. an ICO file that has multiple > subimages). Are you familiar with it? Can we implement what you want in terms > of that code? I would prefer to avoid code duplication if possible. I'm not familiar with that code, can you point me to it? I suspect that it will be orthogonal to this - it sounds like that code correctly chooses the size of multi-resolution images, where as this is designed to choose the best sized icon when more than one <link rel="icon"> is provided. Assuming that the WebKit glue code also matches gfx::kFaviconSize, a multi-resolution image should result in a matching image size.
http://codereview.chromium.org/9696057/diff/8003/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/8003/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:297: i->second.url, image_url, image, i->second.icon_type)); It seemed you don't need favicon_candidates_ vector, just store the currently best matched one while you downloading the icon On 2012/03/15 00:45:53, stevenjb (chromium) wrote: > On 2012/03/14 22:34:14, michaelbai wrote: > > Would you like stop fetch the rest of icons if the preferred_size == > > bitmap_size? > > > > It will reduce the network access, and the right favicon could be stored > before > > the next page loaded. > > Yes that would be better. I thought of this but didn't want to convert from > gfx::Image yet again. I think what I really need to do is cache the SkBitmap in > the Candidate. > > Done.
PTAL http://codereview.chromium.org/9696057/diff/8003/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/8003/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:297: i->second.url, image_url, image, i->second.icon_type)); On 2012/03/15 01:12:00, michaelbai wrote: > It seemed you don't need favicon_candidates_ vector, just store the currently > best matched one while you downloading the icon > > On 2012/03/15 00:45:53, stevenjb (chromium) wrote: > > On 2012/03/14 22:34:14, michaelbai wrote: > > > Would you like stop fetch the rest of icons if the preferred_size == > > > bitmap_size? > > > > > > It will reduce the network access, and the right favicon could be stored > > before > > > the next page loaded. > > > > Yes that would be better. I thought of this but didn't want to convert from > > gfx::Image yet again. I think what I really need to do is cache the SkBitmap > in > > the Candidate. > > > > Done. > That's a fair point. I did another round of re-factoring.
Thought about your change carefully, I am afraid there is race condition, please check my inline comment, you'd better confirm with sky, as I wrote the code a few months ago. If it is not a issue, this CL looks good to me. http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:130: favicon_candidate_.icon_type = history::INVALID_ICON; It seemed that you should store the best matched candidate with DownloadRequest. I am afraid that there is race condition here. Imaging that the 2nd response of DownloadRequest is in the UI queue, and the icon_type is reset to INVALID_ICON here. http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:306: bool exact_match = UpdateFaviconCandidate( nit: You don't need exact_match here. http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:310: } You changed the logical here, previously, we didn't try the reset of icons if error happened. I am not sure if it is a issue, please confirm with sky. http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:323: favicon_candidate_.icon_type = history::INVALID_ICON; Could you reset all by favicon_candidate = FaviconCandidate();
http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:130: favicon_candidate_.icon_type = history::INVALID_ICON; On 2012/03/15 22:39:53, michaelbai wrote: > It seemed that you should store the best matched candidate with DownloadRequest. > I am afraid that there is race condition here. Imaging that the 2nd response of > DownloadRequest is in the UI queue, and the icon_type is reset to INVALID_ICON here. I believe that the call to CancelAllRequests() should prevent any further calls to OnDidDownloadFavicon() until a fresh call to OnUpdateFaviconURL(). I can also clear download_requests_ which will prevent processing any current pending requests. http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:306: bool exact_match = UpdateFaviconCandidate( On 2012/03/15 22:39:53, michaelbai wrote: > nit: You don't need exact_match here. I thought the code would be more readable this way, since the return value isn't entirely obvious from the function name. http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:310: } On 2012/03/15 22:39:53, michaelbai wrote: > You changed the logical here, previously, we didn't try the reset of icons if > error happened. I am not sure if it is a issue, please confirm with sky. Previously we tried the remaining icons only if there was an error. Now we always try the remaining icons unless there was not an error and the icon is an exact match. http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:323: favicon_candidate_.icon_type = history::INVALID_ICON; On 2012/03/15 22:39:53, michaelbai wrote: > Could you reset all by favicon_candidate = FaviconCandidate(); Done.
ping
http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:130: favicon_candidate_.icon_type = history::INVALID_ICON; On 2012/03/15 23:29:06, stevenjb (chromium) wrote: > On 2012/03/15 22:39:53, michaelbai wrote: > > It seemed that you should store the best matched candidate with > DownloadRequest. > > I am afraid that there is race condition here. Imaging that the 2nd response > of > > DownloadRequest is in the UI queue, and the icon_type is reset to INVALID_ICON > here. > > I believe that the call to CancelAllRequests() should prevent any further calls > to OnDidDownloadFavicon() until a fresh call to OnUpdateFaviconURL(). CancelAllRequests only cancels requests to the faviconservice, not to the renderer. I don't know that there is a way to cancel such requests. > I can also clear download_requests_ which will prevent processing any current > pending requests. If we do that, it means when we might not store an image we downloaded when we should.
http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... chrome/browser/favicon/favicon_handler.cc:130: favicon_candidate_.icon_type = history::INVALID_ICON; On 2012/03/16 22:15:09, sky wrote: > On 2012/03/15 23:29:06, stevenjb (chromium) wrote: > > On 2012/03/15 22:39:53, michaelbai wrote: > > > It seemed that you should store the best matched candidate with > > DownloadRequest. > > > I am afraid that there is race condition here. Imaging that the 2nd response > > of > > > DownloadRequest is in the UI queue, and the icon_type is reset to > INVALID_ICON > > here. > > > > I believe that the call to CancelAllRequests() should prevent any further > calls > > to OnDidDownloadFavicon() until a fresh call to OnUpdateFaviconURL(). > > CancelAllRequests only cancels requests to the faviconservice, not to the > renderer. I don't know that there is a way to cancel such requests. > > > I can also clear download_requests_ which will prevent processing any current > > pending requests. > > If we do that, it means when we might not store an image we downloaded when we > should. My understanding is that FetchFavicon() will always be followed by a fresh call to OnUpdateFaviconURL(). Otherwise the existing code would have the same problem if the first entry was invalid. So, by clearing download_requests_, we ignore any pending results and just process the new list, which is what we want. I can clean this up some more by splitting OnUpdateFaviconURL() into two parts if that would help clarify this.
OK, I think this makes things a little cleaner/safer since we now have a separate entry point for new lists of candidates, vs. requesting additional ones. We are now however rapidly approaching a significant re-factoring.
On 2012/03/16 22:31:29, stevenjb (chromium) wrote: > http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... > File chrome/browser/favicon/favicon_handler.cc (right): > > http://codereview.chromium.org/9696057/diff/7012/chrome/browser/favicon/favic... > chrome/browser/favicon/favicon_handler.cc:130: favicon_candidate_.icon_type = > history::INVALID_ICON; > On 2012/03/16 22:15:09, sky wrote: > > On 2012/03/15 23:29:06, stevenjb (chromium) wrote: > > > On 2012/03/15 22:39:53, michaelbai wrote: > > > > It seemed that you should store the best matched candidate with > > > DownloadRequest. > > > > I am afraid that there is race condition here. Imaging that the 2nd > response > > > of > > > > DownloadRequest is in the UI queue, and the icon_type is reset to > > INVALID_ICON > > > here. > > > > > > I believe that the call to CancelAllRequests() should prevent any further > > calls > > > to OnDidDownloadFavicon() until a fresh call to OnUpdateFaviconURL(). > > > > CancelAllRequests only cancels requests to the faviconservice, not to the > > renderer. I don't know that there is a way to cancel such requests. > > > > > I can also clear download_requests_ which will prevent processing any > current > > > pending requests. > > > > If we do that, it means when we might not store an image we downloaded when we > > should. > > My understanding is that FetchFavicon() will always be followed by a fresh call > to OnUpdateFaviconURL(). Otherwise the existing code would have the same problem > if the first entry was invalid. Not sure what you mean by fresh. DoUrlAndIconMatch will return false if the request is from the last navigation, which looking at the code again means it'll be dropped. > So, by clearing download_requests_, we ignore any pending results and just > process the new list, which is what we want. I don't think we can do that as download_requests_ is also populated with random download requests. Yes, I don't know how that aspect of FaviconHandler snuck in. It should be in a separate object. > I can clean this up some more by splitting OnUpdateFaviconURL() into two parts > if that would help clarify this.
LGTM http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.cc:127: favicon_candidate_.icon_type = history::INVALID_ICON; For safety reset favicon_candidate_ entirely here, eg: favicon_candidate_ = FAviconCandidate(); http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.cc:158: int preferred_size = preferred_icon_size(); nit: no need to assign this, use preferred_icon_size() everywhere. http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.cc:306: bool exact_match = UpdateFaviconCandidate( nit: make this request_next_icon = !UpdateF... http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... File chrome/browser/favicon/favicon_handler.h (right): http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.h:60: // DownloadFaviconOrAskHistory does the following: Update comments with the new flow.
http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.cc:127: favicon_candidate_.icon_type = history::INVALID_ICON; On 2012/03/16 23:29:15, sky wrote: > For safety reset favicon_candidate_ entirely here, eg: > > favicon_candidate_ = FAviconCandidate(); Done. http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.cc:158: int preferred_size = preferred_icon_size(); On 2012/03/16 23:29:15, sky wrote: > nit: no need to assign this, use preferred_icon_size() everywhere. Done. http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.cc:306: bool exact_match = UpdateFaviconCandidate( On 2012/03/16 23:29:15, sky wrote: > nit: make this request_next_icon = !UpdateF... Done. http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... File chrome/browser/favicon/favicon_handler.h (right): http://codereview.chromium.org/9696057/diff/17002/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.h:60: // DownloadFaviconOrAskHistory does the following: On 2012/03/16 23:29:15, sky wrote: > Update comments with the new flow. Doh. Thanks. Done.
PTAL I looked more closely at download_requests_ and I see now how clearing it could be bad in some edge cases. I also noted that we don't need to clear it; we already check in OnDidDownloadFavicon() that we received the current request in DoUrlAndIconMatch().
The previous code could save the icon to history backend even the image_urls was change. We need to keep this ability. When the entry() transfer from A to B, the favicon_candidate will be reset before downloading the B's favicon. We lost the matched the favicon we got for A, and the B's favicon could be set to the A's when the response of A's favicon come back. To make the code more clear we should store the image_urls and favicon_candidate along with download_request. So the download will be independent. We could decide whether we continue downloading the rest of icons when current entry changed, or whether we store the favicon we found but might not best matched.
On 2012/03/19 18:27:10, michaelbai wrote: > The previous code could save the icon to history backend even the image_urls was > change. We need to keep this ability. I'm not sure I understand this, but I don't think that behavior should have changed. > > When the entry() transfer from A to B, the favicon_candidate will be reset > before downloading the B's favicon. We lost the matched the favicon we got for > A, and the B's favicon could be set to the A's when the response of A's favicon > come back. I'm not sure I understand what A and B refer to here. > > To make the code more clear we should store the image_urls and favicon_candidate > along with download_request. So the download will be independent. > > We could decide whether we continue downloading the rest of icons when current > entry changed, or whether we store the favicon we found but might not best > matched. I have been trying to keep this change as small as possible and not affect current behavior or significantly re-factor this code. In particular, the intention here is to only change the behavior for: IconHostMsg_DidDownloadFavicon -> OnDidDownloadFavicon() i.e. if an icon is in the history cache, that behavior should be unaffected since OnDidDownloadFavicon() should never get called. I admit that I do not fully understand the history fetch mechanism, so if there is a flow that I have missed, please show me the code flow. I can also come by tomorrow when I am in MTV, but pretty soon we may have to decide to punt on this approach entirely if we are not satisfied with this solution.
http://codereview.chromium.org/9696057/diff/22005/chrome/browser/favicon/favi... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/22005/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.cc:231: void FaviconHandler::OnUpdateFaviconURL( I'm pretty sure that the page can trigger this to be invoked via javascript. Is that going to be problematic?
http://codereview.chromium.org/9696057/diff/22005/chrome/browser/favicon/favi... File chrome/browser/favicon/favicon_handler.cc (right): http://codereview.chromium.org/9696057/diff/22005/chrome/browser/favicon/favi... chrome/browser/favicon/favicon_handler.cc:231: void FaviconHandler::OnUpdateFaviconURL( On 2012/03/19 20:34:56, sky wrote: > I'm pretty sure that the page can trigger this to be invoked via javascript. Is > that going to be problematic? As long as this is always getting called with a complete set of urls, this should be fine. If we were in the middle of updating (i.e. image_urls_ is not empty and we are waiting for a callback), we will start over with the new list. When OnDidDownloadFavicon() gets called with the stale url, the call to DoUrlAndIconMatch() will fail and that data will be ignored. Put another way, every call to OnUpdateFaviconURL() will generate a new complete list of urls to consider, and the closest match from that list will be used.
SLGTM
LGTM I saw your point, I have no issue with your change. Thanks for your patience.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9696057/22005
On 2012/03/20 01:10:37, michaelbai wrote: > LGTM > > I saw your point, I have no issue with your change. Thanks for your patience. No worries, this is some tricky code, thanks for the feedback. I will be certain to keep an eye on any favicon issues for R19.
Change committed as 127737
On 2012/03/15 00:55:34, stevenjb (chromium) wrote: > On 2012/03/14 22:37:28, Peter Kasting wrote: > > We already have code in the WebKit glue layer that tries to pick the right > size > > image for a favicon (when provided with e.g. an ICO file that has multiple > > subimages). Are you familiar with it? Can we implement what you want in > terms > > of that code? I would prefer to avoid code duplication if possible. > > I'm not familiar with that code, can you point me to it? > I suspect that it will be orthogonal to this - it sounds like that code > correctly chooses the size of multi-resolution images, where as this is designed > to choose the best sized icon when more than one <link rel="icon"> is provided. > Assuming that the WebKit glue code also matches gfx::kFaviconSize, a > multi-resolution image should result in a matching image size. Sorry I never got back to you about this. The code I was thinking of is in http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebImageSkia.cpp in WebImage::fromData(). It sounds like it probably is not that useful to you.
Ah, thanks for the pointer, it's good to be aware of. The code is definitely similar, but different enough that we couldn't have used it directly. Thanks! -Steven On Tue, Mar 20, 2012 at 12:16 PM, <pkasting@chromium.org> wrote: > On 2012/03/15 00:55:34, stevenjb (chromium) wrote: > >> On 2012/03/14 22:37:28, Peter Kasting wrote: >> > We already have code in the WebKit glue layer that tries to pick the >> right >> size >> > image for a favicon (when provided with e.g. an ICO file that has >> multiple >> > subimages). Are you familiar with it? Can we implement what you want >> in >> terms >> > of that code? I would prefer to avoid code duplication if possible. >> > > I'm not familiar with that code, can you point me to it? >> I suspect that it will be orthogonal to this - it sounds like that code >> correctly chooses the size of multi-resolution images, where as this is >> > designed > >> to choose the best sized icon when more than one <link rel="icon"> is >> > provided. > >> Assuming that the WebKit glue code also matches gfx::kFaviconSize, a >> multi-resolution image should result in a matching image size. >> > > Sorry I never got back to you about this. The code I was thinking of is in > http://trac.webkit.org/**browser/trunk/Source/WebKit/** > chromium/src/WebImageSkia.cpp<http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/src/WebImageSkia.cpp> > in WebImage::fromData(). It sounds like it probably is not that useful to > you. > > http://codereview.chromium.**org/9696057/<http://codereview.chromium.org/9696... > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
