Replace old synchronous favicons calls in feeds

RESOLVED FIXED in Firefox 15

Status

()

Firefox
File Handling
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Paolo, Assigned: Paolo)

Tracking

Trunk
Firefox 15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [snappy:p1])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 598184 [details] [diff] [review]
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?
Attachment #598184 - Flags: feedback?(sdwilsh)

Updated

6 years ago
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
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.
Attachment #598184 - Flags: feedback?(sdwilsh) → feedback+
Whiteboard: [snappy]

Updated

5 years ago
Whiteboard: [snappy] → [snappy:p1]
(Assignee)

Comment 2

5 years ago
Created attachment 613527 [details] [diff] [review]
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.
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+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc15198c097
Target Milestone: --- → Firefox 15
http://hg.mozilla.org/mozilla-central/rev/bfc15198c097
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Blocks: 750855
You need to log in before you can comment on or make changes to this bug.