Closed Bug 759501 Opened 12 years ago Closed 3 years ago

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

Categories

(Firefox for Android Graveyard :: Favicon Handling, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: Margaret, Unassigned)

Details

Attachments

(1 file)

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.
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...).
Status: NEW → ASSIGNED
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 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
Status: ASSIGNED → NEW
Component: General → Favicon Handling
Hardware: ARM → All
We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INCOMPLETE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.