Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2739173002:
Always select best favicon bitmap (Closed)
DescriptionAlways select best favicon bitmap
We do this by always sorting candidates by score, which should list
the best-matching sizes first.
This has two positive effects:
1. On mobile, selection of high-resolution bitmap doesn't choose
the absolute largest, but the best fit.
2. On desktop, candidates are also sorted, so exact matches are more
likely to be processed first and hence reducing the total number of
downloaded images.
BUG=704496
Review-Url: https://codereview.chromium.org/2739173002
Cr-Commit-Position: refs/heads/master@{#461096}
Committed: https://chromium.googlesource.com/chromium/src/+/89f78d0440e218d828e11e891ad76e5f98a1f41b
Patch Set 1 #Patch Set 2 : One more test. #Patch Set 3 : Polishing. #Patch Set 4 : Revisit TestFaviconWasScaledAfterDownload #
Total comments: 4
Patch Set 5 : Updated. #
Total comments: 4
Patch Set 6 : WIP. #
Total comments: 9
Patch Set 7 : Revert changes to not change behavior. #Patch Set 8 : Finalize implementation. #Patch Set 9 : Rebased. #
Total comments: 6
Patch Set 10 : Reverted FaviconSelector. #Patch Set 11 : Minor changes. #
Total comments: 11
Patch Set 12 : Rebased. #
Total comments: 20
Patch Set 13 : More comments. #
Total comments: 15
Patch Set 14 : Addressed final comments. #Patch Set 15 : Addressed final comments. #Patch Set 16 : Return default value after NOTREACHED. #
Messages
Total messages: 48 (20 generated)
Description was changed from ========== WIP: Make FaviconHandler to always look for an ideal desired size. The goal is to avoid logic divergence for the case where the largest icon is requested. In any case, FaviconHandler should be looking for a bitmap closest (as in matching best) to a target resolution. This improves the situation for sites which publish an icon for an exact match, but also other candidates of higher resolution. BUG=699980 ========== to ========== WIP: Make FaviconHandler to always look for an ideal desired size. This improves two analogous scenarios: 1. On mobile, once above the maximum pixel size (192 on Android), there is no point in preferring even larger images, which are going to be downscaled anyway. 2. On desktop, for sites which publish an icon for an exact match, say 16x16, but also other candidates of higher resolution: there's no point in downloading bigger candidates. From the perspective of implementation compexity, the goal is to avoid diverging logic for the case where the largest icon is requested. In any case, FaviconHandler should be looking for a bitmap closest (as in matching best) to a target resolution. BUG=699980 ==========
Description was changed from ========== WIP: Make FaviconHandler to always look for an ideal desired size. This improves two analogous scenarios: 1. On mobile, once above the maximum pixel size (192 on Android), there is no point in preferring even larger images, which are going to be downscaled anyway. 2. On desktop, for sites which publish an icon for an exact match, say 16x16, but also other candidates of higher resolution: there's no point in downloading bigger candidates. From the perspective of implementation compexity, the goal is to avoid diverging logic for the case where the largest icon is requested. In any case, FaviconHandler should be looking for a bitmap closest (as in matching best) to a target resolution. BUG=699980 ========== to ========== Make FaviconHandler to always look for an ideal desired size This improves two (analogous) scenarios: 1. On mobile, once above the maximum pixel size (192 on Android), there is no point in preferring even larger images, which are going to be downscaled anyway. 2. On desktop, for sites which publish an icon for an exact match, say 16x16, but also other candidates of higher resolution: there's no point in downloading bigger candidates. From the perspective of implementation complexity, the goal is to avoid diverging logic for the case where the largest icon is requested. In any case, FaviconHandler should be looking for a bitmap closest (as in matching best) to a target resolution. BUG=699980 ==========
Description was changed from ========== Make FaviconHandler to always look for an ideal desired size This improves two (analogous) scenarios: 1. On mobile, once above the maximum pixel size (192 on Android), there is no point in preferring even larger images, which are going to be downscaled anyway. 2. On desktop, for sites which publish an icon for an exact match, say 16x16, but also other candidates of higher resolution: there's no point in downloading bigger candidates. From the perspective of implementation complexity, the goal is to avoid diverging logic for the case where the largest icon is requested. In any case, FaviconHandler should be looking for a bitmap closest (as in matching best) to a target resolution. BUG=699980 ========== to ========== Make FaviconHandler to always look for an ideal desired size. This improves two analogous scenarios: 1. On mobile, once above the maximum pixel size (192 on Android), there is no point in preferring even larger images, which are going to be downscaled anyway. 2. On desktop, for sites which publish an icon for an exact match, say 16x16, but also other candidates of higher resolution: there's no point in downloading bigger candidates. A more controversial side effect is reflected in the removed test From the perspective of implementation complexity, the goal is to avoid diverging logic for the case where the largest icon is requested. In any case, FaviconHandler should be looking for a bitmap closest (as in matching best) to a target resolution. BUG=699980 ==========
Please take a high-level look! At this stage, it's most important for me to discuss on the question I raise below, about how we foresee the future with support for multiple favicons per type. https://codereview.chromium.org/2739173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:443: gfx::ImageSkia(gfx::ImageSkiaRep(bitmaps[index], 1)); This is the part that I'd expect most scrutiny: why does the old code try to expose a multi-bitmap image? More specifically: 1. Are there UIs (FaviconDriverObserver) that need this. 2. Is it just a mechanism for SetFavicon() to register all bitmaps? I believe this would be complex to implement in a multi-PNG world which brings us to a key question: would you anticipate a single call to SetFavicon() in a future where multiple favicons per type are supported? On a related note, a key design decision is whether we'll have multiple FaviconHandler instances, and whether each will publish more than one bitmap.
pkotwicz@chromium.org changed reviewers: + pkotwicz@chromium.org
I think that the bucket part of the CL is the most relevant towards supporting multiple favicon URLs per page I will investigate the supported scale factors on Android later this week https://codereview.chromium.org/2739173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:443: gfx::ImageSkia(gfx::ImageSkiaRep(bitmaps[index], 1)); Yes, there are UIs which need this. If you connect a Pixel Chromebook to an external monitor you can have one display which displays in 2x mode and another display which displays in 1x mode. Dragging a browser window from the 1x monitor to the 2x monitor switches which icons are shown in the bookmark bar (and other UIs). This is why gfx::ImageSkia is used a lot in the UI. FaviconService::SetFavicons() does not care about multi resolution icons ContentFaviconDriver::OnFaviconUpdated() does https://codereview.chromium.org/2739173002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:112: GetFaviconCandidateScore(size, target_size.GetMaxPixelSize())); You should use SelectFaviconFrameIndices() to compute the score. If you want, you can compute the score of a file by summing up the scores for each of the desired sizes SelectFaviconFrameIndices(image_url.icon_sizes, desired_size, nullptr, &score_out); A candidate with matches for multiple sizes should have a higher score than a candidate with a match for just one size One day, we will evaluate each frame in .ico file separately (so you might pick the 32x32 and 16x16 icons from different files) but till we support multiple icon URLs per page URL in the database we need to select a single file as the best candidate. https://codereview.chromium.org/2739173002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:375: favicon_base::IconType icon_type) { In order to future proof, we should make FaviconHandler::SetFavicon() and FaviconHandler::UpdateFaviconCandidate() take a std::vector<> of SkBitmaps. (I am suggesting moving the FaviconServiceImpl::SetFavicons() logic into FaviconHandler::SetFavicon()) Delegate::OnFaviconUpdated() should continue to be passed a multi resolution gfx::Image https://codereview.chromium.org/2739173002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:506: !UpdateFaviconCandidate(image_url, image, download_request.icon_type); UpdateFaviconCandidate() should be fed |bitmaps| and |original_bitmap_sizes| UpdateFaviconCandidate() should iterate through all of the buckets and find the best candidate for each bucket and compute the score summing the scores for each match up
Description was changed from ========== Make FaviconHandler to always look for an ideal desired size. This improves two analogous scenarios: 1. On mobile, once above the maximum pixel size (192 on Android), there is no point in preferring even larger images, which are going to be downscaled anyway. 2. On desktop, for sites which publish an icon for an exact match, say 16x16, but also other candidates of higher resolution: there's no point in downloading bigger candidates. A more controversial side effect is reflected in the removed test From the perspective of implementation complexity, the goal is to avoid diverging logic for the case where the largest icon is requested. In any case, FaviconHandler should be looking for a bitmap closest (as in matching best) to a target resolution. BUG=699980 ========== to ========== WIP: Extend FaviconHandler to support multiple favicons per type Some immediate improvements: 1. Support for sites that publish multiple PNG favicons at different sizes (main goal of the patch). 2. On mobile, once above the maximum pixel size (192 on Android), there is no point in preferring even larger images, which are going to be downscaled anyway. The new algorithm will try to prevent this. 3. On desktop, candidates are also sorted, so exact matches are more like to be processed first and hence reducing the total number of downloaded images. BUG=699980 ==========
This is WIP: I followed your advice and went ahead with the multi-bucket support. This makes it a large CL: perhaps we can find ways to split it and make the review process easier. https://codereview.chromium.org/2739173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:443: gfx::ImageSkia(gfx::ImageSkiaRep(bitmaps[index], 1)); On 2017/03/15 03:49:39, pkotwicz wrote: > Yes, there are UIs which need this. If you connect a Pixel Chromebook to an > external monitor you can have one display which displays in 2x mode and another > display which displays in 1x mode. Dragging a browser window from the 1x monitor > to the 2x monitor switches which icons are shown in the bookmark bar (and other > UIs). This is why gfx::ImageSkia is used a lot in the UI. > > FaviconService::SetFavicons() does not care about multi resolution icons > ContentFaviconDriver::OnFaviconUpdated() does > Are you sure SetFavicons() doesn't care about multiple resolutions? I would expect you could (currently) feed an .ico file with bitmaps that will be used for sync and for UI purposes. Another way of putting this: IIUC OnFaviconUpdated() should publish the same image with or without database hit. If SetFavicons() doesn't store all bitmaps, then it's not possible, or am I missing something? https://codereview.chromium.org/2739173002/diff/80001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/80001/components/favicon/core... components/favicon/core/favicon_handler.cc:112: GetFaviconCandidateScore(size, target_size.GetMaxPixelSize())); On 2017/03/15 03:49:39, pkotwicz wrote: > You should use SelectFaviconFrameIndices() to compute the score. > > If you want, you can compute the score of a file by summing up the scores for > each of the desired sizes > SelectFaviconFrameIndices(image_url.icon_sizes, desired_size, nullptr, > &score_out); > > A candidate with matches for multiple sizes should have a higher score than a > candidate with a match for just one size > > One day, we will evaluate each frame in .ico file separately (so you might pick > the 32x32 and 16x16 icons from different files) but till we support multiple > icon URLs per page URL in the database we need to select a single file as the > best candidate. I believe this is a moot point now, because of the proposed buckets. I however still see no use for SelectFaviconFrameIndices().
Your CL looks mostly good https://codereview.chromium.org/2739173002/diff/60001/components/favicon/core... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/60001/components/favicon/core... components/favicon/core/favicon_handler.cc:443: gfx::ImageSkia(gfx::ImageSkiaRep(bitmaps[index], 1)); FaviconService::SetFavicons() should take in a std::vector of SkBitmaps. It does not need to deal with a gfx::Image. One thing that I did not consider is that FaviconService::SetFavicons() should take in resized bitmaps. Currently CreateFaviconImageSkia() in select_favicon_frames.cc does the resizing https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:106: std::list<FaviconHandler::BitmapCandidate> SortAndPruneCandidates( I know that you use the BitmapCandidate::icon_size property in this method. I think that the BitmapCandidate return type should not have a BitmapCandidate::icon_size field. The field is not used anywhere else. This would make the candidate less confusing. https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:243: pending_candidates_(SortAndPruneCandidates(target_size_spec, candidates)), In order to guarantee no change in behavior for now, each queue must have the candidates in the exact same order. I think that each queue should take in its constructor an already sorted list of candidates https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:258: return best_candidate_ != nullptr; This CL removes the logic handling the case of the downloaded bitmap having a different size than the declared size. It would be nice if you could write a separate CL which removes that logic. That way things are super clear https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:307: target_size_spec_.PixelSize()); Here you need to call SelectFaviconFrameIndices() to compute the score here. Otherwise, you might end up using different icon URLs for different sizes. We are not ready for that yet. https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:551: candidate_queue.ProcessDownloadedImage(current_candidate_.value(), Umm, shouldn't |original_bitmap_sizes| be passed in to ProcessDownloadedImage() ? We care about |original_bitmap_sizes| in the case that the sizes in <link sizes=""> are empty. The desktop version of most sites does not specify sizes. https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:625: for (BitmapCandidateQueue& candidate_queue : candidate_queues_) { I think that the IsSatisfied() check should be done here instead of in DequeueCandidate() https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.cc:642: SetFavicon(candidate_queues_.front().BestBitmap()->first.icon_url, This is bad. If - page_urlA maps to icon_url1 and icon_url2 - page_urlB maps to just icon_url1 - the best bitmaps for page_urlA is to use both icon_url1 and icon_url2 The call to SetFavicon() will make page_urlB use icon_url1 and icon_url2. This is a bug You need to restrain the current logic so that it selects all of the bitmaps from the same icon URL https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.h:131: class BitmapCandidateQueue { It is weird for a queue to have so much logic in it. Rename this to FaviconSelector perhaps? https://codereview.chromium.org/2739173002/diff/100001/components/favicon/cor... components/favicon/core/favicon_handler.h:174: std::unique_ptr<std::pair<BitmapCandidate, SkBitmap>> best_candidate_; I think that it would be easier to have: best_candidate_ best_score_ best_bitmap_ as separate variables This would allow you to remove BitmapCandidate::score
PTAL if you find time to see if this is going in the right direction, this is still WIP. I did not address many of your comments so far (will do), but I did follow what we agreed on our offline discussion so split the code to a separate file and avoid behavioral changes for this first CL.
Patch is ready for review overall although I'm planning to address some comments that are still applicable. Tests are passing with two minor behavioral changes: one obviously good (selection of largest bitmap) and one potential minor regression, since the preference towards exact matches for non-largest-scale-factor is removed. I think the second is acceptable but I'm interested in hearing from the requirements that motivated this originally, and whether this would be OK to move forward (since I don't quite see how that would be relevant in the multi-file scenario, i.e. after the next patch).
https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... components/favicon/core/favicon_handler.cc:154: target_size_spec_(GetTargetSizeSpecs(handler_type).front()), Won't this regress the current behavior? https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... components/favicon/core/favicon_handler.cc:392: DoUrlsAndIconsMatch(*current_candidate_, favicon_bitmap_results))) { This if() statement will now always return true because |current_candidate_| will always be null when the statement is hit? Can we use selector_->CurrentCandidate() |FaviconSelector::CurrentCandidate()| and |FaviconHandler::current_candidate_| is confusing https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... File components/favicon/core/favicon_selector.cc (right): https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... components/favicon/core/favicon_selector.cc:130: for (const gfx::Size& favicon_size : favicon_url.icon_sizes) { If you passed in a vector of target sizes (std::vector<int> target_pixel_sizes) you could compute the score by summing together the score computed by GetFaviconCandidateScore() (Or just use the score computed by SelectFaviconFrameIndices() which does the summation for you)
Thanks! Will update CL description to reflect current version. https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... components/favicon/core/favicon_handler.cc:154: target_size_spec_(GetTargetSizeSpecs(handler_type).front()), On 2017/03/20 03:25:16, pkotwicz wrote: > Won't this regress the current behavior? As I mentioned in my earlier message, there's the behavioral difference that one could consider a regression, which is reflected by the test chage in ChooseExactMatchDespiteUpsampling. More on this below, let me know if you had some other regression in mind. https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... components/favicon/core/favicon_handler.cc:392: DoUrlsAndIconsMatch(*current_candidate_, favicon_bitmap_results))) { On 2017/03/20 03:25:16, pkotwicz wrote: > This if() statement will now always return true because |current_candidate_| > will always be null when the statement is hit? > > Can we use selector_->CurrentCandidate() > > |FaviconSelector::CurrentCandidate()| and |FaviconHandler::current_candidate_| > is confusing You're right, thanks. I couldn't decide whether it's best to dequeue the element before this point (then followed by DownloadCurrentCandidateOrAskFaviconService()), or do it as proposed in this version (i.e. all dequeuing in DownloadNextCandidateOrAskFaviconService(). I'm leaning towards the second. Wrt naming confusion, I renamed CurrentCandidate()->PeekCandidate(). Do you like this better? https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... File components/favicon/core/favicon_selector.cc (right): https://codereview.chromium.org/2739173002/diff/160001/components/favicon/cor... components/favicon/core/favicon_selector.cc:130: for (const gfx::Size& favicon_size : favicon_url.icon_sizes) { On 2017/03/20 03:25:16, pkotwicz wrote: > If you passed in a vector of target sizes (std::vector<int> target_pixel_sizes) > you could compute the score by summing together the score computed by > GetFaviconCandidateScore() > > (Or just use the score computed by SelectFaviconFrameIndices() which does the > summation for you) This deserves some discussion, because my proposal for FaviconSelector is that it targets one single pixel size (and doesn't even know about the rest). Hence, the summation you propose would arguably belong outside, FaviconHandler. My high level question is whether this summation is actually needed. What's the actual requirement behind ChooseExactMatchDespiteUpsampling? Intuitively, assuming most platforms care mostly about the largest supported scale factor, it looks like undesirable.
Description was changed from ========== WIP: Extend FaviconHandler to support multiple favicons per type Some immediate improvements: 1. Support for sites that publish multiple PNG favicons at different sizes (main goal of the patch). 2. On mobile, once above the maximum pixel size (192 on Android), there is no point in preferring even larger images, which are going to be downscaled anyway. The new algorithm will try to prevent this. 3. On desktop, candidates are also sorted, so exact matches are more like to be processed first and hence reducing the total number of downloaded images. BUG=699980 ========== to ========== Always select best favicon bitmap We do this by introducing a new class, FaviconSelector, which is responsible for finding the most appropriate bitmap for a given pixel size. As part of this change, three side effects exist, two clearly good and one more questionable: 1. On mobile, selection of high-resolution bitmap doesn't choose the absolute largest, but the best fit. 2. On desktop, candidates are also sorted, so exact matches are more like to be processed first and hence reducing the total number of downloaded images. 3. (Questionable) For desktop platforms supporting multiple scale factors, the best fit for the largest scale factor is chosen, independently from exact matches in smaller scale factors (reflected in tests, see ChooseBestDespiteSmallerExactMatch). BUG=699980 ==========
Description was changed from ========== Always select best favicon bitmap We do this by introducing a new class, FaviconSelector, which is responsible for finding the most appropriate bitmap for a given pixel size. As part of this change, three side effects exist, two clearly good and one more questionable: 1. On mobile, selection of high-resolution bitmap doesn't choose the absolute largest, but the best fit. 2. On desktop, candidates are also sorted, so exact matches are more like to be processed first and hence reducing the total number of downloaded images. 3. (Questionable) For desktop platforms supporting multiple scale factors, the best fit for the largest scale factor is chosen, independently from exact matches in smaller scale factors (reflected in tests, see ChooseBestDespiteSmallerExactMatch). BUG=699980 ========== to ========== Always select best favicon bitmap We do this by always sorting candidates by score, which should list the best-matching sizes first. This has two positive effects: 1. On mobile, selection of high-resolution bitmap doesn't choose the absolute largest, but the best fit. 2. On desktop, candidates are also sorted, so exact matches are more likely to be processed first and hence reducing the total number of downloaded images. BUG=704496 ==========
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL after I reverted FaviconSelector code, in line with our latest discussions to perhaps never add support for multiple favicons per page.
I think that storing the score in the candidates makes the code easier to read https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (left): https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.cc:294: exact_match = score == 1 || preferred_icon_size() == 0; Good eye in catching that we can remove the preferred_icon_size() == 0 check https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.cc:146: } This score is suboptimal. If FaviconHandler wants favicons of size 16x16 and 32x32 the following two candidates will have the same score: A) single resolution 32x32 PNG image B) multi resolution ICO file with 16x16 and 32x32 However, (A) should have a higher score than (B) https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.cc:374: 1)); - You could use SelectFaviconFrameIndices() to compute the score. That way you don't need to introduce a new function in select_favicon_frames.cc - You can use gfx::ImageSkia::CreateFrom1xBitmap() to save some typing https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.h:157: // Compare function used for std::stable_sort to sort as descend. Nit: "as descend" -> "in descending order" https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.h:300: // the image is for a favicon). Can you please update this comment?
PTAL. https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.cc:146: } On 2017/03/23 20:43:25, pkotwicz wrote: > This score is suboptimal. > If FaviconHandler wants favicons of size 16x16 and 32x32 > > the following two candidates will have the same score: > A) single resolution 32x32 PNG image > B) multi resolution ICO file with 16x16 and 32x32 > > However, (A) should have a higher score than (B) Done. https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.cc:374: 1)); On 2017/03/23 20:43:25, pkotwicz wrote: > - You could use SelectFaviconFrameIndices() to compute the score. That way you > don't need to introduce a new function in select_favicon_frames.cc > > - You can use gfx::ImageSkia::CreateFrom1xBitmap() to save some typing Done. https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.h:157: // Compare function used for std::stable_sort to sort as descend. On 2017/03/23 20:43:26, pkotwicz wrote: > Nit: "as descend" -> "in descending order" Done. https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.h:300: // the image is for a favicon). On 2017/03/23 20:43:25, pkotwicz wrote: > Can you please update this comment? Done.
https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.cc:146: } I don't see any changes?
Seems like I forgot to export the last version, PTAL. https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/200001/components/favicon/cor... components/favicon/core/favicon_handler.cc:146: } On 2017/03/26 01:39:53, pkotwicz wrote: > I don't see any changes? Now?
A few last comments https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:116: pixel_sizes.push_back(scale_factor * gfx::kFaviconSize); Nit: Round up the way that GetPixelSizesForFaviconScales() in favicon_service_impl.cc does https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:140: /*best_indices=*/nullptr, &candidate.score); https://www.reddit.com/ specifies two icons: <link rel="icon" href="//www.redditstatic.com/icon.png" sizes="256x256" type="image/png"> <link rel="shortcut icon" href="//www.redditstatic.com/favicon.ico" type="image/x-icon"> Notably, there is no sizes information specified for the .ico file The youtube mobile site also specifies "sizes" information for all of the favicons except the .ico file Perhaps if no sizes information was provided and the file is a .ico file, we can guess that the .ico file has a 16x16 icon and compute the score based on that guess. I think that making this guess will result in downloading the .ico file first on desktop on both reddit and youtube. https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:222: // - current candidate is maximal one we want. Nit: Can you please update the comment? I think this part - "all favicon without sizes attribute are downloaded" is confusing. (even with the old code) https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.h:146: // Builds a scored candidate by selecting the best bitmap size. Nit: size -> sizes https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:914: GURL("http://www.google.com/d"), GURL("http://www.google.com/e"))); Can't you just swap FaviconHandler::image_urls() for FaviconHandler::GetIconURLs(). If you want put the the entire test into one EXPECT_THAT() call, can you name the FaviconURLs for the sizes that they specify the way that TestDownloadLargestFavicon does?
Thanks, PTAL! https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:116: pixel_sizes.push_back(scale_factor * gfx::kFaviconSize); On 2017/03/27 00:49:06, pkotwicz wrote: > Nit: Round up the way that GetPixelSizesForFaviconScales() in > favicon_service_impl.cc does Done. https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:140: /*best_indices=*/nullptr, &candidate.score); On 2017/03/27 00:49:06, pkotwicz wrote: > https://www.reddit.com/ specifies two icons: > > <link rel="icon" href="//www.redditstatic.com/icon.png" sizes="256x256" > type="image/png"> > <link rel="shortcut icon" href="//www.redditstatic.com/favicon.ico" > type="image/x-icon"> > > Notably, there is no sizes information specified for the .ico file > > The youtube mobile site also specifies "sizes" information for all of the > favicons except the .ico file > > Perhaps if no sizes information was provided and the file is a .ico file, we can > guess that the .ico file has a 16x16 icon and compute the score based on that > guess. > I think that making this guess will result in downloading the .ico file first on > desktop on both reddit and youtube. I prefer implementing such logic in a dedicated patch, since it requires careful thinking. For example, on mobile, it could cause .ico files to be chosen more often than before, which is likely a bad thing. https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:222: // - current candidate is maximal one we want. On 2017/03/27 00:49:06, pkotwicz wrote: > Nit: Can you please update the comment? > > I think this part - "all favicon without sizes attribute are downloaded" is > confusing. (even with the old code) Done. https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler.h (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.h:146: // Builds a scored candidate by selecting the best bitmap size. On 2017/03/27 00:49:06, pkotwicz wrote: > Nit: size -> sizes Done. https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:914: GURL("http://www.google.com/d"), GURL("http://www.google.com/e"))); On 2017/03/27 00:49:06, pkotwicz wrote: > Can't you just swap FaviconHandler::image_urls() for > FaviconHandler::GetIconURLs(). No, because we don't have size information. And I wouldn't like to add size information to FaviconCandidate for the sake of this test. > > > If you want put the the entire test into one EXPECT_THAT() call, can you name > the FaviconURLs for the sizes that they specify the way that > TestDownloadLargestFavicon does? Done.
https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:140: /*best_indices=*/nullptr, &candidate.score); If you "assign 16x16 to sizes for .ico files if sizes is unspecified in the <link> tag", it will not cause .ico files to be chosen more often on mobile. SelectFaviconFrameIndices() with desired_sizes = {192x192} will return a bad score for frame_pixel_sizes = {16x16} The only case that the .ico file will be chosen more on mobile is if none of the icons specify sizes information. In this case, the .ico file with its auto assigned size will be deemed a better candidate than the icons with no sizes information I am ok if you do this in a separate CL. Please file a bug in order to fix this https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:222: // - current candidate is maximal one we want. I don't see any changes to the comment https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:285: if (!candidate.icon_url.is_empty() && (candidate.icon_type & icon_types_)) Nit: Can you please add braces since the inner code (286 - 287) now takes up two lines? https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:295: // history database. This comment is no longer valid since FaviconCandidate::Equals() checks that the scores are equal. The score and the order of |sorted_candidates| is based on the size information. Delete the comment? https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:914: GURL("http://www.google.com/d"), GURL("http://www.google.com/e"))); Ok I see now. Can you name each of the FaviconURLs in kSourceIconURLs based on the sizes data in each FaviconURL like in TestDownloadLargestFavicon?
PTAL, thanks! https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:140: /*best_indices=*/nullptr, &candidate.score); On 2017/03/27 19:12:44, pkotwicz wrote: > If you "assign 16x16 to sizes for .ico files if sizes is unspecified in the > <link> tag", it will not cause .ico files to be chosen more often on mobile. > > SelectFaviconFrameIndices() with desired_sizes = {192x192} will return a bad > score for frame_pixel_sizes = {16x16} > > The only case that the .ico file will be chosen more on mobile is if none of the > icons specify sizes information. In this case, the .ico file with its auto > assigned size will be deemed a better candidate than the icons with no sizes > information > > I am ok if you do this in a separate CL. Please file a bug in order to fix this Done. Filed a bug against you since our team is unlikely to work on improvements for desktop. https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:222: // - current candidate is maximal one we want. On 2017/03/27 19:12:44, pkotwicz wrote: > I don't see any changes to the comment Sorry, this is the second time I fail to export an updated patchset. Not sure what's going on... https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:285: if (!candidate.icon_url.is_empty() && (candidate.icon_type & icon_types_)) On 2017/03/27 19:12:43, pkotwicz wrote: > Nit: Can you please add braces since the inner code (286 - 287) now takes up two > lines? Done. https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler.cc:295: // history database. On 2017/03/27 19:12:43, pkotwicz wrote: > This comment is no longer valid since FaviconCandidate::Equals() checks that the > scores are equal. > The score and the order of |sorted_candidates| is based on the size information. > Delete the comment? Done, removed. https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2739173002/diff/220001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:914: GURL("http://www.google.com/d"), GURL("http://www.google.com/e"))); On 2017/03/27 19:12:44, pkotwicz wrote: > Ok I see now. > > Can you name each of the FaviconURLs in kSourceIconURLs based on the sizes data > in each FaviconURL like in TestDownloadLargestFavicon? This was done, but I had failed to export the changes, PTAL.
LGTM https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler.cc:27: const int kNonTouchLargestIconSize = 192; Nit: New line https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler.cc:116: for (float scale_factor : favicon_base::GetFaviconScales()) Can you add braces to the for() ? https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler.cc:305: candidates_.begin(), &FaviconCandidate::Equals)) { Can you use the default struct comparison operator instead of FaviconCandidate::Equals() ? https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:914: // First is icon2, though its size larger than maximal. Can you please update the comments?
One clarification needed below, thanks! https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... File components/favicon/core/favicon_handler.cc (right): https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler.cc:27: const int kNonTouchLargestIconSize = 192; On 2017/03/28 20:45:39, pkotwicz wrote: > Nit: New line Done. https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler.cc:116: for (float scale_factor : favicon_base::GetFaviconScales()) On 2017/03/28 20:45:39, pkotwicz wrote: > Can you add braces to the for() ? Done. https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler.cc:305: candidates_.begin(), &FaviconCandidate::Equals)) { On 2017/03/28 20:45:39, pkotwicz wrote: > Can you use the default struct comparison operator instead of > FaviconCandidate::Equals() ? Done, assuming you meant a custom operator==, since a default one doesn't exist for structs. https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:914: // First is icon2, though its size larger than maximal. On 2017/03/28 20:45:39, pkotwicz wrote: > Can you please update the comments? Sorry, I don't really know what you want to write here. The statement is holds true. Can you clarify?
https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:914: // First is icon2, though its size larger than maximal. On 2017/03/29 08:53:01, mastiz wrote: > On 2017/03/28 20:45:39, pkotwicz wrote: > > Can you please update the comments? > > Sorry, I don't really know what you want to write here. The statement is holds > true. Can you clarify? Friendly ping, thx!
https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:914: // First is icon2, though its size larger than maximal. Perhaps the comment should be: The 512x512 bitmap is the best match for the desired size of 192x192 https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:916: // Second is icon1. Nit: Remove this comment https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:918: // Third is icon3. Nit: Remove this comment
Thanks! Landing... https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... File components/favicon/core/favicon_handler_unittest.cc (right): https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:914: // First is icon2, though its size larger than maximal. On 2017/03/30 14:53:43, pkotwicz wrote: > Perhaps the comment should be: > > The 512x512 bitmap is the best match for the desired size of 192x192 > Done, although I removed 192x192 since it's currently platform-dependent. https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:916: // Second is icon1. On 2017/03/30 14:53:43, pkotwicz wrote: > Nit: Remove this comment Done. https://codereview.chromium.org/2739173002/diff/240001/components/favicon/cor... components/favicon/core/favicon_handler_unittest.cc:918: // Third is icon3. On 2017/03/30 14:53:43, pkotwicz wrote: > Nit: Remove this comment Done.
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2739173002/#ps280001 (title: "Addressed final comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by mastiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkotwicz@chromium.org Link to the patchset: https://codereview.chromium.org/2739173002/#ps300001 (title: "Return default value after NOTREACHED.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by mastiz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 300001, "attempt_start_ts": 1490959360518370,
"parent_rev": "17498ec3190acff0ae88d3083543a50da9d0af83", "commit_rev":
"89f78d0440e218d828e11e891ad76e5f98a1f41b"}
Message was sent while issue was closed.
Description was changed from ========== Always select best favicon bitmap We do this by always sorting candidates by score, which should list the best-matching sizes first. This has two positive effects: 1. On mobile, selection of high-resolution bitmap doesn't choose the absolute largest, but the best fit. 2. On desktop, candidates are also sorted, so exact matches are more likely to be processed first and hence reducing the total number of downloaded images. BUG=704496 ========== to ========== Always select best favicon bitmap We do this by always sorting candidates by score, which should list the best-matching sizes first. This has two positive effects: 1. On mobile, selection of high-resolution bitmap doesn't choose the absolute largest, but the best fit. 2. On desktop, candidates are also sorted, so exact matches are more likely to be processed first and hence reducing the total number of downloaded images. BUG=704496 Review-Url: https://codereview.chromium.org/2739173002 Cr-Commit-Position: refs/heads/master@{#461096} Committed: https://chromium.googlesource.com/chromium/src/+/89f78d0440e218d828e11e891ad7... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/89f78d0440e218d828e11e891ad7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
