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

RESOLVED FIXED in seamonkey2.12

Status

RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: philip.chee, Assigned: sgautherie)

Tracking

Trunk
seamonkey2.12
Dependency tree / graph
Bug Flags:
in-testsuite +

SeaMonkey Tracking Flags

(seamonkey2.10 wontfix, seamonkey2.11 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

7 years ago
Depends on: 730849, 730837
(Reporter)

Updated

7 years ago
Summary: Replace all old synchronous favicons calls in the codebase with Asynchronous Favicons API. → Replace all old synchronous favicons calls in Suite with Asynchronous Favicons API.
(Assignee)

Comment 1

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

Ftr, this is Toolkit: nothing to port.
Depends on: 713642, 728141
(Assignee)

Updated

7 years ago
Blocks: 467530
Target Milestone: --- → seamonkey2.10
(Assignee)

Updated

7 years ago
status-seamonkey2.10: --- → affected
Target Milestone: seamonkey2.10 → seamonkey2.11
(Assignee)

Updated

6 years ago
Depends on: 728174
(Assignee)

Updated

6 years ago
status-seamonkey2.11: --- → affected
Target Milestone: seamonkey2.11 → seamonkey2.12
(Assignee)

Updated

6 years ago
Depends on: 750855
(Assignee)

Comment 2

6 years ago
Created 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]

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

[Approval Request Comment]
No risk, test-only.
Attachment #624364 - Flags: review?(iann_bugzilla)
Attachment #624364 - Flags: approval-comm-aurora?
(Assignee)

Comment 3

6 years ago
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

6 years ago
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.
Attachment #624364 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 5

6 years ago
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
Attachment #624364 - Attachment description: (Av1) Port |Bug 728174 - Replace old synchronous favicons calls in the bookmarks HTML import| to SeaMonkey → (Av1) Port |Bug 728174 - Replace old synchronous favicons calls in the bookmarks HTML import| to SeaMonkey [Checked in: Comment 5]

Updated

6 years ago
Attachment #624364 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Updated

6 years ago
No longer blocks: 467530
No longer depends on: 713642, 730837, 728141, 728174, 730849, 750855
Keywords: checkin-needed
Whiteboard: [c-n: c92117e25748 to c-a]
(Assignee)

Updated

6 years ago
Blocks: 467530
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
Attachment #624364 - Attachment description: (Av1) Port |Bug 728174 - Replace old synchronous favicons calls in the bookmarks HTML import| to SeaMonkey [Checked in: Comment 5] → (Av1) Port |Bug 728174 - Replace old synchronous favicons calls in the bookmarks HTML import| to SeaMonkey [Checked in: Comments 5 and 6]
Keywords: checkin-needed
Whiteboard: [c-n: c92117e25748 to c-a]
(Assignee)

Updated

6 years ago
status-seamonkey2.11: affected → fixed
(Assignee)

Comment 7

6 years ago
Created attachment 629558 [details] [diff] [review]
(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)
Attachment #629558 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 8

6 years ago
(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)
Assignee: nobody → sgautherie.bz
No longer blocks: 467530
Status: NEW → ASSIGNED
status-seamonkey2.10: affected → wontfix
Flags: in-testsuite+

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
(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.
Blocks: 713642
No longer depends on: 713642

Updated

6 years ago
Attachment #629558 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Comment 11

6 years ago
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.)
Attachment #629558 - Attachment description: (Bv1) Port |Bug 728141 - Replace old synchronous favicons calls in browser| to SeaMonkey → (Bv1) Port |Bug 728141 - Replace old synchronous favicons calls in browser| to SeaMonkey [Checked in: Comment 11]
Attachment #629558 - Flags: approval-comm-aurora?

Updated

6 years ago
Attachment #629558 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: 6417fa1a06e1 to c-a]
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...)
Attachment #629558 - Attachment description: (Bv1) Port |Bug 728141 - Replace old synchronous favicons calls in browser| to SeaMonkey [Checked in: Comment 11] → (Bv1) Port |Bug 728141 - Replace old synchronous favicons calls in browser| to SeaMonkey [Checked in: Comments 11 and 12]
Keywords: checkin-needed
Whiteboard: [c-n: 6417fa1a06e1 to c-a]
You need to log in before you can comment on or make changes to this bug.