728168 - Replace old synchronous favicons calls in feeds

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[:Paolo Amadini]

Attachment: For feedback

Not sure whom to ask about "feeds/src/WebContentConverter.js". It appears that
Shawn has worked on that code a long time ago :-)

While working on bug 713642, I noticed a call to getFaviconLinkForIcon that at
first glance appears to pass a Page URI instead of the Favicon URI that the
call expects. See the attached patch for the exact line.

How can I test the expected behavior there?

Comment 1

[Shawn Wilsher :sdwilsh]

Comment on attachment 598184 [details] [diff] [review]
For feedback

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

You'll want to talk to mak about the WebContentConverter stuff I think.  It's really old code!

::: browser/components/feeds/src/WebContentConverter.js
@@ +452,5 @@
>        var fis = Cc["@mozilla.org/browser/favicon-service;1"].
>                  getService(Ci.nsIFaviconService);
> +
> +      // TODO: Not sure how this is supposed to work... getFaviconLinkForIcon
> +      //       expects a Favicon URI, not a Page URI.

This code is probably just broken.

Comment 2

[:Paolo Amadini]

Attachment: The patch

So I figured out what this code was supposed to do, and fixed it to use the
site favicon like we did in bug 731942. To test it:

* Navigate to a site that has a favicon, for example <http:/www.mozilla.org/>
* Register a protocol handler on the same host from the Web Console:
      navigator.registerProtocolHandler("testscheme",
                                        "http://www.mozilla.org/test?url=%s",
                                        "Test Handler");
* Verify that an icon appears in the notification box. In this case, the icon
  should be <http://www.mozilla.org/favicon.ico>.
* Finally, ignore or dismiss the notification box.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 613527 [details] [diff] [review]
The patch

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

yes makes sense, for the same discussion we had regarding the handler.

Comment 4

[:Paolo Amadini]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc15198c097

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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