Closed
Bug 585515
Opened 14 years ago
Closed 14 years ago
Load favicon images into faviconService when allowed to
Categories
(SeaMonkey :: Tabbed Browser, defect)
SeaMonkey
Tabbed Browser
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.1a3
People
(Reporter: kairo, Assigned: kairo)
References
Details
Attachments
(1 file)
|
895 bytes,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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 2•14 years ago
|
||
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>
> <
|
||
Comment 3•14 years ago
|
||
Does the favicon service do the right thing if no favicon.ico exists?
|
|
||
Comment 4•14 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•14 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•14 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
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a3
|
|
||
Comment 7•14 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.
Description
•