913115 - Task.jsm should support ES6 generators

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Brandon Benvie [:benvie]]

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.

Comment 1

[Brandon Benvie [:benvie]]

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".

Comment 2

[Brandon Benvie [:benvie]]

Attachment: WIP1

Adds support for ES6 generators to Task.jsm and adds some new tests.

Comment 3

[Brandon Benvie [:benvie]]

Attachment: WIP2

Remove a wayward dump.

Comment 4

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=10197a855313

Comment 5

[Brandon Benvie [:benvie]]

Attachment: 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.

Comment 6

[:Paolo Amadini]

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.

Comment 7

[Brandon Benvie [:benvie]]

(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.

Comment 8

[Brandon Benvie [:benvie]]

Attachment: 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)

Comment 9

[:Paolo Amadini]

Comment on attachment 800834 [details] [diff] [review]
WIP4

Review of attachment 800834 [details] [diff] [review]:
-----------------------------------------------------------------

Looks excellent, thanks!

Comment 10

[Brandon Benvie [:benvie]]

https://hg.mozilla.org/integration/fx-team/rev/b0d20b693b41

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0d20b693b41

Comment 12

[Marco Castelluccio [:marco]]

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm should be updated.