657961 - Use async API to get favicons for site permissions page

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[:Margaret Leibovic]

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]

Attachment: patch

I got rid of the _favicon cache because getFavicon is only called once.

Comment 2

[Marco Bonardo [:mak]]

notice that the callback is declared with [function] so you don't have to build a object in js :)

Comment 3

[:Margaret Leibovic]

Attachment: 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]

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]

Attachment: 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]

Comment on attachment 534767 [details] [diff] [review]
patch v3

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

r=sdwilsh

Comment 7

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/a0dc4a9ff450

Comment 8

[Virgil Dicu [:virgil] [QA]]

Is this issue verifiable? Could anyone please provide some steps to reproduce in order to verify this issue in QA?

Comment 9

[:Margaret Leibovic]

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