Closed Bug 585515 Opened 14 years ago Closed 14 years ago

Load favicon images into faviconService when allowed to

Categories

(SeaMonkey :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.1a3

People

(Reporter: kairo, Assigned: kairo)

References

Details

Attachments

(1 file)

In bug 554908, I added loading of site icons into the favicon service, but we don't do that for actual favicons yet, not even where those are generally loaded. Once one knows where, it's trivial to add the favicon service call there as well. :)
Here's the trivial patch to do this and load the real favicons into the favicon service when we load them anyhow.

We still should go and switch the actual code that loads icons into tabs and the page proxy icon to use the favicon service, but this patch wins us having the icons in bookmarks and history views - including the personal toolbar, which is a quite good step for now.

As with my other tabbrowser patch today, whoever of you two gets to it first, I'll take your review, and please cancel the request for the other review.
Attachment #463984 - Flags: review?(neil)
Attachment #463984 - Flags: review?(bugspam.Callek)
Comment on attachment 463984 [details] [diff] [review]
v1: Add the call in the right place

>diff --git a/suite/browser/tabbrowser.xml b/suite/browser/tabbrowser.xml
>--- a/suite/browser/tabbrowser.xml
>+++ b/suite/browser/tabbrowser.xml
>@@ -825,16 +825,20 @@
> 
>       <method name="loadFavIcon">
>         <parameter name="aURI"/>
>         <parameter name="aAttr"/>
>         <parameter name="aElt"/>
>         <body>
>           <![CDATA[
>             var iconURL = this.buildFavIconString(aURI);
>+            if (aURI && this.mFaviconService) {
>+              let iURI = Services.io.newURI(iconURL, null, null);
>+              this.mFaviconService.setAndLoadFaviconForPage(aURI, iURI, false);
>+            }

I could just be tired here, but iconURL is built from aURI (with /favicon.ico used) but in the if we check for aURI not the built URL, though we create iURI the iconURL. 

Given my confusion here, I think its safer to let Neil give official review.
Attachment #463984 - Flags: review?(bugspam.Callek)
Does the favicon service do the right thing if no favicon.ico exists?
Comment on attachment 463984 [details] [diff] [review]
v1: Add the call in the right place

>+            if (aURI && this.mFaviconService) {
Nit: aURI should always be set (to /favicon.ico)

>+              let iURI = Services.io.newURI(iconURL, null, null);
iURI? Just uri works.

>             var entry = this.openCacheEntry(iconURL, Components.interfaces.nsICache.ACCESS_READ);
>             if (!entry)
[Follow-up: Switch to isFailedFavicon]
Attachment #463984 - Flags: review?(neil) → review+
(In reply to comment #3)
> Does the favicon service do the right thing if no favicon.ico exists?

http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/public/nsIFaviconService.idl#70 seems to indicate that it does: "Icons that fail to load will automatically be added to the failed favicon cache"

(In reply to comment #4)
> [Follow-up: Switch to isFailedFavicon]

Is this in conjunction with actually fetching the icon from there, or is this comment its own step?
After some talk on IRC, Callek approved landing on the tree, given the fact that the bustage is probably just shared builds and static ones might succeed, i.e. we'll have regular nightlies, and we want this to be fixed there.
Pushed as http://hg.mozilla.org/comm-central/rev/5adda4f06320
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a3
(In reply to comment #5)
> (In reply to comment #3)
> > Does the favicon service do the right thing if no favicon.ico exists?
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/places/public/nsIFaviconService.idl#70
> seems to indicate that it does: "Icons that fail to load will automatically be
> added to the failed favicon cache"
Actually I was thinking more along the lines of not deleting the previous icon.

> (In reply to comment #4)
> > [Follow-up: Switch to isFailedFavicon]
> Is this in conjunction with actually fetching the icon from there, or is this
> comment its own step?
No, just use the favicon service to track the failed icons instead of each tabbrowser having its own cache. (I don't know whether tabbrowsers share cache with each other, the point is they don't share with the favicon service.)
You need to log in before you can comment on or make changes to this bug.