Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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.