730752 - Replace all old synchronous favicons calls in Suite with Asynchronous Favicons API.

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Philip Chee]

References:
Bug 713642 - (asyncFaviconCallers) Replace all old synchronous favicons calls in the codebase.
Bug 713269 - Use Asynchronous Favicons API for PlacesUtils.jsm.
Bug 728141 - Replace old synchronous favicons calls in browser.

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to Philip Chee from comment #0)
> Bug 713269 - Use Asynchronous Favicons API for PlacesUtils.jsm.

Ftr, this is Toolkit: nothing to port.

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Port |Bug 728174 - Replace old synchronous favicons calls in the bookmarks HTML import| to SeaMonkey [Checked in: Comments 5 and 6]

Succeeded (with a workaround) as
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=dc5c1f8f91d4

[Approval Request Comment]
No risk, test-only.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 624364 [details] [diff] [review]
(Av1) Port |Bug 728174 - Replace old synchronous favicons calls in the bookmarks HTML import| to SeaMonkey
[Checked in: Comments 5 and 6]

This is a trivial code copy, except for TEST_FAVICON_DATA_SIZE value.

Comment 4

[Ian Neal]

Comment on attachment 624364 [details] [diff] [review]
(Av1) Port |Bug 728174 - Replace old synchronous favicons calls in the bookmarks HTML import| to SeaMonkey
[Checked in: Comments 5 and 6]

Land this on trunk and once you're happy tests are fine, will consider the approval request.

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 624364 [details] [diff] [review]
(Av1) Port |Bug 728174 - Replace old synchronous favicons calls in the bookmarks HTML import| to SeaMonkey
[Checked in: Comments 5 and 6]

https://hg.mozilla.org/comm-central/rev/c92117e25748

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 624364 [details] [diff] [review]
(Av1) Port |Bug 728174 - Replace old synchronous favicons calls in the bookmarks HTML import| to SeaMonkey
[Checked in: Comments 5 and 6]

http://hg.mozilla.org/releases/comm-aurora/rev/abd0b7f43287

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Port |Bug 728141 - Replace old synchronous favicons calls in browser| to SeaMonkey [Checked in: Comments 11 and 12]

I wonder whether this is it or if being async breaks these code.
(Untested)

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #7)

> I wonder whether this is it or if being async breaks these code.

{
(In reply to Paolo Amadini [:paolo] from bug 728141 comment #1)
> The second change is just changing a function name with an alias.
}
answers this: trivial change only ;-)

> (Untested)

Comment 9

[Ian Neal]

There might be one test that needs fixing:
http://mxr.mozilla.org/comm-central/source/suite/common/places/tests/unit/test_bookmarks_html.js#450
I believe you will have to use PlacesUtils.favicons.getFaviconURLForPage instead but Firefox haven't fixed their equivalent test yet, so maybe is not needed/been forgotten/still to be done.

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to Ian Neal from comment #9)
> Firefox haven't fixed their equivalent test yet, so maybe is not
> needed/been forgotten/still to be done.

Iiuc, that is bug 740468.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 629558 [details] [diff] [review]
(Bv1) Port |Bug 728141 - Replace old synchronous favicons calls in browser| to SeaMonkey
[Checked in: Comments 11 and 12]

http://hg.mozilla.org/comm-central/rev/6417fa1a06e1


[Approval Request Comment]
No risk, trivial aliasing.
(Too late for SM 2.10.)

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 629558 [details] [diff] [review]
(Bv1) Port |Bug 728141 - Replace old synchronous favicons calls in browser| to SeaMonkey
[Checked in: Comments 11 and 12]

http://hg.mozilla.org/releases/comm-beta/rev/d64fc962b931

(c-b is the new c-a...)