627966 - Use Places db's favicon service

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P4)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

We currently load our favicons straight from the source. We should be using the Places database's favicon service:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFaviconService#getFaviconImageForPage%28%29

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

This may, in fact, be just as easy as adding a "moz-anno:favicon:" to the URL:

https://bugzilla.mozilla.org/show_bug.cgi?id=467828

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

While a tab is loading, tab.image doesn't always be available.  Therefore, we don't have icon url to pass to the fav icon service through the moz-anno:favicon protocol. nsIFaviconService.getFaviconImageForPage() handles all that for us.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 525448 [details] [diff] [review]
v1

Looks good, but we'll need this for our AppTab icons as well. Please re-request feedback when this is included :)

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v2

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 526182 [details] [diff] [review]
v2

Nice :)

Comment 6

[Raymond Lee [:raymondlee]]

Comment on attachment 526182 [details] [diff] [review]
v2

Passed Try!
http://tbpl.mozilla.org/?tree=MozillaTry&rev=966ebcd35b72

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v3

Updated the patch to work with latest trunk

Comment 8

[Ian Gilman [:iangilman]]

Comment on attachment 526939 [details] [diff] [review]
v3

>   _onAppTabError : function(event) {
>     iQ(".appTabIcon", this.$appTabTray).each(function(icon) {
>       let $icon = iQ(icon);
>       if ($icon.data("xulTab") == event.target) {
>-        $icon.attr("src", Utils.defaultFaviconURL);
>+        $icon.attr("src",
>+                   gFavIconService.getFaviconImageForPage(
>+                     event.target.linkedBrowser.currentURI).spec);

Do we need this any more? Now that we're using the favicon service, will we ever get errors on favicons? 

If we will still get errors, then we should probably use Utils.defaultFaviconURL to deal with them; setting it to the same icon the favicon service gave us will just give us the error again.

Comment 9

[Raymond Lee [:raymondlee]]

Attachment: v4

Removed the _onAppTabError() since it's not necessary anymore.

Sent it to try and waiting for the results
http://tbpl.mozilla.org/?tree=MozillaTry&rev=aa0f61f22323

Comment 10

[Ian Gilman [:iangilman]]

Comment on attachment 527066 [details] [diff] [review]
v4

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

> Removed the _onAppTabError() since it's not necessary anymore.

Is that true? Please make sure by testing the web page listed in bug 600645 (though I suppose we have a unit test for it as well?). 

Anyway, if it's working with that page, r+

Comment 11

[Raymond Lee [:raymondlee]]

(In reply to comment #10)
> Comment on attachment 527066 [details] [diff] [review] [review]
> v4
> 
> Review of attachment 527066 [details] [diff] [review] [review]:
> 
> > Removed the _onAppTabError() since it's not necessary anymore.
> 
> Is that true? Please make sure by testing the web page listed in bug 600645
> (though I suppose we have a unit test for it as well?). 
> 
> Anyway, if it's working with that page, r+

The test for bug 600645 already covers that

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/projects/cedar/rev/5cd6dbdadeab

Comment 14

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/mozilla-central/rev/5cd6dbdadeab