750855 - Port |Bug 728168 - Replace old synchronous favicons calls in feeds|

Status: RESOLVED FIXED

(SeaMonkey :: Feed Discovery and Preview, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

Bug 728168 made two changes:
1. call setAndFetchFaviconForPage instead of setAndLoadFaviconForPage. AFAIU this somehow works despite the former being part of mozIAsyncFavicons.
2. dropped used of getFaviconLinkForIcon (seems using this there was wrong in the first place, see bug 728168 comment 0)

See bug 728168 comment 2 for how to test this if need be (I didn't).

Comment 1

[[e-mail]]

Comment on attachment 620035 [details] [diff] [review]
patch

>-    this._faviconService.setAndLoadFaviconForPage(readerURI, faviconURI, false,
>+    this._faviconService.setAndFetchFaviconForPage(readerURI, faviconURI, false,
"somehow works"? Why not change the getter for _faviconService to use the correct interface? (Renaming the variable is optional.)

>+      var notificationIcon = uri.prePath + "/favicon.ico";
Interesting, suite prefers to use uri.resolve("/favicon.ico") except ironically later in this very file!

Comment 2

[[e-mail]]

Comment on attachment 620035 [details] [diff] [review]
patch

r- because it needs the right interface on the getter.

Also bz had a very slight preference for .resolve("/favicon.ico") too.

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v1a [Checkin: Comment 4]

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 621458 [details] [diff] [review]
patch v1a [Checkin: Comment 4]

http://hg.mozilla.org/comm-central/rev/664d3445e151