728141 - Replace old synchronous favicons calls in browser

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Paolo Amadini]

See bug 713642 for background.

Comment 1

[:Paolo Amadini]

Attachment: The patch

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?

Comment 2

[Dão Gottwald [:dao]]

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.

Comment 3

[:Paolo Amadini]

Attachment: Revised patch

Comment 4

[Dão Gottwald [: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?

Comment 5

[:Paolo Amadini]

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

Comment 6

[:Paolo Amadini]

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.

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/911c4d5ae460