Last Comment Bug 585515 - Load favicon images into faviconService when allowed to
: Load favicon images into faviconService when allowed to
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: unspecified
: All All
: -- normal (vote)
: seamonkey2.1a3
Assigned To: Robert Kaiser (not working on stability any more)
:
Mentors:
Depends on: 554908
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-08 18:21 PDT by Robert Kaiser (not working on stability any more)
Modified: 2010-08-10 03:47 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1: Add the call in the right place (895 bytes, patch)
2010-08-08 18:26 PDT, Robert Kaiser (not working on stability any more)
neil: review+
Details | Diff | Review

Description Robert Kaiser (not working on stability any more) 2010-08-08 18:21:16 PDT
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. :)
Comment 1 Robert Kaiser (not working on stability any more) 2010-08-08 18:26:26 PDT
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.
Comment 2 Justin Wood (:Callek) 2010-08-08 19:40:57 PDT
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.
Comment 3 neil@parkwaycc.co.uk 2010-08-09 01:44:47 PDT
Does the favicon service do the right thing if no favicon.ico exists?
Comment 4 neil@parkwaycc.co.uk 2010-08-09 06:06:08 PDT
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]
Comment 5 Robert Kaiser (not working on stability any more) 2010-08-09 06:54:31 PDT
(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?
Comment 6 Robert Kaiser (not working on stability any more) 2010-08-09 18:53:12 PDT
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
Comment 7 neil@parkwaycc.co.uk 2010-08-10 03:47:01 PDT
(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.)

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