Closed Bug 728141 Opened 12 years ago Closed 12 years ago

Replace old synchronous favicons calls in browser

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

(Whiteboard: [snappy])

Attachments

(1 file, 1 obsolete file)

See bug 713642 for background.
Attached patch The patch (obsolete) — Splinter Review
Both of the code paths here are not covered by automated tests.

To test the first change:

1. Start the browser.
2. Navigate to a site with a favicon (for example www.mozilla.org).
3. Click a link on the page that leads to another page.
4. Click and pull down the history back button, and check that the correct
   site icon appears in the dropdown menu.

The second change is just changing a function name with an alias. I don't know
how I can test that code path manually. I think I can write an automated test
to check that setIcon actually loads the icon in addition to setting the
property of the tab, however it is also true that the change in the function
name probably has no real effect in any case. Marco, what do you think?
Attachment #598673 - Flags: review?(mak77)
Comment on attachment 598673 [details] [diff] [review]
The patch

>       try {
>-        let iconURL = Cc["@mozilla.org/browser/favicon-service;1"]
>-                         .getService(Ci.nsIFaviconService)
>-                         .getFaviconForPage(entry.URI).spec;
>-        item.style.listStyleImage = "url(" + iconURL + ")";
>-      } catch (ex) {}
>+        function FHM_faviconDataCallback(aURI) {
>+          let iconURL = PlacesUtils.favicons.getFaviconLinkForIcon(aURI).spec;
>+          item.style.listStyleImage = "url(" + iconURL + ")";
>+        }

Defining this function within the try-block seems pointless.

>+        PlacesUtils.favicons.getFaviconURLForPage(entry.URI,
>+                                                  FHM_faviconDataCallback);
>+      } catch (ex) {
>+        Cu.reportError(ex);
>+      }

getFaviconURLForPage shouldn't throw according to the idl.
Attachment #598673 - Flags: review?(mak77) → review-
Attached patch Revised patchSplinter Review
Attachment #598673 - Attachment is obsolete: true
Attachment #598902 - Flags: review?(dao)
Comment on attachment 598902 [details] [diff] [review]
Revised patch

I'd call the function FHM_getFaviconURLCallback, as it doesn't deal with data.

Is there a bug on providing a single async method in replacement for getFaviconForPage?
Attachment #598902 - Flags: review?(dao) → review+
(In reply to Dão Gottwald [:dao] from comment #4)
> Is there a bug on providing a single async method in replacement for
> getFaviconForPage?

Not that I know of. In fact, I think that a valid alternative would be to have a
"moz-favicon:<PageURI>" protocol that takes care of everything and can be set
directly on the element.
Pushed to inbound with the function name changed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/911c4d5ae460

There's a test case for the change in comment 1.
Target Milestone: --- → Firefox 13
https://hg.mozilla.org/mozilla-central/rev/911c4d5ae460
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [snappy]
No longer blocks: 730752
Blocks: 760971
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: