Display site's favicon immediately after site starts loading

RESOLVED INVALID

Status

()

Firefox
Address Bar
RESOLVED INVALID
8 years ago
8 years ago

People

(Reporter: Terepin, Assigned: Margaret)

Tracking

({polish})

Trunk
All
Windows 7
polish
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7pre) Gecko/20100917 Firefox/4.0b7pre

Current PL patch is showing "blank page" favicon when site is loading. This happens every time, regardless if the site was loaded before or not. Site's favicon should be displayed the moment user clicks on link, as showed on mockups.

Reproducible: Always
(Reporter)

Updated

8 years ago
Blocks: 544818
blocking2.0: --- → ?
Version: unspecified → Trunk
You should a least change your title to ... when a site is loading.
It seems that the favicon will be removed from the location bar : so maybe add too "... in the tab ..."
Final result is : Display site's favicon instead of "blank page" favicon in the tab when a site is loading.

Be precise when you fill bug (others can't be in your head ;-))
(Reporter)

Updated

8 years ago
Summary: Display site's favicon instead of "blank page" → Display site's favicon instead of "blank page" when site is loading
(Reporter)

Comment 2

8 years ago
It isn't necessary to add "on tab", because, as you said, it will be removed and it occours on both places.
Yes, but you forgot to add ... "blank page" FAVICON ...
without, others will thought that you are speaking about the label (especially with the "")
;-)
(Reporter)

Comment 4

8 years ago
Damn.
Summary: Display site's favicon instead of "blank page" when site is loading → Display site's favicon instead of "blank page" favicon when site is loading

Updated

8 years ago
Depends on: 597673
Status: UNCONFIRMED → NEW
Ever confirmed: true
Would take it, wouldn't block on it.
No longer blocks: 544818
blocking2.0: ? → -
Keywords: polish
Actually, changed my mind, this has a strong perception of perf impact.
blocking2.0: - → betaN+
(Reporter)

Comment 7

8 years ago
Changing the description since bug 597673 removed "blank page" favicon.
Summary: Display site's favicon instead of "blank page" favicon when site is loading → Display site's favicon immediately after site starts loading
(Assignee)

Updated

8 years ago
Assignee: nobody → margaret.leibovic
So, one option we have here is using what we already have stored in places.  Instead of setting the favicon to being what the site says it is, we'd set it to this url:
"moz-anno://favicon:" + [site favicon url]

That will asynchronously load the favicon from the places database, if we have it stored, or return the default one if we don't.

Things to watch for:
1) if the site has no favicon url, I'm not sure how this would behave.  The code may have to be careful.
2) we'll load the site's favicon when we don't have one.  You can use a history listener to listen for onPageChanged with the what being nsINavHistoryObserver::ATTRIBUTE_FAVICON.

Updated

8 years ago
Component: General → Location Bar
QA Contact: general → location.bar
Note that bug 600756 comment 1 indicates that there would be some lag with the solution in comment 8.
Alternatively, we can do a slight modification of comment 8 for (2).  When we call setAndLoadFaviconFor (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#599) pass in a callback (of the form in http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/public/nsIFaviconService.idl#320).  This will give us back an octet, which I think we can create a data URI out of with something like this (untested):
"data:" + mimeType + ";base64," + window.btoa(octet.join(""))
(In reply to comment #10)
>  This will give us back an octet, which I think we can create a data URI out of
> with something like this (untested):
> "data:" + mimeType + ";base64," + window.btoa(octet.join(""))
Or maybe something more like this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/search/nsSearchService.js#1454
a couple notes (I'm just trying to prevent surprises):

- a full history observer to listen for favicons changes could be expensive. What we pay is xpcom overhead since on each visit addition, or page change, this is a new observer that will have to be called. Could appear in Tp4. Btw, it could be a minor overhead.

- SetAndLoadFaviconForPage is nice, but will also try to fetch the icon from network if possible and put it in the db.  Being async the query is enqueued to page addition.  If we do the opposite and try to add icon before the page, we could pay a price because pages addition in not yet async in that method.  Since page is usually added before, it is almost not an issue today, but the opposite would make it critical to fix (we can copy/paste or share the similar step in async visits to make it async though).
- using moz-anno:icon could lose icon for about pages, since those pages are never added to history, thus the icons are never cached in the db.
(In reply to comment #12)
> - SetAndLoadFaviconForPage is nice, but will also try to fetch the icon from
> network if possible and put it in the db.  Being async the query is enqueued to
> page addition.  If we do the opposite and try to add icon before the page, we
> could pay a price because pages addition in not yet async in that method. 
> Since page is usually added before, it is almost not an issue today, but the
> opposite would make it critical to fix (we can copy/paste or share the similar
> step in async visits to make it async though).
However, we are already calling it, and I'm not proposing that we call it any more often than before.
(Assignee)

Comment 15

8 years ago
Bug 602964 makes this invalid.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → INVALID
(Reporter)

Comment 16

8 years ago
I hate bug 602964!
You need to log in before you can comment on or make changes to this bug.