Load /favicon.ico as early as possible
Categories
(Firefox :: Tabbed Browser, enhancement, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: mossop, Assigned: mossop)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(2 files, 2 obsolete files)
The favicon selection code has a problem in that it only considers new or changed favicons in the set to load since it was last run and doesn't necessarily consider them in document order. The patch here changes it to always consider every icon link in the document in document order. Another issue is that we wait until the page finishes loading before even starting to load /favicon.ico if no other icon is found. This patch starts loading /favicon.ico as early as possible and cancels it if another icon is found.
Assignee | ||
Comment 1•6 years ago
|
||
The favicon selection code has a problem in that it only considers new or changed favicons in the set to load since it was last run and doesn't necessarily consider them in document order. This changeset changes it to always consider every icon link in the document in document order. It also starts loading /favicon.ico as early as possible and cancels it if another icon is found.
Comment 2•6 years ago
|
||
Could you please elaborate on how the new version addresses this problem I commented about on the first version: Off-hand this risks to break pages putting badges on favicons. Those pages tend to add new link rel="icon" at a later time after load to notify that some entries are new/unread. We should always prefer these later additions as best icon. The current code only considers the newly added icons in subsequent runs to better support that case, while it looks like the patch, for each link additon, considers again all of the existing icons. So, it sounds like it could pick a non-up-to-date icon, just because it has a better format/resolution.
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #2) > Could you please elaborate on how the new version addresses this problem I > commented about on the first version: > Off-hand this risks to break pages putting badges on favicons. Those pages > tend to add new link rel="icon" at a later time after load to notify that > some entries are new/unread. We should always prefer these later additions > as best icon. > > The current code only considers the newly added icons in subsequent runs to > better support that case, while it looks like the patch, for each link > additon, considers again all of the existing icons. > So, it sounds like it could pick a non-up-to-date icon, just because it has > a better format/resolution. Sorry I missed that comment. (In reply to Marco Bonardo [::mak] from comment #2) > Could you please elaborate on how the new version addresses this problem I > commented about on the first version: > Off-hand this risks to break pages putting badges on favicons. Those pages > tend to add new link rel="icon" at a later time after load to notify that > some entries are new/unread. We should always prefer these later additions > as best icon. > > The current code only considers the newly added icons in subsequent runs to > better support that case, while it looks like the patch, for each link > additon, considers again all of the existing icons. > So, it sounds like it could pick a non-up-to-date icon, just because it has > a better format/resolution. Sorry, I missed your question before. So here's the problem. The current implementation will do different things based on the timing of events. So if a webapp sets a badged icon within 500ms of us parsing the rest of the icons we'll do something different to if it sets it 500ms after. That doesn't seem right to me but maybe that is the way it has to be to maintain compatibility with the web? We should see this be a problem currently. If a page loads then later sets a badged icon we use the badged icon. If you then navigate to a new URL and then go back we restore the page from the bfcache which causes us to send out DOMLinkAdded events for every link in the document which means we might not use the badged icon this time around. I guess the question is do we want the icon we display to be entirely based on the DOM content or do we need to use timing to make badged icons work?
Comment 4•6 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #3) > So here's the problem. The current implementation will do different things > based on the timing of events. So if a webapp sets a badged icon within > 500ms of us parsing the rest of the icons we'll do something different to if > it sets it 500ms after. Right, though it sounds like a real edge case, it's likely being the js code setting the badged icon, if we didn't even finish parsing it's less likely to do it (not impossible, just improbable). > We should see this be a problem currently. If a page loads then later sets a > badged icon we use the badged icon. If you then navigate to a new URL and > then go back we restore the page from the bfcache which causes us to send > out DOMLinkAdded events for every link in the document which means we might > not use the badged icon this time around. I think that's ok, provided the page once restored will set the icon again. > I guess the question is do we want the icon we display to be entirely based > on the DOM content or do we need to use timing to make badged icons work? I'm not sure how we could base on dom content unless we completely change (again) the way we pick icons. Again, this problem imo should be split into showing and storing. We want to always show a badged icon, but ideally we never want to store it. Currently that's not the case. Though, it may also be non-trivial to recognize such icons, since there's no recognized standard. We could at maximum say "it's an icon that is not in the initial set of <link>s provided by the page".
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Ok based on the discussion I've pared this down to just loading /favicon.ico earlier to help alleviate bug 1473514
Comment 8•6 years ago
|
||
Comment on attachment 8996783 [details] Bug 1478823: Start loading the default favicon as soon as the head element is complete. Marco Bonardo [::mak] (Away 9-26 Aug) has approved the revision. https://phabricator.services.mozilla.com/D2613
Updated•6 years ago
|
Comment 9•6 years ago
|
||
Comment on attachment 8996782 [details] Bug 1478823: Add a chrome DOM event signalling that the head element's children have been parsed. Boris Zbarsky [:bz] (no decent commit message means r-) has approved the revision. https://phabricator.services.mozilla.com/D2612
Comment 10•6 years ago
|
||
Pushed by dtownsend@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c5386a64522c Add a chrome DOM event signalling that the head element's children have been parsed. r=bzbarsky
Updated•6 years ago
|
Comment 11•6 years ago
|
||
Pushed by dtownsend@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/741a654c2f4c Start loading the default favicon as soon as the head element is complete. r=mak
Comment 12•6 years ago
|
||
Backed out changeset 741a654c2f4c (bug 1478823) for mochithest failures at toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html Backout: https://hg.mozilla.org/integration/autoland/rev/25c8ddd2fe538d8765e260af4eb8a9494e22c809 Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=741a654c2f4c183a14c717f3367828f5003078e5 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191963871&repo=autoland&lineNumber=4565 15:15:11 INFO - TEST-PASS | toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html | correct ip for http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_image_bad.png - Expected: 127.0.0.1, Actual: 127.0.0.1 15:15:11 INFO - Buffered messages finished 15:15:11 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html | Test timed out. 15:15:11 INFO - reportError@SimpleTest/TestRunner.js:121:7 15:15:11 INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:163:5 15:15:11 INFO - TestRunner.runTests/<@SimpleTest/TestRunner.js:381:9 15:15:11 INFO - promise callback*TestRunner.runTests@SimpleTest/TestRunner.js:368:5 15:15:11 INFO - RunSet.runtests@SimpleTest/setup.js:194:3 15:15:11 INFO - RunSet.runall@SimpleTest/setup.js:173:5 15:15:11 INFO - hookupTests@SimpleTest/setup.js:266:5 15:15:11 INFO - parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5 15:15:11 INFO - getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11 15:15:11 INFO - EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3 15:15:11 INFO - hookup@SimpleTest/setup.js:246:5 15:15:11 INFO - EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=%2Fvar%2Ffolders%2F_t%2F053z6dhx5rg5_6bvcdf_slt400000w%2FT&cleanupCrashes=true:11:1 15:15:12 INFO - Not taking screenshot here: see the one that was previously logged 15:15:12 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest_filter.html | Extension left running at test shutdown 15:15:12 INFO - ExtensionTestUtils.loadExtension/<@SimpleTest/ExtensionTestUtils.js:109:7 15:15:12 INFO - executeCleanupFunction@SimpleTest/SimpleTest.js:1227:19 15:15:12 INFO - SimpleTest.finish@SimpleTest/SimpleTest.js:1240:5 15:15:12 INFO - killTest@SimpleTest/TestRunner.js:130:7 15:15:12 INFO - delayedKillTest@SimpleTest/TestRunner.js:157:47 15:15:12 INFO - setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:157:7
Comment 13•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c5386a64522c
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 14•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Comment 15•6 years ago
|
||
Comment on attachment 8998034 [details] Bug 1478823: Start loading the default favicon as soon as the head element is complete. Marco Bonardo [::mak] (Away 9-26 Aug) has approved the revision. https://phabricator.services.mozilla.com/D2803
Comment 16•6 years ago
|
||
Pushed by dtownsend@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4073ae1a277f Start loading the default favicon as soon as the head element is complete. r=mak
Comment 17•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4073ae1a277f
Comment 18•6 years ago
|
||
Shouldn't you check that the head element actually is in document? By creating dummy html element and then using innerHTML to add head there, as an example, one gets HTMLSharedElement::DoneAddingChildren called without the element being in document.
Updated•6 years ago
|
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #18) > Shouldn't you check that the head element actually is in document? By > creating dummy html element and then using innerHTML to add head there, as > an example, one gets HTMLSharedElement::DoneAddingChildren called without > the element being in document. Perhaps for correctness. I'm struggling to think of what problem not doing this would cause. I think the worst case is that it would cause us to start loading the root favicon before we encounter the real icons in the document which would then override it and be loaded normally. This is what we used to do before we moved loading favicons to the content process. Anyway I've filed bug 1483222 to clean it up.
Comment 20•5 years ago
|
||
Is there developer documentation for this change? Is this undoing the page load performance optimization enabled at https://bugzilla.mozilla.org/show_bug.cgi?id=1247843, which prioritized actually loading the page over loading the (mostly useless favicon)? If I want the page to be loaded first, and the favicon "whenever", how do I tell Firefox to do this?
Comment 21•5 years ago
|
||
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #20)
Is there developer documentation for this change? Is this undoing the page load performance optimization enabled at https://bugzilla.mozilla.org/show_bug.cgi?id=1247843, which prioritized actually loading the page over loading the (mostly useless favicon)? If I want the page to be loaded first, and the favicon "whenever", how do I tell Firefox to do this?
I don't think this really undoes that, Bug 1247843 acted at the network fetch level (using tailing to queue favicon loads), while this bug is about the order in which we look for icon definitions in the head of the document. The load still happens later, afaict.
Description
•