Last Comment Bug 627966 - Use Places db's favicon service
: Use Places db's favicon service
Status: RESOLVED FIXED
[qa-][cleanup]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P4 normal
: Firefox 6
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks: 649347
  Show dependency treegraph
 
Reported: 2011-01-21 22:56 PST by Michael Yoshitaka Erlewine [:mitcho]
Modified: 2016-04-12 14:00 PDT (History)
5 users (show)
bzbarsky: in‑testsuite?
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (1.96 KB, patch)
2011-04-12 11:23 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review
v2 (5.24 KB, patch)
2011-04-14 21:53 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Review
v3 (5.15 KB, patch)
2011-04-19 00:01 PDT, Raymond Lee [:raymondlee]
ian: review-
Details | Diff | Review
v4 (5.84 KB, patch)
2011-04-19 12:11 PDT, Raymond Lee [:raymondlee]
ian: review+
Details | Diff | Review
Patch for checkin (5.86 KB, patch)
2011-05-02 23:53 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Review

Description Michael Yoshitaka Erlewine [:mitcho] 2011-01-21 22:56:19 PST
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] 2011-01-21 23:00:32 PST
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] 2011-04-12 11:23:09 PDT
Created attachment 525448 [details] [diff] [review]
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] 2011-04-14 15:57:55 PDT
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] 2011-04-14 21:53:57 PDT
Created attachment 526182 [details] [diff] [review]
v2
Comment 5 Tim Taubert [:ttaubert] 2011-04-14 22:00:14 PDT
Comment on attachment 526182 [details] [diff] [review]
v2

Nice :)
Comment 6 Raymond Lee [:raymondlee] 2011-04-15 01:55:49 PDT
Comment on attachment 526182 [details] [diff] [review]
v2

Passed Try!
http://tbpl.mozilla.org/?tree=MozillaTry&rev=966ebcd35b72
Comment 7 Raymond Lee [:raymondlee] 2011-04-19 00:01:15 PDT
Created attachment 526939 [details] [diff] [review]
v3

Updated the patch to work with latest trunk
Comment 8 Ian Gilman [:iangilman] 2011-04-19 11:26:27 PDT
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] 2011-04-19 12:11:42 PDT
Created attachment 527066 [details] [diff] [review]
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] 2011-04-26 09:51:10 PDT
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] 2011-05-02 23:47:47 PDT
(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] 2011-05-02 23:53:06 PDT
Created attachment 529646 [details] [diff] [review]
Patch for checkin
Comment 13 Boris Zbarsky [:bz] 2011-05-03 12:26:43 PDT
Pushed http://hg.mozilla.org/projects/cedar/rev/5cd6dbdadeab
Comment 14 Boris Zbarsky [:bz] 2011-05-04 13:54:01 PDT
http://hg.mozilla.org/mozilla-central/rev/5cd6dbdadeab

Note You need to log in before you can comment on or make changes to this bug.