Closed Bug 1035060 Opened 10 years ago Closed 10 years ago

Implement AbortablePromise

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: xyuan, Assigned: xyuan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

We need this for bug 935883 to implement AbortableProgressPromise.

The AbortablePromise is a cancelable promise, which inherits promise:
[Constructor(PromiseInit init, VoidCallback abortCallback)]
interface AbortablePromise : Promise {
  void abort();
};

The construction of AbortablePromise is like this:

p = new AbortablePromise(function(resolve, reject) { ... },
                         function() { alert("promise aborted") });

So the first argument works the same as a normal promise. It's a function that is synchronously called by the constructor and which provides callbacks that can be used to reject or resolve the promise.
The second argument is a callback that is called when p.abort() is called, unless the promise has already been resolved or rejected. Most likely the promise should automatically be rejected with a DOMException of type AbortError before the callback is called. I.e. p.abort() should first reject the promise and then call the callback.
Is there a spec for this, or is this something we're just making up?

Are there semantics to the abort() method other than rejecting the promise and calling the callback?
No spec for this now and it is something new.

Jonas described how it should like in Bug 935883[1] and may give us full semantics to the abort() method.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=935883#c6
Flags: needinfo?(jonas)
For what it's worth, you can link directly to bug comments like so: bug 935883 comment 6.

Which environments will this new object be exposed in?  Just certified apps, the web, something in between?
Thanks (In reply to Boris Zbarsky [:bz] from comment #3)
> For what it's worth, you can link directly to bug comments like so: bug
> 935883 comment 6.
Really helpful:-)
> 
> Which environments will this new object be exposed in?  Just certified apps,
> the web, something in between?
We want to expose its inheritance AbortableProgressPromise to the web and
AbortablePromise is not required to exposed to web, but may be a nice 
to have feature.
If we want to expose this to the web, there needs to be a standards proposal for it.
For now I think we only need to expose this as part of the DeviceStorage API. So we can make this interface availability conditional on having any devicestorage permission.
Flags: needinfo?(jonas)
Attached patch AbortablePromise.patch (obsolete) — Splinter Review
AbortablePromise extends Promise by adding the "abort" method and the "abortable callback".

When calling promise.abort(), we reject the promise and call its abortable callback.

There are two kinds of abortable callback:
 1) WrapperAbortableCallback for js.
 2) NativeAbortableCallback for C++.

To create AbortablePromise with C++ abortable callback, one need only implement NativeAbortableHandler interface, which will be wrapper by NativeAbortableCallback.

I use prefs to control the visibility of AbortablePromise. By default it will not be exposed to the web.

AbortablePromise will be inherited by AbortableProgressPromise and therefore is not marked as final class.

The try result:
https://tbpl.mozilla.org/?tree=Try&rev=6349ccd83a22
Attached patch AbortablePromise(v1).patch (obsolete) — Splinter Review
Could you help to review this?

Please see Comment 7 about the changes of this patch.

Update patch to fix mochitest failure due to not setting pref value.

The try result:
https://tbpl.mozilla.org/?tree=Try&rev=0f9484ab2ed8
Assignee: nobody → xyuan
Attachment #8454298 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8455091 - Flags: review?(bzbarsky)
Blocks: 1038557
Comment on attachment 8455091 [details] [diff] [review]
AbortablePromise(v1).patch

This needs an intent to implement mail, no?  I don't recall seeing one.

Before we actually turn this on it needs a spec proposal as well, at the very least.  As things stand I don't understand why we need this as opposed to just rejecting promises with some particular kind of rejection and using a catch() on the promise instead of an AbortCallback.  Including this sort of rationale in the intent to implement would be a good idea, by the way.

>+++ b/dom/promise/AbortableCallback.cpp

