Favicons load really slow in home panels

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
Favicon Handling
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sebastian, Assigned: ahunt)

Tracking

unspecified
Firefox 49
All
Android
Points:
---

Firefox Tracking Flags

(firefox49 fixed, fennec49+)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Scrolling through the home panels in Nightly the favicons load really slowly, one at a time. It's almost 1 sec / icon. This seems to be a new regression.
(Assignee)

Comment 1

2 years ago
Created attachment 8759763 [details]
Bug 1277854 - Back out bug 1276050 to ensure we load only local favicons whe requested

(Backed out changeset 9575e608cabe)

loadUncachedFavicon allows loading favicons from the internet, whereas
getSizedFaviconForPageFromLocal restricts itself to the favicon cache.
(LoadFaviconTask's last parameter is (boolean) onlyFromLocal, which is
set to true in getSizedFaviconForPageFromLocal.

We could probably replace this with an Enum to make the parameter's purpose
more obvious.

Review commit: https://reviewboard.mozilla.org/r/57634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57634/
Attachment #8759763 - Flags: review?(s.kaspari)
Attachment #8759764 - Flags: review?(s.kaspari)
(Assignee)

Comment 2

2 years ago
Created attachment 8759764 [details]
Bug 1277854 - Post: add comment explaining local loading

Review commit: https://reviewboard.mozilla.org/r/57636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57636/
(Assignee)

Comment 3

2 years ago
Regression caused by Bug 1276050.

It's simplest to back it out for now. We could probably clean up favicon loading to make this more obvious, or be more sophisticated and paint the fallback icon initially if we have to download icons from the internet - but that doesn't seem like a good use of time right now.
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Priority: -- → P1
(Reporter)

Comment 4

2 years ago
Comment on attachment 8759763 [details]
Bug 1277854 - Back out bug 1276050 to ensure we load only local favicons whe requested

https://reviewboard.mozilla.org/r/57634/#review54452
Attachment #8759763 - Flags: review?(s.kaspari) → review+
(Reporter)

Comment 5

2 years ago
Comment on attachment 8759764 [details]
Bug 1277854 - Post: add comment explaining local loading

https://reviewboard.mozilla.org/r/57636/#review54454
Attachment #8759764 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 6

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/367c7cce892f627ad7e1f1ceba0f04feb601522a
Bug 1277854 - Back out bug 1276050 to ensure we load only local favicons when requested r=sebastian

https://hg.mozilla.org/integration/fx-team/rev/2086e849a7e6f30fc56100f04847143d6e575ea2
Bug 1277854 - Post: add comment explaining local loading r=sebastian

Comment 7

2 years ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/367c7cce892f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
tracking-fennec: ? → 49+
I think this backout may need uplifted to 49.
(Assignee)

Comment 10

2 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #9)
> I think this backout may need uplifted to 49.

I *think* this just made it, although I'm not 100% confiden in my mercurial foo:
https://hg.mozilla.org/releases/mozilla-aurora/rev/367c7cce892f
You need to log in before you can comment on or make changes to this bug.