Closed
Bug 1066432
Opened 10 years ago
Closed 10 years ago
Update ForOfIterator to the changes in Web IDL for sequences as iterables
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(2 files, 1 obsolete file)
10.75 KB,
patch
|
Details | Diff | Splinter Review | |
3.60 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
The relevant change here is that Web IDL now commits to iterating once we Get a non-undefined value for @@iterator, as opposed to requiring a callable.
Assignee | ||
Comment 1•10 years ago
|
||
Unfortunately, none of the existing APIs that are easy to call are overloading sequences with something else, so hard to add a test. But I hacked in an API that tests the behavior locally, and my test passes.
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8488430 -
Flags: review?(jorendorff)
Comment 3•10 years ago
|
||
Comment on attachment 8488430 [details] [diff] [review] Update ForOfIterator to the changes in Web IDL's handling of sequences as iterables. We no commit to an iterable if we get a non-undefined value for the Symbol.iterator property, not just if we get a callable value Review of attachment 8488430 [details] [diff] [review]: ----------------------------------------------------------------- This needs to be tested. Three ideas: 1. jsapi-tests. I think this is the way to go. 2. Maybe you can test this change based on error messages, though I guess probably not in the shell. With the changes, if obj[Symbol.iterator] is a number, for example, you'll get a TypeError like "3 is not a function", right? And without, it'll just fail to match any signature, so you'll get a different message? 3. I seem to recall there are a bunch of test-only DOM bindings (used to unit test Codegen.py and BindingUtils). It might be possible to test using those. r=me with at least one test. ::: js/src/jsapi.h @@ +5141,5 @@ > }; > > /* > + * Initialize the iterator. If AllowNonIterable is passed then if getting > + * the @@iteartor property from iterable returns undefined init() will just s/@@iteartor/@@iterator/ @@ +5143,5 @@ > /* > + * Initialize the iterator. If AllowNonIterable is passed then if getting > + * the @@iteartor property from iterable returns undefined init() will just > + * return true instead of throwing. Callers should then check > + * valueIsIterable() before continuing with the iteration. Pre-existing nit: Since it's required, please change "should" to "must".
Attachment #8488430 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 4•10 years ago
|
||
> And without, it'll just fail to match any signature, so you'll get a different message? No, this only matters for cases when a sequence is overloaded with something, and I couldn't find any such examples so far in our IDL (though we plan to add some). > 3. I seem to recall there are a bunch of test-only DOM bindings Most of these are not actually linked into the binary. I _might_ be able to hook up something in the one that is, though. Doing a jsapi-test here makes sense to me.
Assignee | ||
Comment 5•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8488430 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8497771 -
Flags: review?(jorendorff)
Comment 7•10 years ago
|
||
Comment on attachment 8497771 [details] [diff] [review] JSAPI test, for review purposes Review of attachment 8497771 [details] [diff] [review]: ----------------------------------------------------------------- Sweet tests. ::: js/src/jsapi-tests/testForOfIterator.cpp @@ +11,5 @@ > +{ > + JS::RootedValue v(cx); > + // Hack to make it simple to produce an object that has a property > + // named Symbol.iterator. > + EVAL("var obj = { '@@iterator': 5, [Symbol.iterator]: 5 }; obj;", &v); If you make [Symbol.iterator] an actual method I'll be sure to clean this up in bug 918828. (That will cause the test to fail, which I'm sure to notice.)
Attachment #8497771 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 8•10 years ago
|
||
> If you make [Symbol.iterator] an actual method I'd been trying to make sure I could land in any order wrt bug 918828. In any case, made it Array.prototype[Symbol.iterator]. Same thing for the mochitest in the patch.
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3a49bbc2c5
Flags: in-testsuite+
Target Milestone: --- → mozilla35
Assignee | ||
Comment 10•10 years ago
|
||
And https://hg.mozilla.org/integration/mozilla-inbound/rev/946948bfc6a3 to fix the mochitest bogosity.
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ce3a49bbc2c5 https://hg.mozilla.org/mozilla-central/rev/946948bfc6a3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•