Last Comment Bug 657961 - Use async API to get favicons for site permissions page
: Use async API to get favicons for site permissions page
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 7
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on: 655270 670341
Blocks: 573176
  Show dependency treegraph
 
Reported: 2011-05-18 09:52 PDT by :Margaret Leibovic
Modified: 2011-08-24 08:35 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.18 KB, patch)
2011-05-23 09:55 PDT, :Margaret Leibovic
no flags Details | Diff | Splinter Review
patch v2 (3.14 KB, patch)
2011-05-23 10:40 PDT, :Margaret Leibovic
sdwilsh: review+
Details | Diff | Splinter Review
patch v3 (5.31 KB, patch)
2011-05-24 07:38 PDT, :Margaret Leibovic
sdwilsh: review+
Details | Diff | Splinter Review

Description :Margaret Leibovic 2011-05-18 09:52:27 PDT
Once bug 655270 lands, we can use an async API to get the favicons for the site permissions page implemented in bug 573176.
Comment 1 :Margaret Leibovic 2011-05-23 09:55:42 PDT
Created attachment 534463 [details] [diff] [review]
patch

I got rid of the _favicon cache because getFavicon is only called once.
Comment 2 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-05-23 10:10:45 PDT
notice that the callback is declared with [function] so you don't have to build a object in js :)
Comment 3 :Margaret Leibovic 2011-05-23 10:40:41 PDT
Created attachment 534478 [details] [diff] [review]
patch v2

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.
Comment 4 Shawn Wilsher :sdwilsh 2011-05-23 16:30:43 PDT
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
Comment 5 :Margaret Leibovic 2011-05-24 07:38:19 PDT
Created attachment 534767 [details] [diff] [review]
patch v3

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.
Comment 6 Shawn Wilsher :sdwilsh 2011-05-24 11:15:29 PDT
Comment on attachment 534767 [details] [diff] [review]
patch v3

Review of attachment 534767 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh
Comment 7 Dão Gottwald [:dao] 2011-05-25 00:33:28 PDT
http://hg.mozilla.org/mozilla-central/rev/a0dc4a9ff450
Comment 8 Virgil Dicu [:virgil] [QA] 2011-08-24 02:25:09 PDT
Is this issue verifiable? Could anyone please provide some steps to reproduce in order to verify this issue in QA?
Comment 9 :Margaret Leibovic 2011-08-24 08:35:00 PDT
(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.

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