Favicon size should be inferred before downloading for .ico files |
||||
Issue descriptionThe current favicon-handling code on desktop, in a nutshell, downloads all favicons until one of an ideal size is found. We recently improved (WIP as of today) the logic to process candidates more smartly, to increase the likelihood of finding such an ideal candidate earlier. We do this by relying on size information declared in the HTML head. It is however very common that no size hint is provided in the HTML. Hence, the actual size of a favicon is not known until it's downloaded. It's also very common that .ico files have size 16x16, and often even the 32x32 bitmap. Such an assumption would allow to improve the handling of pages with .ico files that don't advertise a size.
,
Mar 28 2017
,
Apr 4 2018
If no size is provided, assume the size is infinite if the type indicates a vector type, otherwise assume 16x16. Would that work?
,
Apr 4 2018
,
Nov 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e9abde5a831c773c8e0ab85605254a591246925f commit e9abde5a831c773c8e0ab85605254a591246925f Author: Mikel Astiz <mastiz@chromium.org> Date: Mon Nov 26 10:14:50 2018 Avoid unnecessary favicon downloads on desktop Prior to this patch, desktop favicon downloading would only stop when a truly ideal icon is found, which means a favicon with perfect bitmaps sizes for all supported screen densities (often a .ico file with a 16x16 AND a 32x32 bitmap). This often leaded to all icons being downloaded, even for cases where it's obviously unnecessary. The patch adopts the logic we currently use on mobile within FaviconHandler::UpdateFaviconCandidate() in order to stop processing favicon candidates if there is little hope that the next ones will be any better. This requires some smartness for favicon candidates without an explicit 'sizes' attribute, which would score very low prior to this patch and hence would rarely be processed (if other candidates exist). Hence, we special-case them for desktop and score them highest during initial sorting, which mimics the behavior prior to this patch and guarantees they will be processed unless an ideal favicon is found before. It's rather trivial to rule out regressions for the following common scenarios: A. Mobile platforms: no behavioral changes as score stays zero for candidates without explicit 'sizes' attribute. B. A single favicon is listed by the page. C. Multiple favicons are listed and none have a 'sizes' attribute (sorting does nothing and all are processed unless an ideal one is found). D. Multiple favicons are listed and they all have an accurate 'sizes' attribute (scenario improved in this patch). The more complex scenario is when a web page lists multiple favicons and only some have a 'sizes' attribute. In this case, there is some minor risk for regressions given that the patch scores highest the favicons without a 'sizes' attribute, so they will be downloaded and processed earlier. Bug: 895175 , 705900 , 672076 Change-Id: I8216652ede25f55332cacc54d3e0a806fd8b7bc3 Reviewed-on: https://chromium-review.googlesource.com/c/1323551 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org> Cr-Commit-Position: refs/heads/master@{#610800} [modify] https://crrev.com/e9abde5a831c773c8e0ab85605254a591246925f/components/favicon/core/favicon_handler.cc [modify] https://crrev.com/e9abde5a831c773c8e0ab85605254a591246925f/components/favicon/core/favicon_handler.h [modify] https://crrev.com/e9abde5a831c773c8e0ab85605254a591246925f/components/favicon/core/favicon_handler_unittest.cc
,
Nov 26
As per our recent discussions with pkotwicz@, and the somewhat competing improvement above, it seems very unlikely that we address the original scope of this bug. If you think otherwise, please reopen. |
||||
►
Sign in to add a comment |
||||
Comment 1 by pkotw...@chromium.org
, Mar 28 2017