Task.jsm should support ES6 generators

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bbenvie, Assigned: bbenvie)

Tracking

({dev-doc-needed})

Trunk
mozilla26
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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

5 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

5 years ago
Created attachment 800346 [details] [diff] [review]
WIP1

Adds support for ES6 generators to Task.jsm and adds some new tests.
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 800347 [details] [diff] [review]
WIP2

Remove a wayward dump.
Attachment #800346 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Created attachment 800350 [details] [diff] [review]
WIP3

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

5 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

5 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

5 years ago
Created attachment 800834 [details] [diff] [review]
WIP4

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

5 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+
https://hg.mozilla.org/mozilla-central/rev/b0d20b693b41
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla26
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.