Load favicon images into faviconService when allowed to

RESOLVED FIXED in seamonkey2.1a3

Status

SeaMonkey
Tabbed Browser
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Robert Kaiser, Assigned: Robert Kaiser)

Tracking

unspecified
seamonkey2.1a3

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

7 years ago
Created attachment 463984 [details] [diff] [review]
v1: Add the call in the right place

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)

Comment 3

7 years ago
Does the favicon service do the right thing if no favicon.ico exists?

Comment 4

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

Comment 5

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

Comment 6

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a3

Comment 7

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