Use Places db's favicon service

RESOLVED FIXED in Firefox 6

Status

Firefox Graveyard
Panorama
P4
normal
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: mitcho, Assigned: raymondlee)

Tracking

Trunk
Firefox 6
Bug Flags:
in-testsuite ?

Details

(Whiteboard: [qa-][cleanup])

Attachments

(1 attachment, 4 obsolete attachments)

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

Updated

7 years ago
Whiteboard: [qa-][cleanup][good first bug]
(Reporter)

Comment 1

7 years ago
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
(Assignee)

Updated

6 years ago
Blocks: 649347
(Assignee)

Comment 2

6 years ago
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.
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #525448 - Flags: feedback?(tim.taubert)
Version: unspecified → Trunk
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 :)
Attachment #525448 - Flags: feedback?(tim.taubert)
(Assignee)

Comment 4

6 years ago
Created attachment 526182 [details] [diff] [review]
v2
Attachment #525448 - Attachment is obsolete: true
Attachment #526182 - Flags: feedback?(tim.taubert)
Comment on attachment 526182 [details] [diff] [review]
v2

Nice :)
Attachment #526182 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Updated

6 years ago
Attachment #526182 - Flags: review?(ian)
(Assignee)

Comment 6

6 years ago
Comment on attachment 526182 [details] [diff] [review]
v2

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

Comment 7

6 years ago
Created attachment 526939 [details] [diff] [review]
v3

Updated the patch to work with latest trunk
Attachment #526182 - Attachment is obsolete: true
Attachment #526939 - Flags: review?(ian)
Attachment #526182 - Flags: review?(ian)
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.
Attachment #526939 - Flags: review?(ian) → review-
(Assignee)

Comment 9

6 years ago
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
Attachment #526939 - Attachment is obsolete: true
Attachment #527066 - Flags: review?(ian)
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+
Attachment #527066 - Flags: review?(ian) → review+
(Assignee)

Comment 11

6 years ago
(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
(Assignee)

Comment 12

6 years ago
Created attachment 529646 [details] [diff] [review]
Patch for checkin
Attachment #527066 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
Pushed http://hg.mozilla.org/projects/cedar/rev/5cd6dbdadeab
Keywords: checkin-needed
Whiteboard: [qa-][cleanup][good first bug] → [qa-][cleanup][fixed-in-cedar]
Target Milestone: Future → Firefox 6
http://hg.mozilla.org/mozilla-central/rev/5cd6dbdadeab
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [qa-][cleanup][fixed-in-cedar] → [qa-][cleanup]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.