Closed Bug 1123117 Opened 9 years ago Closed 9 years ago

fixIterator should support for-of iteration

Categories

(MailNews Core :: Backend, defect)

Product:
MailNews Core
Component:
Backend
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird38 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 38.0
Tracking Flags:
Tracking Status
thunderbird38 --- fixed

People

(Reporter: jcranmer, Assigned: jcranmer)

References

Details

Attachments

(1 file, 1 obsolete file)

Support for-of in fixIterator
12.51 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
Attached patch for-of-fixIterator (obsolete) — — 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)
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)
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.
Attachment #8550984 - Flags: review?(kent) → review?(mkmelin+mozilla)
> +function toArray(aObj) {
> +  if (Symbol_iterator in aObj) {
> +    return Array.from(aObj);
>    } else if (constructor.contains("NodeList")) {
Where is constructor defined?
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+
But remember to fix the bug Philip points out in comment 3. Looks easy enough.
https://hg.mozilla.org/comm-central/rev/c2fefa9244d1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
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)
Blocks: 1126509
(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)
Comment 7 and comment 8 will be fixed as part of bug 1126509.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: