Closed
Bug 913115
Opened 11 years ago
Closed 11 years ago
Task.jsm should support ES6 generators
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: bbenvie, Assigned: bbenvie)
Details
(Keywords: dev-doc-needed)
Attachments
(1 file, 3 obsolete files)
13.17 KB,
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
Now that ES6 generators have landed, Task.jsm should support them. This means supporting the boxed return values, losing the StopIteration checking, and supporting return values. I think it'll be too awkward to support both legacy generators and ES6 generators, so we should either just put it in a new file or add new exported functions that are specifically for ES6 generators.
Assignee | ||
Comment 1•11 years ago
|
||
Actually I take it back, it might not be too awkward to support both using the same function. Also note, another difference is the removal of the "send" method on generators in favor of simply passing a value to "next".
Assignee | ||
Comment 2•11 years ago
|
||
Adds support for ES6 generators to Task.jsm and adds some new tests.
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•11 years ago
|
||
Remove a wayward dump.
Attachment #800346 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=10197a855313
Assignee | ||
Comment 5•11 years ago
|
||
Comment update. Paolo, I hope you don't mind if I r? you on this. Seeing as you authored most of the file I figured you'd be the best person to do it. Let me know if you need me to get someone else.
Attachment #800347 -
Attachment is obsolete: true
Attachment #800350 -
Flags: review?(paolo.mozmail)
Comment 6•11 years ago
|
||
Comment on attachment 800350 [details] [diff] [review] WIP3 Review of attachment 800350 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Brandon Benvie [:bbenvie] from comment #0) > Now that ES6 generators have landed, Task.jsm should support them. That's great news! :-) The patch looks good, I've just a few questions and comments below. ::: toolkit/modules/Task.jsm @@ +103,5 @@ > + */ > +function isGenerator(aValue) { > + if (aValue) { > + return typeof(aValue.send) == "function" || > + Object.prototype.toString.call(aValue) == "[object Generator]"; I get that legacy generators don't result in a "[object Generator]"? @@ +178,5 @@ > */ > function TaskImpl(iterator) { > this.deferred = Promise.defer(); > this._iterator = iterator; > + this._isStarGenerator = !iterator.send; I've still not clearly understood in which cases we get a reference strict warning, does this trigger one? In any case the "in" operator may be clearer. @@ +243,5 @@ > + this.deferred.resolve(); > + } catch (ex) { > + // The generator function failed with an uncaught exception. > + this.deferred.reject(ex); > + } Please rebase the patch on top of bug 908955, and share the outer exception handler (I agree that Task.Result and StopIteration should not be supported in ES6 generators and can be kept in the "else" branch). @@ +253,5 @@ > + * > + * @param aResult > + * The yielded value to handle. > + */ > + _handleResult: function TaskImpl_handleResult(aResult) { nit: _handleResultValue(aValue), as the "result" is different object.
Attachment #800350 -
Flags: review?(paolo.mozmail) → feedback+
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to :Paolo Amadini from comment #6) > I get that legacy generators don't result in a "[object Generator]"? Actually, it seems they do! That will simplify isGenerator. > I've still not clearly understood in which cases we get a reference strict > warning, does this trigger one? In any case the "in" operator may be clearer. Neither do, but you're right that "in" works here and is clearer anyway.
Assignee | ||
Comment 8•11 years ago
|
||
Thanks for the feedback! Addresses feedback and rebases to bug 908955: * simplify `isGenerator` * change how `_isStarGenerator` is detected to use "in" * change `_handleValue` to `_handleResultValue` * factor out uncaught exception handling into `_handleException` method * remove a redundant test I had added that tested return values (other tests do this)
Attachment #800350 -
Attachment is obsolete: true
Attachment #800834 -
Flags: review?(paolo.mozmail)
Comment 9•11 years ago
|
||
Comment on attachment 800834 [details] [diff] [review] WIP4 Review of attachment 800834 [details] [diff] [review]: ----------------------------------------------------------------- Looks excellent, thanks!
Attachment #800834 -
Flags: review?(paolo.mozmail) → review+
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/b0d20b693b41
Whiteboard: [fixed-in-fx-team]
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b0d20b693b41
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla26
Comment 12•10 years ago
|
||
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm should be updated.
Keywords: dev-doc-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•