Closed
Bug 657961
Opened 13 years ago
Closed 13 years ago
Use async API to get favicons for site permissions page
Categories
(Firefox :: Settings UI, defect)
Firefox
Settings UI
Tracking
()
RESOLVED
FIXED
Firefox 7
People
(Reporter: Margaret, Assigned: Margaret)
References
Details
Attachments
(1 file, 2 obsolete files)
5.31 KB,
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
Once bug 655270 lands, we can use an async API to get the favicons for the site permissions page implemented in bug 573176.
Assignee | ||
Comment 1•13 years ago
|
||
I got rid of the _favicon cache because getFavicon is only called once.
Assignee: nobody → margaret.leibovic
Status: NEW → ASSIGNED
Attachment #534463 -
Flags: review?(sdwilsh)
Comment 2•13 years ago
|
||
notice that the callback is declared with [function] so you don't have to build a object in js :)
Assignee | ||
Comment 3•13 years ago
|
||
Thanks, Marco :) I was debating between making getFavicon just take a nsIFaviconDataCallback as its callback, but I think I like abstracting that away in the Site object. However, I don't feel too strongly either way, so I can change it if that would be better.
Attachment #534463 -
Attachment is obsolete: true
Attachment #534463 -
Flags: review?(sdwilsh)
Attachment #534478 -
Flags: review?(sdwilsh)
Comment 4•13 years ago
|
||
Comment on attachment 534478 [details] [diff] [review] patch v2 Review of attachment 534478 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh with comments addressed ::: browser/components/preferences/aboutPermissions.js @@ +93,4 @@ > */ > + getFavicon: function Site_getFavicon(aCallback) { > + function faviconDataCallback(aURI, aDataLen, aData, aMimeType) { > + aCallback(aURI.spec); You should consider placing this call in a try-catch and Cu.reportError an errors that happen. @@ +98,5 @@ > + > + // Try to find favicion for both URIs. Callback will only be called if a > + // favicon URI is found. > + gFaviconService.getFaviconURLForPage(this.httpURI, faviconDataCallback); > + gFaviconService.getFaviconURLForPage(this.httpsURI, faviconDataCallback); In this case, we'll always prefer the https favicon (assuming they are different). This is likely fine. @@ +539,5 @@ > item.setAttribute("class", "site"); > item.setAttribute("value", aSite.host); > + > + // Set default favicon in case a favicon isn't found for the site. > + item.setAttribute("favicon", "chrome://mozapps/skin/places/defaultFavicon.png"); Everywhere else that we use this with the exception of panorama, we actually set the default in CSS, and then override it with the real one if needed. I think we should probably do the same here. https://mxr.mozilla.org/mozilla-central/search?string=chrome://mozapps/skin/places/defaultFavicon.png
Attachment #534478 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 5•13 years ago
|
||
Addressed comments. To set the default favicon in CSS, I also had to put in a style rule to get rid of the favicon image for the "All Sites" item. This adds a little bit more complexity, so I want to make sure it's still what you think we should do.
Attachment #534478 -
Attachment is obsolete: true
Attachment #534767 -
Flags: review?(sdwilsh)
Comment 6•13 years ago
|
||
Comment on attachment 534767 [details] [diff] [review] patch v3 Review of attachment 534767 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh
Attachment #534767 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 7•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/a0dc4a9ff450
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Comment 8•12 years ago
|
||
Is this issue verifiable? Could anyone please provide some steps to reproduce in order to verify this issue in QA?
Assignee | ||
Comment 9•12 years ago
|
||
(In reply to Virgil Dicu from comment #8) > Is this issue verifiable? Could anyone please provide some steps to > reproduce in order to verify this issue in QA? This is just an implementation issue. There isn't anything visible to the user, other than better performance.
You need to log in
before you can comment on or make changes to this bug.
Description
•