Closed Bug 728168 Opened 8 years ago Closed 8 years ago

Replace old synchronous favicons calls in feeds


(Firefox :: File Handling, defect)

Not set



Firefox 15


(Reporter: Paolo, Assigned: Paolo)



(Whiteboard: [snappy:p1])


(1 file, 1 obsolete file)

Attached patch For feedback (obsolete) — Splinter Review
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?
Attachment #598184 - Flags: feedback?(sdwilsh)
Assignee: nobody → paolo.mozmail
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[";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.
Attachment #598184 - Flags: feedback?(sdwilsh) → feedback+
Whiteboard: [snappy]
Whiteboard: [snappy] → [snappy:p1]
Attached patch The patchSplinter Review
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:/>
* Register a protocol handler on the same host from the Web Console:
                                        "Test Handler");
* Verify that an icon appears in the notification box. In this case, the icon
  should be <>.
* Finally, ignore or dismiss the notification box.
Attachment #598184 - Attachment is obsolete: true
Attachment #613527 - Flags: review?(mak77)
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.
Attachment #613527 - Flags: review?(mak77) → review+
Target Milestone: --- → Firefox 15
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.