Why is this needed at all?  It looks like the main upshot here is that it allows us to do a simple mAbortableCallback->Call() in AbortablePromise::Abort, but at the cost of an extra object allocation and having a much more complicated overall model here.  It would be clearer, I think, to use a CallbackObjectHolder<AbortCallback,NativeAbortableHandler> member and just have a simple branch in Abort() based on what's stored in the CallbackObjectHolder.

I'm ignoring the rest of this file and corresponding header, since I think they shouldn't exist.

>+++ b/dom/promise/AbortablePromise.cpp
>+#include "PromiseCallback.h"

Shouldn't need this; more on it below.

>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(AbortablePromise)
>+  NS_INTERFACE_MAP_ENTRY(nsISupports)

Why do you need a separate entry for this in addition to the one you inherit from Promise?

>+AbortablePromise::AbortablePromise(nsIGlobalObject* aGlobal,

Note that this will need merging with bug NNNNNN depending on which lands first.

>+  MOZ_ASSERT(aAbortable);

If you want to somewhat enforce this, take a reference, not a pointer?  That should make it clearer to people that null is not allowed.

>+AbortablePromise::Constructor(const GlobalObject& aGlobal, PromiseInit& aInit,
>+            AbortCallback& aAbortCallback, ErrorResult& aRv)

Fix indent.

>+  JS::Rooted<JSObject*> resolveFunc(cx,

Instead of copy/pasting all this code, can you please factor it out into a protected method on Promise that would be invoked from both here and Promise::Constructor?  Pretty much everything from here to the end of the method seems like it could become:

  return CallInitFunction(aGlobal, aInit, aRv);

or some such.
  
>+AbortablePromise::Abort()
>+  if (mState != Pending || !mAbortableCallback) {

mAbortableCallback would only be null if we got unlinked, right?  Can we actually get this called after Unlink?  I would not think so.

The mState != Pending check is, imo, wrong.  Consider this testcase:

  var p = new AbortablePromise(function(resolve, reject) {
      resolve();
    },
    function aborted() {
      alert("BOGUS");
    });
  p.abort();

I'm pretty sure your implementation will alert "bogus".

Or even simpler:

  var p = new AbortablePromise(function(resolve, reject) {
    },
    function aborted() {
      alert("HELLO");
    });
  p.abort();
  p.abort();

will alert twice afaict.

You want to be checking the mResolvePending boolean here instead.  If it's true, then you have been rejected or resolved already.  We should perhaps rename this boolean to a better name...  Or set mState more eagerly when resolving/rejecting promises; that would help other things as well (like debugger views of promises).  Could you please file a followup on that?

>+  mAbortableCallback->Call();

Do we really want to call the callback synchronously from abort()?  This is the sort of thing that would benefit from a clear spec...

In either case, the API should document this behavior, since I, at least, found it very surprising.

>+++ b/dom/promise/AbortablePromise.h
>+struct JSContext;

#include "js/TypeDecls.h"

>+++ b/dom/promise/NativeAbortableHandler.h

This is a really generic class name that does nothing to indicate this is related to Promises.  Since this is being exported, I think it should be renamed to make it clear what it actually does.

>+++ b/dom/promise/Promise.h
>+  friend class AbortablePromise;

Why is this needed, since it's a subclass?  What private members is it accessing, and can they just be made protected?

>+++ b/dom/promise/tests/test_abortable_promise.html

This should have a test for the "abort on a rejected/resolved promise still calls the callback" issue above.

>+function testPending(succeed, fail) {
>+    succeed();

So this will happen as soon as abort() is called on this promise...

>+  p.then(function() {
>+    ok(false, "Failed to abort pending promise.");

Which means that if the test fails and we fail to reject the promise _this_ will potentially happen after SimpleTest.finish(), since it happens async.

I think the succeed() call in this test should be in the rejection handler, not in the abort handler.  Or better yet it should happen once both the rejection handler and the abort handler have been called, which would not assume anything about ordering.

>+function testResolved(succeed, fail) {
>+    SimpleTest.executeSoon(function() {
>+      succeed();
>+    });

Please document why this executeSoon is here (presumably to give the promise a chance to call the abort handler, though that currently happens sync anyway... and if it didn't what would guarantee that it would happen before executeSoon?).

Also, why not just:

  SimpleTest.executeSoon(succeed);

?

>+    ok(false, "Failed to reslove promise: " + what);

"resolve"

>+function testRejected(succeed, fail) {
>+    SimpleTest.executeSoon(function() {

As above.

>+  Promise.all(tests).then(SimpleTest.finish, SimpleTest.finish);

This is a bit worrisome, since it will run the pieces of the tests in random order, but I guess it's OK here, since the messages from the different tests are different enough.
Attachment #8455091 - Flags: review?(bzbarsky) → feedback+
@bz, thanks.

I didn't know the implementation process of new WebAPI[1] and didn't conform to that.

I sent an intent-to-implement mail[2] to dev.platform mail list.

In the meanwhile, a API proposal[3] was sent to dev.webapi mail list, so that we could get a spec for this.

[1]https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Implement
[2]https://groups.google.com/forum/#!topic/mozilla.dev.platform/L4sNoARavtU
[3]https://groups.google.com/forum/#!topic/mozilla.dev.webapi/N1FjiEqCflI
(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8455091 [details] [diff] [review]
> AbortablePromise(v1).patch
> 
> This needs an intent to implement mail, no?  I don't recall seeing one.
> 
> Before we actually turn this on it needs a spec proposal as well, at the
> very least.  As things stand I don't understand why we need this as opposed
> to just rejecting promises with some particular kind of rejection and using
> a catch() on the promise instead of an AbortCallback.  Including this sort
> of rationale in the intent to implement would be a good idea, by the way.

I do not have an opinion on this specific API yet, but the question of abortable promise has been raised many times in many contexts. An example scenario would be a Promise-based main thread API that defers some real-time computation to a Worker thread. The client of the API may decide to cancel the computation by calling `abort()`. The main difference with rejecting a Promise is that this `abort` comes from the client of the Promise, rather than from its definition.

There are alternatives. For instance, one could extend `Promise` as follows:

new AbortablePromise2((resolve, reject, self) => {
  // `self` is a Promise with the same behavior as the result of the entire `new Promise(...)` expression
  // the implementation may now register itself as a client of `self`, which fulfills the
  // same role as the second argument in `AbortablePromise` as introduced in comment 0
});

`AbortablePromise2` also needs a mechanism to abort, either with the same `abort()` as `AbortablePromise` or by letting the promise implementation itself expose some access to `reject`.

Also, something very close to either `AbortablePromise` or `AbortablePromise2` may already be encoded in web code with the available `Promise` API.
> The main difference with rejecting a Promise 

Ah, so the key is that the entity doing the aborting is not the entity that created the promise.

OK, in that case having a separate abort() method definitely makes sense.  It's not clear to me whether having the separate abort callback does, but maybe the fact that it runs more eagerly than a catch() on the promise would is relevant?
(In reply to Boris Zbarsky [:bz] from comment #12)
> > The main difference with rejecting a Promise 
> 
> Ah, so the key is that the entity doing the aborting is not the entity that
> created the promise.
Yes, I think so.
> 
> OK, in that case having a separate abort() method definitely makes sense. 
> It's not clear to me whether having the separate abort callback does, but
> maybe the fact that it runs more eagerly than a catch() on the promise would
> is relevant?
I tend to keep a separate abort callback, thought we could do the cancellation
work inside the catch().

The "abort" comes from the client of the promise, but the actual abort operation 
should be done by the entity that created the promise. As the "catch()" and "then()" 
are methods for promise client, it's not a good place to place the code for the promise 
creator.

In addition, "abort callback" is required for a properly instantiated AbortablePromise.
The constructor way could enforce this requirement, but the "catch()" way can not.

The "catch()" will also create and return an unused promise.
I am a bit nervous that adding a second callback will not be compatible with future evolutions of DOM Promise. Is there a way to make something somehow moz-prefixed?
Such moz-prefixed thing shouldn't at least exposed to the web.
> As the "catch()" and "then()" are methods for promise client

That's an interesting point.  While then() with only one argument is somewhat idempotent in that it's transparent whether it's called or not, catch() is not: it affects error reporting.  That's annoying.

I, like David, and am seriously worried about how this will interact with future evolution of ES Promises.  People keep talking about adding lots of Promise features in ES7, once they've shipped basic Promises in ES6.  This is why it also worries me that we haven't made any public spec proposals for this.  Those might give us useful feedback on what might or might not collide with future Promise development work.

At the very least, it's worth running this by Domenic Denicola to see what feedback he has on the API, since he has the best idea of where ES Promises are headed.
Yes, there's definitely concern that this won't be compatible with whatever the equivalent ES "cancellable Promise" API will look like. Which is why we're not planning on exposing this to the web in general, but rather just to pages that have access to DeviceStorage (which is another Firefox-only API).
Well, my concern extends to the proposed upgrade path for pages that have access to DeviceStorage. If we don't use any kind of moz-prefix, I am afraid that both the webdevs and us are going to be in an awkward situation.

I agree with bz that we should provide public specs and run this through Domenic Denicola.

Also, regarding moz-only features, here's a variant that should not conflict with ES7 next-gen Promise, whatever they may end up being.

var p = new mozAbortablePromise((resolve, reject) => {
  reject.addEventListener("mozAbort", (reason) => {
    // This stuff is triggered when `p.mozAbort` is called
  });
  // Usual stuff
});
p.mozAbort(reason);
I'm not too concerned about the constructor. We can even hide that. The main use case here is for the DeviceStorage API to return Promises that have a .abort()/.cancel() function.
Ok, if we hide the constructor, there are less chances of collision. What does `then` return? A regular `Promise` or an `AbortablePromise`?
What I suggest we do is to call the interface MozAbortablePromise, and only expose it to pages that have DeviceStorage access.

I know that we shouldn't do prefixes, but in FirefoxOS we need to evolve the platform faster than specs are evolving right now. Using the prefix will allow us to switch to the standard API as soon as it specced. In the meantime having the API exposed to some content allows us to test the API and get developer feedback which can feed into the spec process.

And by only exposing MozAbortablePromise to pages that have DeviceStorage access, it means that it's not exposed to the general web.
(In reply to David Rajchenbach Teller [:Yoric] (PTO + various activities, hard to reach until August 20th - please use "needinfo") from comment #20)
> Ok, if we hide the constructor, there are less chances of collision. What
> does `then` return? A regular `Promise` or an `AbortablePromise`?

We might let `then` return regular `Promise` with our first implementation. 
It needs much design work for `then` to return an `AbortablePromise`, as discussed in[1].

[1] https://github.com/promises-aplus/cancellation-spec/issues/6
Depends on: 1055925
Attached patch AbortablePromise(v2).patch (obsolete) — Splinter Review
(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8455091 [details] [diff] [review]
...
> >+++ b/dom/promise/AbortableCallback.cpp
> 
> Why is this needed at all?  It looks like the main upshot here is that it
> allows us to do a simple mAbortableCallback->Call() in
> AbortablePromise::Abort, but at the cost of an extra object allocation and
> having a much more complicated overall model here.  It would be clearer, I
> think, to use a CallbackObjectHolder<AbortCallback,NativeAbortableHandler>
> member and just have a simple branch in Abort() based on what's stored in
> the CallbackObjectHolder.
> 
> I'm ignoring the rest of this file and corresponding header, since I think
> they shouldn't exist.
> 
The AbortableCallback is removed. Instead I add two member variable mJSAbortCallback
and mNativeAbortCallback to hold the js and native callback handlers respectively.
The reason why I don't use a CallbackObjectHolder member is that CallbackObjectHolder
prevents us utilizing cycle collection macro(NS_IMPL_CYCLE_COLLECTION_INHERITED)
and makes cycle collection implementation complicated.
> >+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(AbortablePromise)
> >+  NS_INTERFACE_MAP_ENTRY(nsISupports)
> 
> Why do you need a separate entry for this in addition to the one you inherit
> from Promise?
Don't need this and removed.
> 
> >+AbortablePromise::AbortablePromise(nsIGlobalObject* aGlobal,
> 
> Note that this will need merging with bug NNNNNN depending on which lands
> first.
> 
> >+  MOZ_ASSERT(aAbortable);
> 
> If you want to somewhat enforce this, take a reference, not a pointer?  That
> should make it clearer to people that null is not allowed.
The pointer parameter is changed to a reference.
> 
> >+AbortablePromise::Constructor(const GlobalObject& aGlobal, PromiseInit& aInit,
> >+            AbortCallback& aAbortCallback, ErrorResult& aRv)
> 
> Fix indent.
> 
> >+  JS::Rooted<JSObject*> resolveFunc(cx,
> 
> Instead of copy/pasting all this code, can you please factor it out into a
> protected method on Promise that would be invoked from both here and
> Promise::Constructor?  Pretty much everything from here to the end of the
> method seems like it could become:
> 
>   return CallInitFunction(aGlobal, aInit, aRv);
> 
> or some such.
Fixed.
>   
> >+AbortablePromise::Abort()
> >+  if (mState != Pending || !mAbortableCallback) {
> 
> mAbortableCallback would only be null if we got unlinked, right?  Can we
> actually get this called after Unlink?  I would not think so.
Fixed. Remove the extra non-null check of mAbortableCallback.
> 
> The mState != Pending check is, imo, wrong.  Consider this testcase:
> 
>   var p = new AbortablePromise(function(resolve, reject) {
>       resolve();
>     },
>     function aborted() {
>       alert("BOGUS");
>     });
>   p.abort();
> 
> I'm pretty sure your implementation will alert "bogus".
> 
> Or even simpler:
> 
>   var p = new AbortablePromise(function(resolve, reject) {
>     },
>     function aborted() {
>       alert("HELLO");
>     });
>   p.abort();
>   p.abort();
> 
> will alert twice afaict.
> 
> You want to be checking the mResolvePending boolean here instead.  If it's
> true, then you have been rejected or resolved already.  We should perhaps
> rename this boolean to a better name...  Or set mState more eagerly when
> resolving/rejecting promises; that would help other things as well (like
> debugger views of promises).  Could you please file a followup on that?
Fixed. promise.IsPending() is added to check the mResolvePending boolean here.
> 
> >+  mAbortableCallback->Call();
> 
> Do we really want to call the callback synchronously from abort()?  This is
> the sort of thing that would benefit from a clear spec...
Fixed and make asynchronous calling.
> 
> In either case, the API should document this behavior, since I, at least,
> found it very surprising.
> 
> >+++ b/dom/promise/AbortablePromise.h
> >+struct JSContext;
> 
> #include "js/TypeDecls.h"
Fixed.
> 
> >+++ b/dom/promise/NativeAbortableHandler.h
> 
> This is a really generic class name that does nothing to indicate this is
> related to Promises.  Since this is being exported, I think it should be
> renamed to make it clear what it actually does.
It is renamed to PromiseNativeAbortCallback.
> 
> >+++ b/dom/promise/Promise.h
> >+  friend class AbortablePromise;
> 
> Why is this needed, since it's a subclass?  What private members is it
> accessing, and can they just be made protected?
Remove the friend class declaration and expose the private members accessed by 
the subclass as protected.
> 
> >+++ b/dom/promise/tests/test_abortable_promise.html
> 
> This should have a test for the "abort on a rejected/resolved promise still
> calls the callback" issue above.
The tests of "testResolved" and "testRejected" cover this case.
> 
> >+function testPending(succeed, fail) {
> >+    succeed();
> 
> So this will happen as soon as abort() is called on this promise...
> 
> >+  p.then(function() {
> >+    ok(false, "Failed to abort pending promise.");
> 
> Which means that if the test fails and we fail to reject the promise _this_
> will potentially happen after SimpleTest.finish(), since it happens async.
> 
> I think the succeed() call in this test should be in the rejection handler,
> not in the abort handler.  Or better yet it should happen once both the
> rejection handler and the abort handler have been called, which would not
> assume anything about ordering.
Fixed and calling succeed() once both the rejection handler and the abort handler
have been called.
> 
> >+function testResolved(succeed, fail) {
> >+    SimpleTest.executeSoon(function() {
> >+      succeed();
> >+    });
> 
> Please document why this executeSoon is here (presumably to give the promise
> a chance to call the abort handler, though that currently happens sync
> anyway... and if it didn't what would guarantee that it would happen before
> executeSoon?).
Fixed by documenting and setting larger timeout.
> 
> Also, why not just:
> 
>   SimpleTest.executeSoon(succeed);
> 
> ?
> 
> >+    ok(false, "Failed to reslove promise: " + what);
> 
> "resolve"
> 
> >+function testRejected(succeed, fail) {
> >+    SimpleTest.executeSoon(function() {
> 
> As above.
> 
> >+  Promise.all(tests).then(SimpleTest.finish, SimpleTest.finish);
> 
> This is a bit worrisome, since it will run the pieces of the tests in random
> order, but I guess it's OK here, since the messages from the different tests
> are different enough.
The running order is changed to be sequential.
Attachment #8455091 - Attachment is obsolete: true
Attachment #8480341 - Flags: review?(bzbarsky)
Comment on attachment 8480341 [details] [diff] [review]
AbortablePromise(v2).patch

For future reference, an interdiff here would have made the review happen much faster.  I'd appreciate one for the next update to the patch.

> The reason why I don't use a CallbackObjectHolder member is that
> CallbackObjectHolder prevents us utilizing cycle collection
> macro(NS_IMPL_CYCLE_COLLECTION_INHERITED)

Why, exactly?  It should work fine afaict.

Your code doesn't traverse/unlink mNativeAbortCallback, which is broken.

> Note that this will need merging with bug NNNNNN depending on which lands

I meant bug 1040263, sorry.  I wrote that comment when I was offline and couldn't look up the bug number, then forgot to fill it in.

The merging looks more or less fine, but CreateWrapper shouldn't need the aGlobal argument.  It's an instance method on Promise, so can use mGlobal.

>+++ b/dom/promise/AbortablePromise.h
>+  // Constructor used to create native AbortablePromise with C++.

This needs to be a protected constructor, because we need to make sure that AbortablePromise instances created from C++ still do the CreateWrapper thing.  So you'll want a factory function for creating from C++.

>+  class AbortCallbackTask MOZ_FINAL : public nsRunnable

Why did NS_NewRunnableMethod not do what you wanted?

>+++ b/dom/promise/Promise.h
>+  virtual
>+  ~Promise();

I think this would look less weird on one line.

>+  CreateWrapper(nsIGlobalObject* aGlobal, ErrorResult& aRv);
>+  CallInitFunction(const GlobalObject& aGlobal, PromiseInit& aInit,

Please document these.

>+++ b/dom/promise/PromiseNativeAbortCallback.h
>+ * PromiseNativeAbortCallback allows C++ to react to a AbortablePromise being

s/to a/to an/

>+  virtual void
>+  Call() = 0;

Again, one line.

>+++ b/dom/promise/tests/test_abortable_promise.html
>+   new Promise(gTests.shift()).then(function() {
>+    runTest();

Why not just pass "runTest" itself as the first argument to then()?

>+    ok(true, "Promised resolved.");

s/Promised/Promise/

>+++ b/dom/webidl/AbortablePromise.webidl

What happened to only exposing this to pages with the DeviceStorage permission?  I thought that was the plan.
Attachment #8480341 - Flags: review?(bzbarsky) → review-
Flags: needinfo?(xyuan)
Attached patch AbortablePromise(v3).patch (obsolete) — Splinter Review
See the interdiff below about the changes.
Attachment #8480341 - Attachment is obsolete: true
Attachment #8487107 - Flags: review?(bzbarsky)
Flags: needinfo?(xyuan)
Attached patch interdiff(v2-v3).diff (obsolete) — Splinter Review
(In reply to Boris Zbarsky [:bz] from comment #25)
> Comment on attachment 8480341 [details] [diff] [review]
> AbortablePromise(v2).patch
> 
> For future reference, an interdiff here would have made the review happen
> much faster.  I'd appreciate one for the next update to the patch.
> 
> > The reason why I don't use a CallbackObjectHolder member is that
> > CallbackObjectHolder prevents us utilizing cycle collection
> > macro(NS_IMPL_CYCLE_COLLECTION_INHERITED)
> 
> Why, exactly?  It should work fine afaict.
> 
> Your code doesn't traverse/unlink mNativeAbortCallback, which is broken.
CallbackObjectHolder works and fixed. 
> 
> > Note that this will need merging with bug NNNNNN depending on which lands
> 
> I meant bug 1040263, sorry.  I wrote that comment when I was offline and
> couldn't look up the bug number, then forgot to fill it in.
> 
> The merging looks more or less fine, but CreateWrapper shouldn't need the
> aGlobal argument.  It's an instance method on Promise, so can use mGlobal.
aGlobal argement of Promise::CreateWrapper is removed.
> 
> >+++ b/dom/promise/AbortablePromise.h
> >+  // Constructor used to create native AbortablePromise with C++.
> 
> This needs to be a protected constructor, because we need to make sure that
> AbortablePromise instances created from C++ still do the CreateWrapper
> thing.  So you'll want a factory function for creating from C++.
// Add a factory function Create for AbortablePromise and make this ctor protected.
> 
> >+  class AbortCallbackTask MOZ_FINAL : public nsRunnable
> 
> Why did NS_NewRunnableMethod not do what you wanted?
It is much better than what I want, but I didn't know it.
This patch uses NS_NewRunnableMethod.
> 
> >+++ b/dom/promise/Promise.h
> >+  virtual
> >+  ~Promise();
> 
> I think this would look less weird on one line.
Fixed.
A question about code style:
In C++ headers, does it prefer to place the function name the same line with the virtual, static and return type?
> 
> >+  CreateWrapper(nsIGlobalObject* aGlobal, ErrorResult& aRv);
> >+  CallInitFunction(const GlobalObject& aGlobal, PromiseInit& aInit,
> 
> Please document these.
Fixed.
> 
> >+++ b/dom/promise/PromiseNativeAbortCallback.h
> >+ * PromiseNativeAbortCallback allows C++ to react to a AbortablePromise being
> 
> s/to a/to an/
Fixed.
> 
> >+  virtual void
> >+  Call() = 0;
> 
> Again, one line.
Fixed.
> 
> >+++ b/dom/promise/tests/test_abortable_promise.html
> >+   new Promise(gTests.shift()).then(function() {
> >+    runTest();
> 
> Why not just pass "runTest" itself as the first argument to then()?
Fixed.
> 
> >+    ok(true, "Promised resolved.");
> 
> s/Promised/Promise/
Fixed.
> 
> >+++ b/dom/webidl/AbortablePromise.webidl
> 
> What happened to only exposing this to pages with the DeviceStorage
> permission?  I thought that was the plan.
We don't use AbortablePromise directly with DeviceStorage, but use its child class AbortableProgressPromise. So we will expose AbortableProgressPromise with DeviceStorage permssion instead and hide AbortablePromise from web.
Comment on attachment 8487107 [details] [diff] [review]
AbortablePromise(v3).patch

> In C++ headers, does it prefer to place the function name the same line with
> the virtual, static and return type?

Most DOM code puts them on the same line.

>+  Promise::DispatchToMainOrWorkerThread(
>+    NS_NewRunnableMethod(this, &AbortablePromise::DoAbort));

Please store the return value of NS_NewRunnableMethod in a local nsCOMPtr<nsIRunnable> so we're not handing around 0-refcount objects.

>+  CallbackObjectHolder<AbortCallback, PromiseNativeAbortCallback>
>+  mAbortCallback;

One line here please, unless it hits 80 chars.  In that case, I'd probably put the linebreak between the two template parameters like so:

  CallbackObjectHolder<AbortCallback, 
                       PromiseNativeAbortCallback> mAbortCallback;

> and hide AbortablePromise from web

But this patch isn't doing that.  It should, no?

r=me with those bits fixed.  Thank you for the interdiff; it made things much simpler!
Attachment #8487107 - Flags: review?(bzbarsky) → review+
Thank you for the patient review.
(In reply to Boris Zbarsky [:bz] from comment #28)
...
> > and hide AbortablePromise from web
> 
> But this patch isn't doing that.  It should, no?
It did that.
AbortablePromise is disabled by the pref of "dom.abortablepromise.enabled", which is
declared in the WebIDL and set to false in modules/libpref/init/all.js.
Is is sufficient to hide the interface?
Flags: needinfo?(bzbarsky)
It's sufficient to hide it, but I thought you were planning to expose it to apps with the DeviceStorage permission.  The patch is not doing that.

Do we expect any configurations to set that pref to true?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #30)
> It's sufficient to hide it, but I thought you were planning to expose it to
> apps with the DeviceStorage permission.  The patch is not doing that.
> 
> Do we expect any configurations to set that pref to true?
For DeviceStorage, the answer is no.
Fix nits of comment 28.

try:
https://tbpl.mozilla.org/?tree=Try&rev=5d1a73ad1104
https://treeherder.mozilla.org/ui/#/jobs?repo=try&

The interdiff:
diff --git a/dom/promise/AbortablePromise.cpp b/dom/promise/AbortablePromise.cpp
index 36c25ec..a0cd814 100644
--- a/dom/promise/AbortablePromise.cpp
+++ b/dom/promise/AbortablePromise.cpp
@@ -98,8 +98,9 @@ AbortablePromise::Abort()
   }
   MaybeReject(NS_ERROR_ABORT);
 
-  Promise::DispatchToMainOrWorkerThread(
-    NS_NewRunnableMethod(this, &AbortablePromise::DoAbort));
+  nsCOMPtr<nsIRunnable> runnable =
+    NS_NewRunnableMethod(this, &AbortablePromise::DoAbort);
+  Promise::DispatchToMainOrWorkerThread(runnable);
 }
 
 void
diff --git a/dom/promise/AbortablePromise.h b/dom/promise/AbortablePromise.h
index a3f920c..6a90747 100644
--- a/dom/promise/AbortablePromise.h
+++ b/dom/promise/AbortablePromise.h
@@ -56,8 +56,8 @@ private:
   void DoAbort();
 
   // The callback functions to abort the promise.
-  CallbackObjectHolder<AbortCallback, PromiseNativeAbortCallback>
-  mAbortCallback;
+  CallbackObjectHolder<AbortCallback,
+                       PromiseNativeAbortCallback> mAbortCallback;
 };
 
 } // namespace dom
Attachment #8487107 - Attachment is obsolete: true
Attachment #8487124 - Attachment is obsolete: true
Attachment #8488405 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/25e20dd15ec5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
No docs for this, see bug 1242054
Keywords: dev-doc-needed
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.