Last Comment Bug 728168 - Replace old synchronous favicons calls in feeds
: Replace old synchronous favicons calls in feeds
Product: Firefox
Classification: Client Software
Component: File Handling (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 15
Assigned To: :Paolo Amadini
: :Paolo Amadini
Depends on:
Blocks: asyncFaviconCallers 750855
  Show dependency treegraph
Reported: 2012-02-17 02:43 PST by :Paolo Amadini
Modified: 2012-05-14 20:35 PDT (History)
8 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

For feedback (2.52 KB, patch)
2012-02-17 02:43 PST, :Paolo Amadini
sdwilsh: feedback+
Details | Diff | Splinter Review
The patch (2.68 KB, patch)
2012-04-10 02:42 PDT, :Paolo Amadini
mak77: review+
Details | Diff | Splinter Review

Description User image :Paolo Amadini 2012-02-17 02:43:33 PST
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?
Comment 1 User image Shawn Wilsher :sdwilsh 2012-02-21 20:05:26 PST
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.
Comment 2 User image :Paolo Amadini 2012-04-10 02:42:41 PDT
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:/>
* 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.
Comment 3 User image Marco Bonardo [::mak] 2012-04-12 12:36:35 PDT
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 5 User image Ryan VanderMeulen [:RyanVM] 2012-04-29 14:04:57 PDT

Note You need to log in before you can comment on or make changes to this bug.