bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

Look into updating favicon immediately, even if page is still loading




Firefox for Android
Favicon Handling
6 years ago
4 years ago


(Reporter: Margaret, Unassigned)



Firefox Tracking Flags

(Not tracked)



(1 attachment)



6 years ago
Follow-up to bug 715263 comment 22. 

Right now in handleLinkAdded, we wait to see if the page is done before changing the favicon, but it might be better to just immediately update the favicon
Note: Setting the favicon while the page is still loading would not hide the throbber. It would merely set the favicon for the tab. When the throbber is hidden, the right favicon would be ready to show right away. Right now, you can see a flash of the "no favicon" favicon after the throbber stops.
Created attachment 801112 [details] [diff] [review]
Load favicons earlier.

This simple patch appears to have the desired effect. (Independent from ongoing Favicon overhaul patches)
It seems that when a Favicon is specified by the page, a tab changed event of type LINK_FAVICON is created - if we immediately start loading the Favicon, we load it earlier.

If none is specified, we load the Favicon when the page has completed loading (We are thus certain no other Favicon is going to be specified), and we use the default favicon URL (whatever.something/favicon.ico).
Attachment #801112 - Flags: review?
Attachment #801112 - Flags: review? → review?(margaret.leibovic)
Assignee: nobody → ckitching
I always assumed we did this to avoid doing extra work during pageload if we didn't need to do it (Note this happens for background tabs too though...).

Comment 4

5 years ago
Comment on attachment 801112 [details] [diff] [review]
Load favicons earlier.

I dug way through blame to find that we originally added this logic in bug 697194, so I want to ask lucasr what he thinks of this change.
Attachment #801112 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 801112 [details] [diff] [review]
Load favicons earlier.

Review of attachment 801112 [details] [diff] [review]:

Looks fine. The only thing I wonder is whether this will cause page load regressions or not. I assume the intended improvement here is to be able to show the favicon just after the throbber stops spinning.
Attachment #801112 - Flags: feedback?(lucasr.at.mozilla) → feedback+

Comment 6

5 years ago
Comment on attachment 801112 [details] [diff] [review]
Load favicons earlier.

Review of attachment 801112 [details] [diff] [review]:

::: mobile/android/base/BrowserApp.java
@@ +232,5 @@
>              case PAGE_SHOW:
>                  loadFavicon(tab);
>                  break;
>              case LINK_FAVICON:
> +                loadFavicon(tab);

This will only affect pages that include favicons in link tags. I don't love that we'll still always end up calling loadFavicon on page show. I think that if we are going to go this route of immediately loading favicons as we process link tags, we should add some logic to avoid loading the favicon again on page show.
Attachment #801112 - Flags: review?(margaret.leibovic)
Yes, this approach seems incorrect.

We can load the favicon at the point when we know there are no more link tags coming - we can then pick the tag we care about and load the appropriate favicon or, if none exists, load from the default url.
But - when have all link tags arrived? Link tags can only exist in the header, right? Do we have an event for "end of header"?
If we do, then we simply collect link tags in LOAD_FAVICON and then do the selection logic in the HEADER_LOADED event.

What happens if you put a link tag with a favicon in an iframe in the page? I guess that should be ignored?
Assignee: chriskitching → nobody
Component: General → Favicon Handling
Hardware: ARM → All
You need to log in before you can comment on or make changes to this bug.