Closed Bug 1478823 Opened 6 years ago Closed 6 years ago

Load /favicon.ico as early as possible

Categories

(Firefox :: Tabbed Browser, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
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.
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.
Blocks: 1473514
Priority: -- → P1
Blocks: 1478424
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.
Flags: needinfo?(dtownsend)
(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?
Flags: needinfo?(dtownsend)
(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".
Attachment #8995339 - Attachment is obsolete: true
Ok based on the discussion I've pared this down to just loading /favicon.ico earlier to help alleviate bug 1473514
Summary: Load /favicon.ico as early as possible and always consider all icons when choosing which icon to load → Load /favicon.ico as early as possible
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
Attachment #8996783 - Flags: review+
Attachment #8996782 - Attachment description: Bug 1478823: Add a chrome DOM event signalling that the body element is available. → Bug 1478823: Add a chrome DOM event signalling that the head element's children have been parsed.
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
Attachment #8996782 - Flags: review+
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
Attachment #8996783 - Attachment description: Bug 1478823: Start loading the default favicon as soon as the body element is seen. → Bug 1478823: Start loading the default favicon as soon as the head element is complete.
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
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
Flags: needinfo?(dtownsend)
https://hg.mozilla.org/mozilla-central/rev/c5386a64522c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Status: RESOLVED → REOPENED
Flags: needinfo?(dtownsend)
Resolution: FIXED → ---
Attachment #8996783 - Attachment is obsolete: true
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
Attachment #8998034 - Flags: review+
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
https://hg.mozilla.org/mozilla-central/rev/4073ae1a277f
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
Depends on: 1483222

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?

(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.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: