history.pushState triggers icon downloading |
|||||
Issue descriptionReported by user on Twitter (https://twitter.com/nekrtemplar/status/1051618550052507650). Unknown Chrome version, platform, etc. (@nekrtemplar, can you provide this info?) I couldn't reproduce this when quickly trying it from the console on Linux Chrome 71. Apparently, using history.pushState() redownloads the page favicon unnecessarily. This compounds with Issue 324820 where all icons are downloaded, not just the appropriately sized one. So every pushState causes like 8 icons to be downloaded.
,
Oct 15
I'll try to make a repro. It's related to PWA Conpat library (Chrome tem makes it) which generates link tags on the fly in JS. Maybe this causes they to be redowbloaded on each pushState, not sure. But it definitely happens.
,
Oct 15
I couldn't reproduce this with a simple test page. Reporter, would you provide a complete HTML please?
,
Oct 15
This does it for me. A request is fired per icon, per pushState.
,
Oct 29
The NextAction date has arrived: 2018-10-29
,
Nov 5
Confirmed. Add //components/favicon/ owners.
,
Nov 5
Thanks all for reporting. I suspect this issue is unrelated to history.pushState() and repros on desktop only. Short answer: the issue should go away if the site provided 16x16 and 32x32 icons (dependent on screen density). Wrt comment 4: the provided example differs from the original report because the favicons don't provide a 'sizes' hint. It's still a legit feature request, but a different one, and better tracked in https://bugs.chromium.org/p/chromium/issues/detail?id=672076. If I read the twitter thread correctly, all icon paths are absolute and they don't change from one page load to another (well, history.pushState()). I'll assume that the page's URL itself is dynamically changing. In such circumstances, Chrome will sort the listed favicons according to their |sizes| (most desirable first) and process them one by one. At this point, the reported scenario can be reproduced on desktop, *if* no ideal favicon exists (based on [1]), which probably means 16x16 or 32x32. On desktop, we don't optimize these loads. Fixing this should be relatively easy (essentially do something similar to what we do for mobile), but we should be careful with candidates that do *not* provide a 'sizes' hint (perhaps they should score second-highest on desktop). I'd be happy to review code contributions but cannot myself commit to any improvement. [1] https://cs.chromium.org/chromium/src/components/favicon/core/favicon_handler.cc?l=241&rcl=02b4622b5182f80acd62e85aa17a0d86ad77ab8b
,
Nov 5
In 72.0.3595.2 I no longer see excess icon requests, sizes hint or not.
,
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
Should be fixed now.
,
Nov 26
,
Nov 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4e76a579eae62db2bbb6ed808b1665076aa25815 commit 4e76a579eae62db2bbb6ed808b1665076aa25815 Author: Mikel Astiz <mastiz@chromium.org> Date: Fri Nov 30 13:06:58 2018 Stop downloading favicons regardless of last outcome The decision of whether the next candidate needs to be downloaded or not should be independent of possible errors in the last download. Let's move that logic to a dedicated predicate (ShouldDownloadNextCandidate()) and make sure it's honored in all codepaths (e.g. 404s, or if image_skia.isNull()). Bug: 895175 Change-Id: Id4d41ea873b62559c3d191952045f9c714fe729f Reviewed-on: https://chromium-review.googlesource.com/c/1350631 Commit-Queue: Mikel Astiz <mastiz@chromium.org> Reviewed-by: Peter Kotwicz <pkotwicz@chromium.org> Cr-Commit-Position: refs/heads/master@{#612625} [modify] https://crrev.com/4e76a579eae62db2bbb6ed808b1665076aa25815/components/favicon/core/favicon_handler.cc [modify] https://crrev.com/4e76a579eae62db2bbb6ed808b1665076aa25815/components/favicon/core/favicon_handler.h [modify] https://crrev.com/4e76a579eae62db2bbb6ed808b1665076aa25815/components/favicon/core/favicon_handler_unittest.cc |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by mgiuca@chromium.org
, Oct 14