597665 - Display site's favicon immediately after site starts loading

Status: RESOLVED INVALID

(Firefox :: Address Bar, defect)

Description

[Peter Henkel [:Terepin]]

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

Comment 1

[Nicolas Mandil (:NicolasWeb)]

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 ;-))

Comment 2

[Peter Henkel [:Terepin]]

It isn't necessary to add "on tab", because, as you said, it will be removed and it occours on both places.

Comment 3

[Nicolas Mandil (:NicolasWeb)]

Yes, but you forgot to add ... "blank page" FAVICON ...
without, others will thought that you are speaking about the label (especially with the "")
;-)

Comment 4

[Peter Henkel [:Terepin]]

Damn.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Would take it, wouldn't block on it.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Actually, changed my mind, this has a strong perception of perf impact.

Comment 7

[Peter Henkel [:Terepin]]

Changing the description since bug 597673 removed "blank page" favicon.

Comment 8

[Shawn Wilsher :sdwilsh]

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.

Comment 9

[Shawn Wilsher :sdwilsh]

Note that bug 600756 comment 1 indicates that there would be some lag with the solution in comment 8.

Comment 10

[Shawn Wilsher :sdwilsh]

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(""))

Comment 11

[Shawn Wilsher :sdwilsh]

(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

Comment 12

[Marco Bonardo [:mak]]

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).

Comment 13

[Marco Bonardo [:mak]]

- 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.

Comment 14

[Shawn Wilsher :sdwilsh]

(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.

Comment 15

[:Margaret Leibovic]

Bug 602964 makes this invalid.

Comment 16

[Peter Henkel [:Terepin]]

I hate bug 602964!