audit and remove unnecessary nsISimpleEnumerator usage , move over to the JS iteration protocol
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: khushil324)
References
Details
Attachments
(7 files, 12 obsolete files)
11.78 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
14.73 KB,
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
78.07 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
104.96 KB,
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
8.72 KB,
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
1.12 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
2.12 KB,
patch
|
khushil324
:
review+
|
Details | Diff | Splinter Review |
Bug 1484496 made JavaScript callers able to use js iterations i.e. for...of.
Comm-central changes from that bug are in bug 1485820.
We should move more of the js callers to that to make things less XPCOM. Seems you don't need to QueryInterface the values either!
Many of the things needing auditing: https://searchfox.org/comm-central/search?q=fixIterator(&case=false®exp=false&path=
E.g. https://searchfox.org/comm-central/rev/8c5e7bafb442e0f691665d39c36d660cfd2adecd/calendar/base/content/calendar-chrome-startup.js#135 would work just fine as
for (let win of Services.ww.getWindowEnumerator()) {
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Reporter | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #2)
Looks good -
https://treeherder.mozilla.org/#/jobs?repo=try-comm-
central&revision=81066f381630f30debc7bdf81313a05a1ae7dcb6&selectedJob=2831771
05
(Didn't check if there are more to do.)
Is this unit test failure known or caused by this patch? I am seeing the same error in local machine without this patch.
Reporter | ||
Comment 4•5 years ago
|
||
Known issue.
Assignee | ||
Updated•5 years ago
|
Comment 5•5 years ago
|
||
Yes, known issue, the "1st of the month" bug, bug 1244818. It's starred on the C-C tree.
Assignee | ||
Comment 6•5 years ago
|
||
Linting error corrected.
Assignee | ||
Comment 7•5 years ago
|
||
Reporter | ||
Comment 8•5 years ago
|
||
Assignee | ||
Comment 9•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Reporter | ||
Comment 10•5 years ago
|
||
Comment 11•5 years ago
|
||
Assignee | ||
Comment 12•5 years ago
•
|
||
(In reply to Patrick Cloke [:clokep] from comment #11)
I have a couple of questions about missing QIs though.
No, we don't need them. Look at here what Firefox people have done: https://phabricator.services.mozilla.com/D3729 (removed QIs also)
Assignee | ||
Comment 13•5 years ago
|
||
Assignee | ||
Comment 14•5 years ago
|
||
Removed one usage of hasMore() from chat/.
Comment 15•5 years ago
|
||
Assignee | ||
Comment 16•5 years ago
|
||
Check in needed for chat patch.
Assignee | ||
Updated•5 years ago
|
Reporter | ||
Comment 17•5 years ago
|
||
Updated•5 years ago
|
Comment 18•5 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/089f6048f53f
Convert remaining nsISimpleEnumerator users to use JS iteration in /chat. r=mkmelin,clokep
Assignee | ||
Comment 19•5 years ago
|
||
Assignee | ||
Comment 20•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #17)
why do you need to switch to fixIterator?
It was showing an error that the subject is not iterable. So I have used fixIterator.
please inline enumerator
We are using it at multiple places in this file. So I have kept it as it is.
Assignee | ||
Comment 21•5 years ago
|
||
Assignee | ||
Comment 22•5 years ago
|
||
![]() |
||
Comment 23•5 years ago
|
||
Reporter | ||
Comment 24•5 years ago
|
||
(In reply to Khushil Mistry [:khushil324] from comment #20)
(In reply to Magnus Melin [:mkmelin] from comment #17)
why do you need to switch to fixIterator?
It was showing an error that the subject is not iterable. So I have used fixIterator.
That seems like papering over some problem.
Actually, can't you just pass a normal array in there? https://searchfox.org/comm-central/search?q=%22chat-buddy-add%22®exp=true&path=chat
please inline enumerator
We are using it at multiple places in this file. So I have kept it as it is.
Please change it. What you've done now is wrong: you can't reuse an enumerator - it has a certain state, goes to the end, and that's it for it. If you look at the original code you'll see it's re-assigning enumerator to get another one, it's not reusing it.
Reporter | ||
Comment 25•5 years ago
|
||
Reporter | ||
Comment 26•5 years ago
|
||
Reporter | ||
Comment 27•5 years ago
|
||
Assignee | ||
Comment 28•5 years ago
•
|
||
(In reply to :aceman from comment #23)
Thank you very much for your suggestions. I have updated the patch according to the suggestions.
Will this work? The original loop called .nextFile, not .getNext().
Yes, it works.
Assignee | ||
Comment 29•5 years ago
•
|
||
(In reply to Magnus Melin [:mkmelin] from comment #24)
That seems like papering over some problem.
Actually, can't you just pass a normal array in there? https://searchfox.org/comm-central/search?q=%22chat-buddy-add%22®exp=true&path=chat
I have query interfaced subject and it worked.
for (let nick of subject.QueryInterface(Ci.nsISimpleEnumerator)) {
this.insertBuddy(this.createBuddy(nick));
}
Please change it. What you've done now is wrong: you can't reuse an enumerator - it has a certain state, goes to the end, and that's it for it. If you look at the original code you'll see it's re-assigning enumerator to get another one, it's not reusing it.
We need to use hasMoreElements to detect if the window is the last one or not. That's why we need to use the variable for the enumerator to detect hasMoreElements on the same enumerator instance.
Comment 30•5 years ago
|
||
Assignee | ||
Comment 31•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #27)
when you don't need the variable name you can just use
for (let {} of foo)
It is showing a linting error: "Unexpected empty object pattern".
Assignee | ||
Comment 32•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #27)
just inline gInbox.messages instead of declaring the iterator
Here, we want to access hasMoreElements() of the same enumerator instance during the iterations.
Reporter | ||
Comment 33•5 years ago
|
||
(In reply to Khushil Mistry [:khushil324] from comment #31)
It is showing a linting error: "Unexpected empty object pattern".
Right... seems these are just countings, so you could just use let count = [ ... iterable ].length;
instead
Assignee | ||
Comment 34•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 35•5 years ago
|
||
check-in needed for calendar patch.
Assignee | ||
Comment 36•5 years ago
|
||
Assignee | ||
Comment 37•5 years ago
|
||
Comment 38•5 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/74fb2e349d4e
Convert remaining nsISimpleEnumerator users to use JS iteration in /calendar. r=pmorris
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 39•5 years ago
|
||
Reporter | ||
Comment 40•5 years ago
|
||
Assignee | ||
Comment 41•5 years ago
|
||
Assignee | ||
Comment 42•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 43•5 years ago
|
||
Comment 44•5 years ago
|
||
Sorry about the X1 failures in https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0fa2b60cacb5dfed14760009d252834cc18955ff. They are fixed now.
Assignee | ||
Comment 45•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 46•5 years ago
|
||
Check-in needed for both the patches.
Comment 47•5 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/741493b66b6a
Convert remaining nsISimpleEnumerator users to use JS iteration in /mail. r=mkmelin
https://hg.mozilla.org/comm-central/rev/216cbe602979
Convert remaining nsISimpleEnumerator users to use JS iteration in /mailnews. r=mkmelin
Reporter | ||
Comment 48•5 years ago
|
||
Is this bug done now, or is there more to do?
Assignee | ||
Comment 49•5 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #48)
Is this bug done now, or is there more to do?
I will check again in a while and update you here. We are almost done but will still look at it.
Assignee | ||
Comment 50•5 years ago
|
||
A small patch for remaining parts. Linting error fixed.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 51•5 years ago
|
||
Reporter | ||
Comment 52•5 years ago
|
||
Assignee | ||
Comment 53•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 54•5 years ago
|
||
Check-in needed for Bug-1594000_Convert-nsISimpleEnumerator-JS-iteration-remaining-2.patch. This is the last patch on this task.
Reporter | ||
Updated•5 years ago
|
Comment 55•5 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/dfdf71999801
Convert remaining nsISimpleEnumerator users to use JS iteration in remaining parts. r=mkmelin
Comment 56•5 years ago
|
||
https://hg.mozilla.org/comm-central/rev/741493b66b6a
Convert remaining nsISimpleEnumerator users to use JS iteration in /mail. r=mkmelin
Follow-up to fix a syntax error introduced in above commit.
https://hg.mozilla.org/comm-central/rev/741493b66b6a#l45.44
Reporter | ||
Updated•5 years ago
|
Comment 57•5 years ago
|
||
Comment 58•5 years ago
|
||
Reporter | ||
Comment 59•5 years ago
|
||
The fix for cookies was wrong
Assignee | ||
Comment 60•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Comment 61•5 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/b7cf1a257c23
correct iteration conversion of cookies.enumerator. r=khushil
Description
•