Closed
Bug 1123117
Opened 10 years ago
Closed 10 years ago
fixIterator should support for-of iteration
Categories
(MailNews Core :: Backend, defect)
Tracking
(thunderbird38 fixed)
RESOLVED
FIXED
Thunderbird 38.0
| Tracking | Status | |
|---|---|---|
| thunderbird38 | --- | fixed |
People
(Reporter: jcranmer, Assigned: jcranmer)
References
Details
Attachments
(1 file, 1 obsolete file)
|
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
__iterator__ is deprecated and will be removed before long (see bug 1098412). Our single biggest use of it is in fixIterator.
The solution is to support both the old iterator protocol and the new iterator protocol on the same objects, to prevent everyone breaking. I'm putting this as tracking-38, since it would be really nice for extensions if this could go out on ESR, as I don't think __iterator__ will last through the next ESR release.
Try push: <https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=835a6b82f0ff>.
Attachment #8550954 -
Flags: review?(kent)
|
Assignee | |
Comment 1•10 years ago
|
||
Oops, forgot that I hadn't gotten around to fixing that mozmill test failure.
Attachment #8550954 -
Attachment is obsolete: true
Attachment #8550954 -
Flags: review?(kent)
Attachment #8550984 -
Flags: review?(kent)
|
||
Comment 2•10 years ago
|
||
I looked at this a bit, but I have never been a user of fixIterator, and all of this Symbol.iterator and @@iterator stuff is quite new to me. I also see that Magnus has already been involved in one other similar patch. I suggest he would be a better reviewer. If for some reason you don't agree, I could give this another look.
|
|
Assignee | |
Updated•10 years ago
|
Attachment #8550984 -
Flags: review?(kent) → review?(mkmelin+mozilla)
|
||
Comment 3•10 years ago
|
||
> +function toArray(aObj) {
> + if (Symbol_iterator in aObj) {
> + return Array.from(aObj);
> } else if (constructor.contains("NodeList")) {
Where is constructor defined?
|
||
Comment 4•10 years ago
|
||
Comment on attachment 8550984 [details] [diff] [review]
Support for-of in fixIterator
Review of attachment 8550984 [details] [diff] [review]:
-----------------------------------------------------------------
Can't claim I have a deep knowledge about this, but looks ok I think.
::: mail/test/mozmill/utils/html/collections.html
@@ +2,5 @@
> <head>
> <title>Collections</title>
> <script type="application/javascript;version=1.7">
> + var Symbol_iterator = typeof Symbol === "function" && Symbol.iterator ?
> + Symbol.iterator : "@@iterator";
I think the, eh, syntax used elsewhere is clearer
const JS_HAS_SYMBOLS = typeof Symbol === "function";
const ITERATOR_SYMBOL = JS_HAS_SYMBOLS ? Symbol.iterator : "@@iterator";
(here and elsewhere)
::: mailnews/base/util/iteratorUtils.jsm
@@ +71,5 @@
> + // something we want to get rid of anyways.
> + // Note that the new-style iterator uses Symbol.iterator to work, and anything
> + // that has Symbol.iterator works with for-of.
> + function makeDualIterator(newStyle) {
> + newStyle.__iterator__ = function () {
nit: no space before ()
@@ +88,5 @@
> else
> + return makeDualIterator((function*() {
> + for (let o of aEnum)
> + yield o.QueryInterface(aIface);
> + })());
this if-else could use braces!
Attachment #8550984 -
Flags: review?(mkmelin+mozilla) → review+
|
|
||
Comment 5•10 years ago
|
||
But remember to fix the bug Philip points out in comment 3. Looks easy enough.
|
|
Assignee | |
Comment 6•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
|
|
||
Comment 7•10 years ago
|
||
I believe it needs to be constructor.toString().contains at http://mxr.mozilla.org/comm-central/source/mailnews/base/util/iteratorUtils.jsm#32
Flags: needinfo?(Pidgeot18)
|
|
Assignee | |
Comment 8•10 years ago
|
||
(In reply to Magnus Melin from comment #7)
> I believe it needs to be constructor.toString().contains at
> http://mxr.mozilla.org/comm-central/source/mailnews/base/util/iteratorUtils.
> jsm#32
constructor.name, yes. Although the fact that it's been so long without an error report suggests that iteratorUtils isn't used with NodeLists anymore?
Flags: needinfo?(Pidgeot18)
|
|
||
Updated•10 years ago
|
status-thunderbird38:
--- → fixed
|
|
Assignee | |
Updated•10 years ago
|
Flags: in-testsuite+
|
|
||
Updated•10 years ago
|
tracking-thunderbird38:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•