The default bug view has changed. See this FAQ.

Use async API to get favicons for site permissions page

RESOLVED FIXED in Firefox 7

Status

()

Firefox
Preferences
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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

6 years ago
Created attachment 534463 [details] [diff] [review]
patch

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)
notice that the callback is declared with [function] so you don't have to build a object in js :)
(Assignee)

Comment 3

6 years ago
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.
Attachment #534463 - Attachment is obsolete: true
Attachment #534463 - Flags: review?(sdwilsh)
Attachment #534478 - Flags: review?(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
Attachment #534478 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 5

6 years ago
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.
Attachment #534478 - Attachment is obsolete: true
Attachment #534767 - Flags: review?(sdwilsh)
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

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/a0dc4a9ff450
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
(Assignee)

Updated

6 years ago
Depends on: 670341
Is this issue verifiable? Could anyone please provide some steps to reproduce in order to verify this issue in QA?
(Assignee)

Comment 9

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