Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2347173002:
Extend FaviconService to support fetching favicons from a Google server (Closed)
|
Created:
4 years, 3 months ago by jkrcal Modified:
3 years, 8 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, browser-components-watch_chromium.org, droger+watchlist_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtend FaviconService to support fetching favicons from a Google server
Currently, the chrome client-side favicon service does not support
fetching favicons from any Google favicon server. Showing good favicons
independently of the local cache is important for synced content or for
content suggested by the server-side.
This CL adds this feature.
BUG=644102
Patch Set 1 #Patch Set 2 : Minor polish #
Total comments: 27
Patch Set 3 : First round of comments #
Total comments: 10
Patch Set 4 : Addressing some comments #
Total comments: 19
Patch Set 5 : Resizing the icons to desired_size #Patch Set 6 : Requiring minimum_size #
Total comments: 37
Patch Set 7 : Peter's comments #
Total comments: 15
Patch Set 8 : Peter's comments #2 #
Total comments: 9
Messages
Total messages: 33 (4 generated)
jkrcal@chromium.org changed reviewers: + noyau@chromium.org, pkotwicz@chromium.org, sclittle@chromium.org, treib@chromium.org
Peter, could you PTAL? Éric, could you PTAL at the iOS factory? Marc, could you PTAL at the image_fetcher dependency? Scott, could you PTAL at the data_use_measurement dependency? Thanks!
data_use_measurement dependency LGTM
lgtm https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:39: if (size > arbitrary_size) I think this should be >=
https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:39: if (size > arbitrary_size) On 2016/09/19 08:26:39, Marc Treib wrote: > I think this should be >= Definitely should be >=. I'm surprised that there is no unitttests whatsoever on this service. It probably need some... https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:422: favicon_base::IconType::FAVICON, image_result.image); The documentation for SetFavicons is very explicit: "It is important that |image| contains image reps for all of ui::GetSupportedScaleFactors()". This is not the case here, at least on iOS. The image is 1x, the supported scale factor is 2x or 3x. On Android, there could be more than one supported scale factor if the device is hi-dpi. Desktop always have more than one scale factor (1x and 2x). Also I'm sad that after all these efforts to fetch images directly as UIImage, those are saved as ImageSkia bitmap representation, incurring a transform on save and another on fetch. https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:171: int desired_size_in_pixel, I would prefer this API to have a desired size in point and screen scale instead of a size in pixel. This would express what's desired at the UI level better. Probably outside the scope of this particular CL, and this would need to be changed through the fetching API, but this would allow returning images setup with the proper scale to display on screen.
I will take a look at this CL today. Sorry for the delay
https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:232: std::string client_id, |client_id| seems unused. Do you need to add a TODO() https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:413: image_result.icon_url = GURL(); The "icon URL" is used as a key in the database. Use |domain_url| as the "icon URL" to get something unique. (Otherwise, StoreFaviconFromGoogleServiceAndRunFaviconImageCallback() will clobber whatever it has last stored in the database) https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:425: SetFaviconOutOfDateForPage(domain_url); - favicon_base::FAVICON is only meant to store 16x16 DIP (device independant pixel) favicons. The 16x16px favicons are used by sync. Perhaps these should be stored as favicon_base::TOUCH_ICON instead? I think that favicon_base::TOUCH_ICON is mobile only. Are we planning on using DownloadFromGoogleServerFaviconImageForPageURL() on non-mobile? - I am concerned that people will use DownloadFromGoogleServerFaviconImageForPageURL() and that will overwrite an existing favicon stored for the page. Maybe add a warning in the comment for DownloadFromGoogleServerFaviconImageForPageURL() ? For a given page URL the database only supports storing one icon per favicon_base::IconType The other FaviconService methods such as FaviconService#GetFaviconImageForPageURL() call the callback with the image resized to the pixel size requested in the caller. Is there a reason that this method does not resize the image to the pixel size passed to DownloadFromGoogleServerFaviconImageForPageURL() ? https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:144: Thank you for the clear and very informative function comment https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:153: // subdomain.foo.com/foo/bar -> subdomain.foo.com/). If no icon is available I think that ignoring the path should be done in the caller of this function not inside this function. From local testing, the empty path does not seem to be an API requirement. (I get different icons for www.google.com and www.google.com/maps) https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:171: int desired_size_in_pixel, Passing in the desired size in pixels matches the other methods in FaviconService. noyau@: I share your concern about making sure we fetch the right thing for retina iOS devices.
Thanks for all the helpful comments! PTAL, again. https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:39: if (size > arbitrary_size) On 2016/09/19 08:26:39, Marc Treib wrote: > I think this should be >= Done. https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:232: std::string client_id, On 2016/09/19 20:07:51, pkotwicz wrote: > |client_id| seems unused. Do you need to add a TODO() Oh, my mistake, thanks! Used now. https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:413: image_result.icon_url = GURL(); On 2016/09/19 20:07:51, pkotwicz wrote: > The "icon URL" is used as a key in the database. Use |domain_url| as the "icon > URL" to get something unique. (Otherwise, > StoreFaviconFromGoogleServiceAndRunFaviconImageCallback() will clobber whatever > it has last stored in the database) I do not understand what you mean. Maybe I puzzled you by the missing "return" statement? This image_result.icon_url only returns back to the caller, does not go into the database. Still, it maybe makes sense to fill in the used icon_url. https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:422: favicon_base::IconType::FAVICON, image_result.image); On 2016/09/19 09:00:50, noyau wrote: > The documentation for SetFavicons is very explicit: "It is important that > |image| contains image reps for all of ui::GetSupportedScaleFactors()". This is > not the case here, at least on iOS. The image is 1x, the supported scale factor > is 2x or 3x. On Android, there could be more than one supported scale factor if > the device is hi-dpi. Desktop always have more than one scale factor (1x and > 2x). I am not sure how to address your comment. - Do you oppose the idea that this function fetches only one image? (which is important as regards data usage for users from emerging markets) - Do oppose the way I call SetFavicons with this one image? If so, would you prefer me: * getting the raw png data, * fill it in a favicon_base::FaviconRawBitmapResult raw_image, and * call favicon_base::SelectFaviconFramesFromPNGs( std::vector<favicon_base::FaviconRawBitmapResult>{raw_image}, favicon_base::GetFaviconScales(), 16)? > Also I'm sad that after all these efforts to fetch images directly as UIImage, > those are saved as ImageSkia bitmap representation, incurring a transform on > save and another on fetch. I do not get this comment. Does it target the SetFavicons API (and not my CL)? https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:425: SetFaviconOutOfDateForPage(domain_url); On 2016/09/19 20:07:51, pkotwicz wrote: > - favicon_base::FAVICON is only meant to store 16x16 DIP (device independant > pixel) favicons. The 16x16px favicons are used by sync. Perhaps these should be > stored as favicon_base::TOUCH_ICON instead? I think that > favicon_base::TOUCH_ICON is mobile only. Are we planning on using > DownloadFromGoogleServerFaviconImageForPageURL() on non-mobile? > - I am concerned that people will use > DownloadFromGoogleServerFaviconImageForPageURL() and that will overwrite an > existing favicon stored for the page. Maybe add a warning in the comment for > DownloadFromGoogleServerFaviconImageForPageURL() ? > > For a given page URL the database only supports storing one icon per > favicon_base::IconType > My observations: - If I correctly understand what does IconType::FAVICON mean, the Google server provides only these icons (possibly in 1x, 1.5x, 2x, 2.5x, and 3x scale factors). - This function is meant to be used only if the local cache does not contain any FAVICON images. I've added the warning about overwriting existing entries. - In the near future (say next 6 months), I see only mobile use of this function from our side. Still I do not think that icons that are conceptually of the FAVICON type should be stored as TOUCH_ICONs. > The other FaviconService methods such as > FaviconService#GetFaviconImageForPageURL() call the callback with the image > resized to the pixel size requested in the caller. Is there a reason that this > method does not resize the image to the pixel size passed to > DownloadFromGoogleServerFaviconImageForPageURL() ? Good point, I plan to change it (once I get more feedback above). https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:144: On 2016/09/19 20:07:51, pkotwicz wrote: > Thank you for the clear and very informative function comment Thanks :) https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:153: // subdomain.foo.com/foo/bar -> subdomain.foo.com/). If no icon is available On 2016/09/19 20:07:51, pkotwicz wrote: > I think that ignoring the path should be done in the caller of this function not > inside this function. From local testing, the empty path does not seem to be an > API requirement. (I get different icons for http://www.google.com and > http://www.google.com/maps) You are right, I've changed the |domain_url| param to |page_url|. https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:171: int desired_size_in_pixel, On 2016/09/19 20:07:51, pkotwicz wrote: > Passing in the desired size in pixels matches the other methods in > FaviconService. > > noyau@: I share your concern about making sure we fetch the right thing for > retina iOS devices. I would like to keep it consistent with the rest of the service. Anyway, Éric, would a change of the fetching API be necessary? If we know the favicon server provides only favicons, aren't all of them meant to be 16x16 DIP (with 1x, 1.5x, 2x, 2.5x and 3x scale factors)?
https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:422: favicon_base::IconType::FAVICON, image_result.image); On 2016/09/20 15:52:06, jkrcal wrote: > On 2016/09/19 09:00:50, noyau wrote: > > The documentation for SetFavicons is very explicit: "It is important that > > |image| contains image reps for all of ui::GetSupportedScaleFactors()". This > is > > not the case here, at least on iOS. The image is 1x, the supported scale > factor > > is 2x or 3x. On Android, there could be more than one supported scale factor > if > > the device is hi-dpi. Desktop always have more than one scale factor (1x and > > 2x). > > I am not sure how to address your comment. > - Do you oppose the idea that this function fetches only one image? (which is > important as regards data usage for users from emerging markets) > > - Do oppose the way I call SetFavicons with this one image? If so, would you > prefer me: > * getting the raw png data, > * fill it in a favicon_base::FaviconRawBitmapResult raw_image, and > * call favicon_base::SelectFaviconFramesFromPNGs( > std::vector<favicon_base::FaviconRawBitmapResult>{raw_image}, > favicon_base::GetFaviconScales(), > 16)? > I'm not sure how to solve the issue, pkotwicz@ is more qualified than me to provide an answer to this, or to tell you that your proposal is fine. I was simply pointing out that your use of SetFavicons seems to go against the spirit of it as explained in its comment. > > Also I'm sad that after all these efforts to fetch images directly as UIImage, > > those are saved as ImageSkia bitmap representation, incurring a transform on > > save and another on fetch. > > I do not get this comment. Does it target the SetFavicons API (and not my CL)? No, I'm just sad that this callback is going to receive a gfx::Image with an UIImage as its underlying represnetation and that the code saving the image will generate a bitmap from it to save to disk. The only way to fix this would be to have the favicon save its data in a platform specific way, and this is completely ouside the scope of this CL. But still, this makes me sad. https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:171: int desired_size_in_pixel, On 2016/09/20 15:52:06, jkrcal wrote: > On 2016/09/19 20:07:51, pkotwicz wrote: > > Passing in the desired size in pixels matches the other methods in > > FaviconService. > > > > noyau@: I share your concern about making sure we fetch the right thing for > > retina iOS devices. > > I would like to keep it consistent with the rest of the service. > > Anyway, Éric, would a change of the fetching API be necessary? If we know the > favicon server provides only favicons, aren't all of them meant to be 16x16 DIP > (with 1x, 1.5x, 2x, 2.5x and 3x scale factors)? I thought the servers was only serving 1x images. Am I mistaken?
A few more comments I played around with the API https://s2.googleusercontent.com/s2/favicons?domain=https://www.wikipedia.org... (These are not really more review comments, but rather for my own general knowledge) As you said, apple-touch-icon and apple-touch-icon-precomposed icons are ignored These things surprised me: - https://www.reddit.com has two favicons (type="icon", type="shortcut icon"). http://www.redditstatic.com/favicon.ico http://www.redditstatic.com/icon.png https://s2.googleusercontent.com/s2/favicons?domain=https://www.reddit.com&sz... seems to always pick http://www.redditstatic.com/icon.png regardless of the value of the "sz" query parameter. - https://s2.googleusercontent.com/s2/favicons?domain=https://www.abc.com&sz=25... results in a 404. I am surprised because www.abc.com is a popular website https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:413: image_result.icon_url = GURL(); Sorry my bad. I did not realize that |icon_url| is passed into this function https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:422: favicon_base::IconType::FAVICON, image_result.image); noyau: Not having image reps for all of ui::GetSupportedScaleFactors() should be OK since the icon is stored as expired https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:425: SetFaviconOutOfDateForPage(domain_url); We only sync favicons for bookmarks. It is possible (but unlikely) that: - One of the most-visited sites on the NTP is bookmarked - We do not have a favicon for the bookmarked page - There is an icon for the site in gstatic In StoreFaviconFromGoogleServiceAndRunFaviconImageCallback() I would recommend: - constructing a multi-resolution gfx::ImageSkia with: - the fetched bitmap - the fetched bitmap resized to 16x16 Ideally you would download both the |desired_size_in_pixel|x|desired_size_in_pixel| and the 16x16 images from the Google server. However, not doing so should be OK because you are setting the stored favicon as expired (For the sake of clarity I still think that you should construct a multi-resolution gfx::ImageSkia) Storing icons with the FAVICON type should be ok. https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.cc:250: image_fetcher_->StartOrQueueNetworkRequest( I think that ImageFetcherImpl::StartOrQueueNetworkRequest() does an HTTP GET request. I assume that if someone malicious: - used one of Chrome's client IDs (The client IDs will not be in the internal portion of Chrome?) - does millions of HTTP GET requests That this would not affect Chrome's legitimate requests?
Description was changed from ========== Extend FaviconService to support fetching favicons from a Google server Currently, the chrome client-side favicon service does not support fetching favicons from any Google favicon server. Showing good favicons independently of the local cache is important for synced content or for content suggested by the server-side. This CL adds this feature. BUG=644102 ========== to ========== Extend FaviconService to support fetching favicons from a Google server Currently, the chrome client-side favicon service does not support fetching favicons from any Google favicon server. Showing good favicons independently of the local cache is important for synced content or for content suggested by the server-side. This CL adds this feature. BUG=644102 ==========
noyau@chromium.org changed reviewers: + stkhapugin@chromium.org
drive-by. Thank you for exceptional documenting comments. https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.h:147: // 32, 48, and 64 are supported and only FAVICON IconType is supported. If you I know this may not be the best place to ask, but on iOS we use up to 144x144 icons. Can we request higher-resolution favicons somehow? https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.h:168: void DownloadFromGoogleServerFaviconImageForPageURL( It looks like every other async call here returns a TaskId to cancel unnecessary async operations. Can you make this cancellable as well?
https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.cc:415: if (image_result.image.IsEmpty()) { Can you move this if to line 412 I am suggesting: if (image.IsEmpty()) { callback.Run(base::FaviconImageResult()); } favicon base::FaviconImageResult image_result; ...
The CL is not complete. Submitting now to be able to ask questions to pkotwicz@. Peter, could you PTAL? https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:413: image_result.icon_url = GURL(); On 2016/09/21 20:55:53, pkotwicz wrote: > Sorry my bad. I did not realize that |icon_url| is passed into this function No problem :) https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:422: favicon_base::IconType::FAVICON, image_result.image); On 2016/09/21 20:55:53, pkotwicz wrote: > noyau: Not having image reps for all of ui::GetSupportedScaleFactors() should be > OK since the icon is stored as expired Acknowledged. https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.cc:425: SetFaviconOutOfDateForPage(domain_url); On 2016/09/21 20:55:53, pkotwicz wrote: > We only sync favicons for bookmarks. It is possible (but unlikely) that: > - One of the most-visited sites on the NTP is bookmarked > - We do not have a favicon for the bookmarked page > - There is an icon for the site in gstatic > > In StoreFaviconFromGoogleServiceAndRunFaviconImageCallback() I would recommend: > - constructing a multi-resolution gfx::ImageSkia with: > - the fetched bitmap > - the fetched bitmap resized to 16x16 > Ideally you would download both the > |desired_size_in_pixel|x|desired_size_in_pixel| and the 16x16 images from the > Google server. However, not doing so should be OK because you are setting the > stored favicon as expired (For the sake of clarity I still think that you should > construct a multi-resolution gfx::ImageSkia) > > Storing icons with the FAVICON type should be ok. As per offline discussion with pkotwicz@, only one size is ok. https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/20001/components/favicon/core... components/favicon/core/favicon_service.h:171: int desired_size_in_pixel, On 2016/09/21 09:49:20, noyau wrote: > On 2016/09/20 15:52:06, jkrcal wrote: > > On 2016/09/19 20:07:51, pkotwicz wrote: > > > Passing in the desired size in pixels matches the other methods in > > > FaviconService. > > > > > > noyau@: I share your concern about making sure we fetch the right thing for > > > retina iOS devices. > > > > I would like to keep it consistent with the rest of the service. > > > > Anyway, Éric, would a change of the fetching API be necessary? If we know the > > favicon server provides only favicons, aren't all of them meant to be 16x16 > DIP > > (with 1x, 1.5x, 2x, 2.5x and 3x scale factors)? > > I thought the servers was only serving 1x images. Am I mistaken? All the images are favicons with 16 dip. They serve different sizes and thus different scales (1x, 1.5x, 2x, 3x, 4x). https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.cc:250: image_fetcher_->StartOrQueueNetworkRequest( On 2016/09/21 20:55:53, pkotwicz wrote: > I think that ImageFetcherImpl::StartOrQueueNetworkRequest() does an HTTP GET > request. > > I assume that if someone malicious: > - used one of Chrome's client IDs (The client IDs will not be in the internal > portion of Chrome?) > - does millions of HTTP GET requests > That this would not affect Chrome's legitimate requests? This would probably affect Chrome's legitimate requests. None of the both favicon services teams I am in contact with seem to be too concerned about having such a public API. The client ID is just for basic understanding of the traffic. Do you see any other reasonably simple mechanism that - would work also for non-signed-in users and - would not be prone to such simple attacks? https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.cc:415: if (image_result.image.IsEmpty()) { On 2016/12/07 04:31:22, pkotwicz wrote: > Can you move this if to line 412 > > I am suggesting: > if (image.IsEmpty()) { > callback.Run(base::FaviconImageResult()); > } > favicon base::FaviconImageResult image_result; > ... Sure. It's much better this way! I was not sure what is the contract (whether the icon_url can be empty). https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.h:147: // 32, 48, and 64 are supported and only FAVICON IconType is supported. If you On 2016/09/28 16:12:18, stkhapugin wrote: > I know this may not be the best place to ask, but on iOS we use up to 144x144 > icons. Can we request higher-resolution favicons somehow? Having another function for fetching larger APPLE_TOUCH_ICONS will be part of follow-up work (when we move to the new favicon service mentioned above). https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.h:168: void DownloadFromGoogleServerFaviconImageForPageURL( On 2016/09/28 16:12:18, stkhapugin wrote: > It looks like every other async call here returns a TaskId to cancel unnecessary > async operations. Can you make this cancellable as well? Done. https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:420: // simplest path? Or should I do it similarly to GetLargestRawFaviconForID()? Peter, can you help me with this, please? https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:469: // SelectFaviconFramesFromPNGs(). Peter, can you help me with this, please? https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.h:178: GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded( Peter, in an offline discussion, you've suggested the name GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded. Why "Raw" when it returns an gfx::Image? (and not RawData)
Jan, I think I responded to all of your questions. Let me know if you have more questions! https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.cc:250: image_fetcher_->StartOrQueueNetworkRequest( I don't know of a mechanism which would not be prone to simple attacks. (I am no security expert though) I don't understand why we need a client ID at all (other than for one Google app to affect another Google app's usage of the service). In the feature that I work on (WebAPKs) we use an API key as well. I don't understand why we are using the API key in the case of WebAPKs either The API key is probably OK because it is "the Google Way" https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:420: // simplest path? Or should I do it similarly to GetLargestRawFaviconForID()? Things get complicated if you want to download a favicon from the google service if there is a favicon in the database but it is smaller than |minimum_size_in_pixels| For instance, you need to worry about whether you want to store the downloaded favicon into the database. If yes, you cannot clobber the existing data These things can be solved but over the course of many CLs not one CL I suggest doing the download only if there is nothing in the database https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:469: // SelectFaviconFramesFromPNGs(). My understanding is that you are currently not rescaling to |desired_size_in_px|. Do you want to rescale to |desired_size_in_px|? https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.h:178: GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded( The goal of this function is to get a single resolution bitmap as a opposed to a multi resolution gfx::Image. We currently return single resolution bitmaps as base::FaviconRawBitmapResult I need to think some more about what the best function name would be. I understand why you are currently using FaviconImageResult https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.h:321: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_; Is the security team OK with decoding images from the web in the browser process? The security team has been uncomfortable with this in the past. (Hence the existance of WebContents::DownloadImage()) If they are ok with this, that's great but I would love to know the reasoning for this change in policy
Thanks, Peter! Follow-up questions below. PTAL. https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/40001/components/favicon/core... components/favicon/core/favicon_service.cc:250: image_fetcher_->StartOrQueueNetworkRequest( On 2016/12/09 19:46:30, pkotwicz wrote: > I don't know of a mechanism which would not be prone to simple attacks. (I am no > security expert though) > > I don't understand why we need a client ID at all (other than for one Google app > to affect another Google app's usage of the service). > > In the feature that I work on (WebAPKs) we use an API key as well. I don't > understand why we are using the API key in the case of WebAPKs either > > The API key is probably OK because it is "the Google Way" Acknowledged. https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:420: // simplest path? Or should I do it similarly to GetLargestRawFaviconForID()? On 2016/12/09 19:46:30, pkotwicz wrote: > Things get complicated if you want to download a favicon from the google service > if there is a favicon in the database but it is smaller than > |minimum_size_in_pixels| I do not want to download anything in such a case. The expected behaviour is documented in the .h file. > For instance, you need to worry about whether you want to store the downloaded > favicon into the database. If yes, you cannot clobber the existing data > These things can be solved but over the course of many CLs not one CL > > I suggest doing the download only if there is nothing in the database I still would like to find out what is the size of the largest cached icon. - If it does not meet the minimum requested size, the function will return an empty gfx::Image(). - If it meets the minimum requested size, it should return this cached favicon scaled to desired_size. How can I find out the largest size with duplicating much of work? https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:469: // SelectFaviconFramesFromPNGs(). On 2016/12/09 19:46:30, pkotwicz wrote: > My understanding is that you are currently not rescaling to > |desired_size_in_px|. Do you want to rescale to |desired_size_in_px|? > Sorry, the comment is a bit unclear :) Yes, I would like to rescale to desired size for the sake of consistency. (Do I get it right that other similar functions in FaviconService do rescale to the desired size?) https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.h:178: GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded( On 2016/12/09 19:46:30, pkotwicz wrote: > The goal of this function is to get a single resolution bitmap as a opposed to a > multi resolution gfx::Image. We currently return single resolution bitmaps as > base::FaviconRawBitmapResult > > I need to think some more about what the best function name would be. I > understand why you are currently using FaviconImageResult Okay, thanks. Will keep it as it is, rename it in later revisions. https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.h:321: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_; On 2016/12/09 19:46:30, pkotwicz wrote: > Is the security team OK with decoding images from the web in the browser > process? The security team has been uncomfortable with this in the past. (Hence > the existance of WebContents::DownloadImage()) > > If they are ok with this, that's great but I would love to know the reasoning > for this change in policy componentes/image_fetcher uses an isolated process for decoding the images (applying https://cs.chromium.org/chromium/src/chrome/browser/image_decoder.h).
I responded to your questions. As a heads up I will be out of the office starting next Tuesday and will be back on Jan 9. I will try to answer all of the questions before I leave (and hopefully quicker too. I don't have any more interviews till the holidays!) https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:420: // simplest path? Or should I do it similarly to GetLargestRawFaviconForID()? I recomment adding FaviconRawBitmapResult::width_in_db and FaviconRawBitmapResult::height_in_db to the FaviconRawBitmapResult struct https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:469: // SelectFaviconFramesFromPNGs(). You are right. Similar functions in FaviconService do rescale to the desired size. Looking at image_decoder_impl it looks like the gfx::Image is created from a single SkBitmap. You can get the SkBitmap via gfx::Image::AsBitmap() and then call ResizeBitmapByDownsamplingIfPossible() in favicon_util.cc The function is currently in the anonymous namespace but you can move it out https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.h:321: std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher_; You are right. I see now :)
Peter, thanks! One question hopefully resolved. For the other, I still have a follow-up question. PTAL! https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:420: // simplest path? Or should I do it similarly to GetLargestRawFaviconForID()? On 2016/12/15 01:22:27, pkotwicz wrote: > I recomment adding FaviconRawBitmapResult::width_in_db and > FaviconRawBitmapResult::height_in_db to the FaviconRawBitmapResult struct Thanks for the hint! When looking in the code, I identified that these new fields need to be set in HistoryBackend::GetFaviconBitmapResultsForBestMatch() and in HistoryBackend::GetLargestFaviconForURL(). Does it SG? More importantly, I do not understand what is the difference between FaviconRawBitmapResult::width/height_in_db and FaviconRawBitmapResult::pixel_size. Why cannot I use the latter to find out what is the size of the largest icon available? https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:469: // SelectFaviconFramesFromPNGs(). On 2016/12/15 01:22:27, pkotwicz wrote: > You are right. Similar functions in FaviconService do rescale to the desired > size. > > Looking at image_decoder_impl it looks like the gfx::Image is created from a > single SkBitmap. > > You can get the SkBitmap via gfx::Image::AsBitmap() and then call > ResizeBitmapByDownsamplingIfPossible() in favicon_util.cc The function is > currently in the anonymous namespace but you can move it out Done, thanks!
https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:420: // simplest path? Or should I do it similarly to GetLargestRawFaviconForID()? For the sake of clarity, I recommend adding width_in_db and height_in_db not renaming/replacing FaviconRawBitmapResult::width and FaviconRawBitmapResult::height https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:420: // simplest path? Or should I do it similarly to GetLargestRawFaviconForID()? Sorry for the confusion. FaviconRawBitmapResult::pixel_size contains the size of the bitmap after it was resized by FaviconService. We can add FaviconRawBitmapResult::pixel_size_in_db which would contain the size of the bitmap before it was resized by HistoryBackend::GetFaviconsForURL(). FaviconRawBitmapResult::pixel_size_in_db might be different than the favicon's size on the server because we store favicons into the database after resizing. This should be ok because we either: store the bitmap without resizing (Android) or store the bitmap after resizing to 32x32 and 16x16 (desktop). If the minimum size is > 32x32 we are guaranteed that the matching favicon would not have been resized I am not sure, but we might be able to replace all uses of FaviconRawBitmapResult::pixel_size with FaviconRawBitmapResult::pixel_size_in_db
Thanks, Peter! PTAL, again! Did you come up with any better name for the function? https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/60001/components/favicon/core... components/favicon/core/favicon_service.cc:420: // simplest path? Or should I do it similarly to GetLargestRawFaviconForID()? On 2016/12/15 18:43:57, pkotwicz wrote: > Sorry for the confusion. > > FaviconRawBitmapResult::pixel_size contains the size of the bitmap after it was > resized by FaviconService. > > We can add FaviconRawBitmapResult::pixel_size_in_db which would contain the size > of the bitmap before it was resized by HistoryBackend::GetFaviconsForURL(). > > FaviconRawBitmapResult::pixel_size_in_db might be different than the favicon's > size on the server because we store favicons into the database after resizing. > This should be ok because we either: store the bitmap without resizing (Android) > or store the bitmap after resizing to 32x32 and 16x16 (desktop). If the minimum > size is > 32x32 we are guaranteed that the matching favicon would not have been > resized > > I am not sure, but we might be able to replace all uses of > FaviconRawBitmapResult::pixel_size with FaviconRawBitmapResult::pixel_size_in_db Thanks for the clarification! I did not want to mess it up, so I rather introduced the new member. Should I add a "TODO(pkotwicz, jkrcal): Consider removing |pixel_size| and replacing all usage by |pixel_size_in_db|"?
Mostly nits Yes, you should add a TODO to remove FaviconRawBitmapResult::pixel_size and file a bug for the task https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... File chrome/browser/favicon/favicon_service_factory.cc (right): https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... chrome/browser/favicon/favicon_service_factory.cc:32: profile->GetRequestContext())); Is ImageFetcherImpl the new hot thing? ContentFaviconDriver uses WebContents::DownloadImage(). It is kind of confusing for the FaviconService to use both. It looks like ImageFetcherImpl is not ready to be used in ContentFaviconDriver::StartDownload() because it does not decode all of the frames from a .ico file https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:36: Perhaps GetClosestGoogleFaviconServerSupportedSize() is clearer https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:47: How about: GetGoogleFaviconServerURLForPageURL() https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:234: FaviconService::GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded( We should add a TODO to switch FaviconHelper#ensureIconIsAvailable() to use this instead https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:234: FaviconService::GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded( How about Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() This name is not like the other names in the FaviconService interface but it is not misleading - It does not have RawRavicon as a substring. This is good because it does not use a FaviconRawBitmapResult callback - It makes it explicit that the return value is single resolution https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:418: "FaviconService::DownloadFromGoogleServerFaviconImageForPageURL"); Shouldn't the second argument be "FaviconService::RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer" https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:423: for (const auto& bitmap : favicon_bitmap_results) { Can you make |max_size_in_pixel| a boolean and early return once you got a match? https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:433: base::ThreadTaskRunnerHandle::Get()->PostTask( What's the reason for posting a task? It looks like only RunWithEmptyResultAsync() posts a task in this class. RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer() is not called synchronously from GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded() so I don't think you need to post a task https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:440: favicon_bitmap_results, You should set the desired scale to be 1.0f and not do anything fancy with |desired_size_in_pixel| https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:456: if (!image_fetcher_.get()) { You don't need to post a task https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:479: void FaviconService::StoreFaviconFromGoogleServiceAndRunFaviconImageCallback( How about: StoreFaviconFromGoogleServerAndRunFaviconImageCallback https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:488: // Resize the icon to the desired size. Nuke this comment. What ResizeBitmapByDownsamplingIfPossible() does is obvious from its name https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:497: // Store the favicon in the cache. Nuke this comment. What the function does should be obvious from its name. If it isn't we should change the name. I understand that it currently isn't obvious that SetFavicons() store bitmaps into the cache. I am not suggesting that you rename SetFavicons() in this CL https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:500: // Mark them as out-of-date so that they are refetched when we visit the Nit: "them" -> "the icons" https://codereview.chromium.org/2347173002/diff/100001/components/favicon_bas... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/2347173002/diff/100001/components/favicon_bas... components/favicon_base/favicon_types.h:72: // size. How about: "The pixel dimensions of the bitmap stored in the database. Icons are resized prior to storing into the database so the dimensions are different than those of the bitmap at |icon_url|. The dimensions are taken prior to the resizing in FaviconService::Get###()." https://codereview.chromium.org/2347173002/diff/100001/components/favicon_bas... File components/favicon_base/favicon_util.cc (right): https://codereview.chromium.org/2347173002/diff/100001/components/favicon_bas... components/favicon_base/favicon_util.cc:247: int desired_size) { Now that this function is not in the anonymous namespace, you need to handle |input_bitmaps| being empty. You can still dcheck that |desired_size| != 0 https://codereview.chromium.org/2347173002/diff/100001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2347173002/diff/100001/components/history/cor... components/history/core/browser/history_backend.cc:1562: *bitmap_results, desired_sizes[0])); You need to update ResizeFaviconBitmapResult() in order to set the correct pixel_size_in_db You should add tests too :)
Thanks, Peter. PTAL, again! https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... File chrome/browser/favicon/favicon_service_factory.cc (right): https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... chrome/browser/favicon/favicon_service_factory.cc:32: profile->GetRequestContext())); On 2016/12/19 00:03:28, pkotwicz wrote: > Is ImageFetcherImpl the new hot thing? ContentFaviconDriver uses > WebContents::DownloadImage(). It is kind of confusing for the FaviconService to > use both. > > It looks like ImageFetcherImpl is not ready to be used in > ContentFaviconDriver::StartDownload() because it does not decode all of the > frames from a .ico file I need to ask colleagues whether image fetcher has any advantage over WebContents::DownloadImage(). Would you like me to convert my new code to WebContents::DownloadImage()? https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:36: On 2016/12/19 00:03:28, pkotwicz wrote: > Perhaps GetClosestGoogleFaviconServerSupportedSize() is clearer Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:47: On 2016/12/19 00:03:28, pkotwicz wrote: > How about: GetGoogleFaviconServerURLForPageURL() Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:234: FaviconService::GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded( On 2016/12/19 00:03:28, pkotwicz wrote: > How about Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() > > This name is not like the other names in the FaviconService interface but it is > not misleading > - It does not have RawRavicon as a substring. This is good because it does not > use a FaviconRawBitmapResult callback > - It makes it explicit that the return value is single resolution Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:418: "FaviconService::DownloadFromGoogleServerFaviconImageForPageURL"); On 2016/12/19 00:03:28, pkotwicz wrote: > Shouldn't the second argument be > "FaviconService::RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer" Sure, thanks! https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:423: for (const auto& bitmap : favicon_bitmap_results) { On 2016/12/19 00:03:28, pkotwicz wrote: > Can you make |max_size_in_pixel| a boolean and early return once you got a > match? Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:433: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/12/19 00:03:28, pkotwicz wrote: > What's the reason for posting a task? It looks like only > RunWithEmptyResultAsync() posts a task in this class. > > RunFaviconImageCallbackOrDownloadFromGoogleFaviconServer() is not called > synchronously from GetRawFaviconForPageURLDownloadFromGoogleServerIfNeeded() so > I don't think you need to post a task Ah, sure, thanks! https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:440: favicon_bitmap_results, On 2016/12/19 00:03:28, pkotwicz wrote: > You should set the desired scale to be 1.0f and not do anything fancy with > |desired_size_in_pixel| Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:456: if (!image_fetcher_.get()) { On 2016/12/19 00:03:28, pkotwicz wrote: > You don't need to post a task Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:479: void FaviconService::StoreFaviconFromGoogleServiceAndRunFaviconImageCallback( On 2016/12/19 00:03:28, pkotwicz wrote: > How about: StoreFaviconFromGoogleServerAndRunFaviconImageCallback Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:488: // Resize the icon to the desired size. On 2016/12/19 00:03:28, pkotwicz wrote: > Nuke this comment. What ResizeBitmapByDownsamplingIfPossible() does is obvious > from its name Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_service.cc:497: // Store the favicon in the cache. On 2016/12/19 00:03:28, pkotwicz wrote: > Nuke this comment. What the function does should be obvious from its name. If it > isn't we should change the name. > > I understand that it currently isn't obvious that SetFavicons() store bitmaps > into the cache. > > I am not suggesting that you rename SetFavicons() in this CL Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon_bas... File components/favicon_base/favicon_types.h (right): https://codereview.chromium.org/2347173002/diff/100001/components/favicon_bas... components/favicon_base/favicon_types.h:72: // size. On 2016/12/19 00:03:29, pkotwicz wrote: > How about: "The pixel dimensions of the bitmap stored in the database. Icons are > resized prior to storing into the database so the dimensions are different than > those of the bitmap at |icon_url|. The dimensions are taken prior to the > resizing in FaviconService::Get###()." Done. https://codereview.chromium.org/2347173002/diff/100001/components/favicon_bas... File components/favicon_base/favicon_util.cc (right): https://codereview.chromium.org/2347173002/diff/100001/components/favicon_bas... components/favicon_base/favicon_util.cc:247: int desired_size) { On 2016/12/19 00:03:29, pkotwicz wrote: > Now that this function is not in the anonymous namespace, you need to handle > |input_bitmaps| being empty. > > You can still dcheck that |desired_size| != 0 Done. https://codereview.chromium.org/2347173002/diff/100001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2347173002/diff/100001/components/history/cor... components/history/core/browser/history_backend.cc:1562: *bitmap_results, desired_sizes[0])); On 2016/12/19 00:03:29, pkotwicz wrote: > You need to update ResizeFaviconBitmapResult() in order to set the correct > pixel_size_in_db > > You should add tests too :) Isn't the struct copied in favicon_util.cc:209 incl. the pixel_size_in_db? Later, the function only overwrites pixel_size which seems okay to me. Will add unit-tests, in a later patch set.
Your CL looks mostly good by me. I don't have comments other the ones that I have already posted. I am going on vacation starting tomorrow and will be back on Jan 9th. sky@ is also an OWNER of the favicon code. From his calendar, it looks like he is back from vacation on Jan 3rd. Thank you very much for bearing with me https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... File chrome/browser/favicon/favicon_service_factory.cc (right): https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... chrome/browser/favicon/favicon_service_factory.cc:32: profile->GetRequestContext())); If I were you I would wait to get feedback from your colleagues first. If ImageFetcherImpl is the new hotness, I am all for using it. If there is no preference between the two, I suggest using WebContents::DownloadImage() for the sake of consistency https://codereview.chromium.org/2347173002/diff/100001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2347173002/diff/100001/components/history/cor... components/history/core/browser/history_backend.cc:1562: *bitmap_results, desired_sizes[0])); You are right ResizeFaviconBitmapResult() does a copy. I am very sorry about the confusion. https://codereview.chromium.org/2347173002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/favicon/FaviconHelper.java (right): https://codereview.chromium.org/2347173002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/favicon/FaviconHelper.java:128: * FaviconService::Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing. Super nit: I think that it makes more sense to have the comment in chrome/browser/android/favicon_helper.cc It is slightly weird to refer to a C++ file from Java code https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.cc:38: // Take the smallest size larger than arbitrary_size. Nit: arbitrary_size -> |arbitrary_size| https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.cc:236: int minimum_size_in_pixel, Aside: Sorry I did not think about this before. If you make this function use a new type of callback favicon_base::FaviconBitmapCallback struct FaviconBitmapResult { SkBitmap bitmap; gfx::Size pixel_size_in_db; GURL icon_url; IconType icon_type; }; You can remove the |minimum_size_in_pixel| argument. The caller will check FaviconBitmapResult::pixel_size_in_db to detect whether the bitmap in the callback was upscaled from a really small size. I would hold off on making this change till you get sky@'s comments. He might not think that my proposal is a good idea https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.cc:415: favicon_bitmap_results) { You need to pass CancelableTaskTracker as an argument too. The callback to Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() should not be called if CancelableTaskTracker::TryCancel() is called https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.h:144: Your function comment should not start with "if". I believe the recommendation is to start function comments with a verb. In the .cc file, I discuss that it might be possible to remove the |minimum_size_in_pixel| parameter from this function. This affects the function comment. Once you get sky@'s feedback about |minimum_size_in_pixel|, I will try to suggest an improved function comment. (It is mean for me to tell you that your function comment should be improved but not to tell you how) https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.h:165: // If the request is successful, it stores the resulting favicon in the cache Nit: "cache" -> "history database" https://codereview.chromium.org/2347173002/diff/120001/components/favicon_bas... File components/favicon_base/favicon_util.h (right): https://codereview.chromium.org/2347173002/diff/120001/components/favicon_bas... components/favicon_base/favicon_util.h:48: // Returns a resampled bitmap of |desired_size| x |desired_size| by resampling Nit: |desired_size|x|desired_size| (remove the spaces)
jkrcal@chromium.org changed reviewers: + sky@chromium.org
Peter, I finally managed to get back to the CL. Could you please take another look (mostly at my comments)? Scott, there are questions to you (by Peter), could you PTAL? https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... File chrome/browser/favicon/favicon_service_factory.cc (right): https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... chrome/browser/favicon/favicon_service_factory.cc:32: profile->GetRequestContext())); On 2016/12/20 02:33:01, pkotwicz wrote: > If I were you I would wait to get feedback from your colleagues first. > > If ImageFetcherImpl is the new hotness, I am all for using it. If there is no > preference between the two, I suggest using WebContents::DownloadImage() for the > sake of consistency Indeed, the ImageFetcherImpl is not ready to decode all frames from .ico files. So at the moment, we cannot implement fetching images in FaviconDriverImpl generally using ImageFetcherImpl. Anyway, after taking another look, I am not convinced about using WebContents::DownloadImage() here. Before my CL, FaviconService never initiated downloading images on its own. WebContents::DownloadImage() is currently called from FaviconDriver (ContentFaviconDriver, in particular). It does not feel correct for the FaviconService to ask the FaviconDriver to do the job because: - FaviconDriver is conceptually bound to a tab (being a TabHelper) whereas FaviconService::Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() is conceptually tab-independent (as we can ask for icons from pages that are not loaded in any open tab). - FaviconService has no reference to any FaviconDriver instance and it does not feel correct to add this dependency. WDYT, Peter? https://codereview.chromium.org/2347173002/diff/100001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/2347173002/diff/100001/components/history/cor... components/history/core/browser/history_backend.cc:1562: *bitmap_results, desired_sizes[0])); On 2016/12/20 02:33:01, pkotwicz wrote: > You are right ResizeFaviconBitmapResult() does a copy. I am very sorry about the > confusion. Added a simple unit-test. https://codereview.chromium.org/2347173002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/favicon/FaviconHelper.java (right): https://codereview.chromium.org/2347173002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/favicon/FaviconHelper.java:128: * FaviconService::Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing. On 2016/12/20 02:33:01, pkotwicz wrote: > Super nit: I think that it makes more sense to have the comment in > chrome/browser/android/favicon_helper.cc It is slightly weird to refer to a C++ > file from Java code Done. https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.cc:38: // Take the smallest size larger than arbitrary_size. On 2016/12/20 02:33:01, pkotwicz wrote: > Nit: arbitrary_size -> |arbitrary_size| Done. https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.cc:236: int minimum_size_in_pixel, On 2016/12/20 02:33:02, pkotwicz wrote: > Aside: Sorry I did not think about this before. If you make this function use a > new type of callback favicon_base::FaviconBitmapCallback > > struct FaviconBitmapResult { > SkBitmap bitmap; > gfx::Size pixel_size_in_db; > GURL icon_url; > IconType icon_type; > }; > > You can remove the |minimum_size_in_pixel| argument. The caller will check > FaviconBitmapResult::pixel_size_in_db to detect whether the bitmap in the > callback was upscaled from a really small size. > > I would hold off on making this change till you get sky@'s comments. He might > not think that my proposal is a good idea Do you understand it correctly that you suggest that I introduce a new callback type and use it (solely for this function) instead of favicon_base::FaviconImageCallback? This sounds reasonable to me. Scott, what do you think? https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.cc:415: favicon_bitmap_results) { On 2016/12/20 02:33:01, pkotwicz wrote: > You need to pass CancelableTaskTracker as an argument too. The callback to > Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() should not be called > if CancelableTaskTracker::TryCancel() is called I see the problem. I've spent quite some time thinking about how to fix it. I am not sure that what you propose is the solution to the problem because: - I could only post a new task to the tracker which has a different id than the original id returned to the caller of Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() so the caller cannot cancel it anyway; and - it does not feel very safe to pass the tracker to this callback because I do not own it. From implementation details of the tracker it seems like the tracker must exist at the moment the reply is called but I am not sure that this must always stay like this. The core of the problem is that my task consists of two async subtasks run in a sequence. CancelableTaskTracker does not seem to be well-suited for such tasks. I see two suboptimal solutions: 1) Change the signature of Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing so that it does not support any canceling. 2) Create a wrapper task that wraps the two subtasks and post it from FaviconService to ThreadTaskRunnerHandle::Get(). This way, you are guaranteed that the reply is not called after calling TryCancel(). Still, cancelling the task while it is running will not have any impact on running the subtasks, both of them will be run. In both solutions I would need - either another CancelableTaskTracker owned by the favicon service that I pass to HistoryService (and never use for canceling anything), - or create an analogous function in the HistoryService that does not need any CancelableTaskTracker. WDYT? https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... File components/favicon/core/favicon_service.h (right): https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.h:144: On 2016/12/20 02:33:02, pkotwicz wrote: > Your function comment should not start with "if". I believe the recommendation > is to start function comments with a verb. > > In the .cc file, I discuss that it might be possible to remove the > |minimum_size_in_pixel| parameter from this function. This affects the function > comment. Once you get sky@'s feedback about |minimum_size_in_pixel|, I will try > to suggest an improved function comment. (It is mean for me to tell you that > your function comment should be improved but not to tell you how) Okay, let's postpone editing this comment until we get feedback from Scott. https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.h:165: // If the request is successful, it stores the resulting favicon in the cache On 2016/12/20 02:33:02, pkotwicz wrote: > Nit: "cache" -> "history database" Are you also fine with "thumbnail database" (mostly used in comments across this file)? https://codereview.chromium.org/2347173002/diff/120001/components/favicon_bas... File components/favicon_base/favicon_util.h (right): https://codereview.chromium.org/2347173002/diff/120001/components/favicon_bas... components/favicon_base/favicon_util.h:48: // Returns a resampled bitmap of |desired_size| x |desired_size| by resampling On 2016/12/20 02:33:02, pkotwicz wrote: > Nit: |desired_size|x|desired_size| (remove the spaces) Done. https://codereview.chromium.org/2347173002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/140001/components/favicon/cor... components/favicon/core/favicon_service.cc:427: break; This was obviously wrong :) https://codereview.chromium.org/2347173002/diff/140001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/favicon_service_factory.cc (right): https://codereview.chromium.org/2347173002/diff/140001/ios/chrome/browser/fav... ios/chrome/browser/favicon/favicon_service_factory.cc:59: base::MakeUnique<FaviconClientImpl>(), I am sorry, I've mixed in one rebase line.
Do you have a design doc you could point me at? If not a design doc, at least a general overview of where fetching icons from google comes in and if these icons are treated differently? Favicon fetching is tricky, and before jumping into the code I would like to understand where you want to go.
https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/120001/components/favicon/cor... components/favicon/core/favicon_service.cc:415: favicon_bitmap_results) { Solution #1 sounds ok to me. Especially since we likely aren't planning on cancelling calls to Get1xFaviconForPageURLDownloadFromGoogleServerIfMissing() yet
https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... File chrome/browser/favicon/favicon_service_factory.cc (right): https://codereview.chromium.org/2347173002/diff/100001/chrome/browser/favicon... chrome/browser/favicon/favicon_service_factory.cc:32: profile->GetRequestContext())); I am ok with the ImageFetcherImpl https://codereview.chromium.org/2347173002/diff/140001/components/favicon/cor... File components/favicon/core/favicon_service.cc (right): https://codereview.chromium.org/2347173002/diff/140001/components/favicon/cor... components/favicon/core/favicon_service.cc:431: // If there are some cached bitmaps but none is large enough, we cannot return Nit: 'is' -> 'are' https://codereview.chromium.org/2347173002/diff/140001/components/favicon/cor... components/favicon/core/favicon_service.cc:434: if (favicon_bitmap_results.size() > 0 && !large_enough) { Nit: !favicon_bitmap_results.empty() https://codereview.chromium.org/2347173002/diff/140001/components/favicon/cor... components/favicon/core/favicon_service.cc:462: // |minimum_size_in_pixel|. I think that it makes sense to enforce |minimum_size_in_pixel| here https://codereview.chromium.org/2347173002/diff/140001/components/history/cor... File components/history/core/browser/history_backend_unittest.cc (right): https://codereview.chromium.org/2347173002/diff/140001/components/history/cor... components/history/core/browser/history_backend_unittest.cc:3004: // the DB. How about: "Test that |FaviconBitmapResult::pixel_size_in_db| is populated prior to resizing the database bitmap data." - There are other tests which test selecting the best icons - There aren't any tests which test resizing the bitmap. However, the current implementation is so weird that I do not know if it is worth testing How about renaming the test to "GetFaviconsForURLPopulateSizeInDbPriorToResizing" https://codereview.chromium.org/2347173002/diff/140001/components/history/cor... components/history/core/browser/history_backend_unittest.cc:3012: bitmaps.push_back(CreateBitmap(SK_ColorRED, kLargeEdgeSize)); Given that you are not testing selecting the bitmap, you don't need multiple resolutions https://codereview.chromium.org/2347173002/diff/140001/components/history/cor... components/history/core/browser/history_backend_unittest.cc:3034: EXPECT_EQ(favicon_base::FAVICON, bitmap_results_out[0].icon_type); Testing the color, icon_url and type are not necessary in this test https://codereview.chromium.org/2347173002/diff/140001/ios/chrome/browser/fav... File ios/chrome/browser/favicon/favicon_service_factory.cc (right): https://codereview.chromium.org/2347173002/diff/140001/ios/chrome/browser/fav... ios/chrome/browser/favicon/favicon_service_factory.cc:10: #include "components/image_fetcher/image_decoder.h" Nit: I think you don't need this include
On 2017/02/06 22:29:24, sky wrote: > Do you have a design doc you could point me at? If not a design doc, at least a > general overview of where fetching icons from google comes in and if these icons > are treated differently? Favicon fetching is tricky, and before jumping into the > code I would like to understand where you want to go. Scott, while writing up the doc, we realized we probably want to do it slightly differently. I'll loop you in again when the code & the design-doc are ready (probably tomorrow or on Monday).
On 2017/02/09 20:38:04, jkrcal wrote: > On 2017/02/06 22:29:24, sky wrote: > > Do you have a design doc you could point me at? If not a design doc, at least > a > > general overview of where fetching icons from google comes in and if these > icons > > are treated differently? Favicon fetching is tricky, and before jumping into > the > > code I would like to understand where you want to go. > > Scott, while writing up the doc, we realized we probably want to do it slightly > differently. > I'll loop you in again when the code & the design-doc are ready (probably > tomorrow or on Monday). Scott, the design doc and the new CL are here, PTAL! go/favicon-pe-google-server https://codereview.chromium.org/2685173002/ Both is WIP -> please ask questions if anything is not clear enough!
I added a couple of comments to the doc. -Scott On Mon, Feb 13, 2017 at 6:38 AM, <jkrcal@chromium.org> wrote: > On 2017/02/09 20:38:04, jkrcal wrote: >> On 2017/02/06 22:29:24, sky wrote: >> > Do you have a design doc you could point me at? If not a design doc, at > least >> a >> > general overview of where fetching icons from google comes in and if >> > these >> icons >> > are treated differently? Favicon fetching is tricky, and before jumping >> > into >> the >> > code I would like to understand where you want to go. >> >> Scott, while writing up the doc, we realized we probably want to do it > slightly >> differently. >> I'll loop you in again when the code & the design-doc are ready (probably >> tomorrow or on Monday). > > Scott, the design doc and the new CL are here, PTAL! > go/favicon-pe-google-server > https://codereview.chromium.org/2685173002/ > Both is WIP -> please ask questions if anything is not clear enough! > > https://codereview.chromium.org/2347173002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks! I've expanded the text to address your comments. Jan On Tue, Feb 14, 2017 at 6:55 PM, Scott Violet <sky@chromium.org> wrote: > I added a couple of comments to the doc. > > -Scott > > On Mon, Feb 13, 2017 at 6:38 AM, <jkrcal@chromium.org> wrote: > > On 2017/02/09 20:38:04, jkrcal wrote: > >> On 2017/02/06 22:29:24, sky wrote: > >> > Do you have a design doc you could point me at? If not a design doc, > at > > least > >> a > >> > general overview of where fetching icons from google comes in and if > >> > these > >> icons > >> > are treated differently? Favicon fetching is tricky, and before > jumping > >> > into > >> the > >> > code I would like to understand where you want to go. > >> > >> Scott, while writing up the doc, we realized we probably want to do it > > slightly > >> differently. > >> I'll loop you in again when the code & the design-doc are ready > (probably > >> tomorrow or on Monday). > > > > Scott, the design doc and the new CL are here, PTAL! > > go/favicon-pe-google-server > > https://codereview.chromium.org/2685173002/ > > Both is WIP -> please ask questions if anything is not clear enough! > > > > https://codereview.chromium.org/2347173002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
