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)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: Margaret, Unassigned)
Details
Attachments
(1 file)
1.25 KB,
patch
|
lucasr
:
feedback+
|
Details | Diff | Splinter Review |
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
Comment 1•12 years ago
|
||
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.
Comment 2•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #801112 -
Flags: review? → review?(margaret.leibovic)
Updated•11 years ago
|
Assignee: nobody → ckitching
Comment 3•11 years ago
|
||
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...).
Updated•11 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 4•11 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 5•11 years ago
|
||
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+
Reporter | ||
Comment 6•11 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)
Comment 7•11 years ago
|
||
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?
Updated•10 years ago
|
Assignee: chriskitching → nobody
Updated•10 years ago
|
Status: ASSIGNED → NEW
Component: General → Favicon Handling
Hardware: ARM → All
Comment 8•3 years ago
|
||
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
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•