Closed Bug 763311 Opened 12 years ago Closed 12 years ago

Implement basic "Task.js" interfaces in Toolkit

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: Paolo, Assigned: Paolo)

References

Details

Attachments

(1 file, 2 obsolete files)

The technique implemented by "Task.js" <http://taskjs.org/> makes it extremely
simple to transform synchronous calls made by existing code into asynchronous
calls, preserving natural exception handling even in recursive contexts.

I've developed a proof of concept in attachment 631750 [details] [diff] [review] of bug 740468, for the
bookmarks HTML export functionality.

With specific use cases in mind, we should determine what features and
interfaces of "Task.js" could be more useful to our code, and start porting or
implementing them as appropriate, with a corresponding detailed test suite.

This implementation should definitely use the Promise interfaces of the
"Promise.jsm" module that is being developed in bug 708984.
Generally, I like the idea. Unfortunately, I suspect that this will not be possible whenever any XPCOM is involved, but regardless, I believe this is worth investigating.
(In reply to David Rajchenbach Teller [:Yoric] from comment #1)
> Generally, I like the idea. Unfortunately, I suspect that this will not be
> possible whenever any XPCOM is involved, but regardless, I believe this is
> worth investigating.

Good to hear! Yes, calls into and from XPCOM interfaces would still use completion
callbacks like now, if this is what you mean. See for example _promiseFaviconData
in attachment 631750 [details] [diff] [review], that calls into the getFaviconDataForPage XPCOM function.
Depends on: 774816
Attached patch The patch (obsolete) — Splinter Review
This implements the minimal set of Task.js interfaces required to make the code
in attachment 643097 [details] [diff] [review] of bug 740468 (asynchronous bookmarks export) work.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Attachment #643101 - Flags: review?(dtownsend+bugmail)
Attached patch Simpler usage (obsolete) — Splinter Review
I've renamed the TaskResult constructor to Task.Result, so that the module can
be lazy-loaded at once in a simpler way:

XPCOMUtils.defineLazyModuleGetter(this, "Task",
                                  "resource://gre/modules/Task.jsm");

I've also made it so that both "yield" and "Task.spawn" can forward primitive
values transparently, because I've noticed while working on bug 775489 that it
allows me to avoid special-casing most function calls.  It's akin to the
Promise.resolved() function being able to accept either a promise or primitive.
Attachment #643101 - Attachment is obsolete: true
Attachment #643101 - Flags: review?(dtownsend+bugmail)
Attachment #643836 - Flags: review?(dtownsend+bugmail)
Comment on attachment 643836 [details] [diff] [review]
Simpler usage

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

Mostly looks good but I've made a few comments about hiding the internals from consumers where possible that I'd like to see addressed before I review more.

::: toolkit/content/Task.jsm
@@ +104,5 @@
> + * Represents a promise that is fulfilled when an associated generator function
> + * terminates.
> + *
> + * Use the Task.spawn() function to create and start a new task. Creating a Task
> + * object directly from consumer code is not supported.

Given this, why not instead call the implementation details TaskImpl or something that isn't exposed so consumers can't accidentally make that mistake?

@@ +202,5 @@
> + */
> +Task.spawn = function Task_spawn(aTask) {
> +  if (aTask && typeof(aTask.send) == "function") {
> +    // This is already an iterator resulting from calling a generator function.
> +    return new Task(aTask);

Instead of returning the Task object, why not just (new Tast(aTask))._deferred.promise()?

This should hide the Task object's internals from consumers.

@@ +208,5 @@
> +
> +  if (aTask && typeof(aTask) == "function") {
> +    // Let's call into the generator function ourselves.
> +    return new Task(aTask());
> +  }

It looks like if aTask() returns a value (i.e. isn't a generator function) then this will end up throwing an exception in line 121. In this case I think it'd be better to just return a resolved promise for that value.
Attachment #643836 - Flags: review?(dtownsend+bugmail)
Attached patch Hiding internalsSplinter Review
Thnaks for the review! I've addressed the comments about hiding internals, as
well as accepting a function that is not a generator in "Task.spawn".

(In reply to Dave Townsend (:Mossop) from comment #5)
> It looks like if aTask() returns a value (i.e. isn't a generator function)
> then this will end up throwing an exception in line 121. In this case I
> think it'd be better to just return a resolved promise for that value.

In fact I realized it would throw an exception. At the moment I didn't want to
engineer for more use cases than I needed, but probably this solution is more
robust and way better than a difficult to understand exception.

Now ready for style review (like reassigning function arguments or not)!
Attachment #643836 - Attachment is obsolete: true
Attachment #644871 - Flags: review?(dtownsend+bugmail)
Comment on attachment 644871 [details] [diff] [review]
Hiding internals

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

Sorry, I fail at checking my review queue, don't hesitate to bug me when I take a while to get to something. This looks good to me.
Attachment #644871 - Flags: review?(dtownsend+bugmail) → review+
Depends on: 756542
Blocks: 775580
Landed to inbound with the new Promise module path:

https://hg.mozilla.org/integration/mozilla-inbound/rev/80de6c5ae79f
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/80de6c5ae79f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 899214
You need to log in before you can comment on or make changes to this bug.