Closed Bug 750855 Opened 8 years ago Closed 8 years ago

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

Categories

(SeaMonkey :: Feed Discovery and Preview, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.12

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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).
Attachment #620035 - Flags: review?(neil)
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 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.
Attachment #620035 - Flags: review?(neil) → review-
Attachment #620035 - Attachment is obsolete: true
Attachment #621458 - Flags: review?(neil)
Attachment #621458 - Flags: review?(neil) → review+
Comment on attachment 621458 [details] [diff] [review]
patch v1a [Checkin: Comment 4]

http://hg.mozilla.org/comm-central/rev/664d3445e151
Attachment #621458 - Attachment description: patch v1a → patch v1a [Checkin: Comment 4]
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.12
Blocks: 730752
Depends on: 728168
No longer blocks: 730752
You need to log in before you can comment on or make changes to this bug.