Last Comment Bug 728168 - Replace old synchronous favicons calls in feeds
: Replace old synchronous favicons calls in feeds
Status: RESOLVED FIXED
[snappy:p1]
:
Product: Firefox
Classification: Client Software
Component: File Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 15
Assigned To: :Paolo Amadini
:
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 :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 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["@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 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:/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] 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 Ryan VanderMeulen [:RyanVM] 2012-04-29 14:04:57 PDT
http://hg.mozilla.org/mozilla-central/rev/bfc15198c097

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