Closed Bug 856410 (promises) Opened 11 years ago Closed 11 years ago

Implement promises

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: annevk, Assigned: baku)

References

(Depends on 2 open bugs, Blocks 3 open bugs, )

Details

(Keywords: dev-doc-complete, Whiteboard: [Async])

Attachments

(7 files, 55 obsolete files)

2.77 KB, patch
Details | Diff | Splinter Review
9.05 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
23.75 KB, patch
Details | Diff | Splinter Review
27.12 KB, patch
Details | Diff | Splinter Review
5.45 KB, patch
Details | Diff | Splinter Review
2.16 KB, patch
Details | Diff | Splinter Review
6.53 KB, patch
Details | Diff | Splinter Review
We should implement futures so we can start using them in new APIs.

When implementing we should probably also look at how we can improve DOMRequest to provide an easier migration path for developers.
Would be good to have some API which is using Futures and then implement Futures and that API.
Well, e.g. navigator.mozPay() ought to be using it. navigator.push() too. Probably a bunch of our other experimental APIs can start migrating towards it the sooner we implement this.
Attached patch WIP (obsolete) — Splinter Review
Comment on attachment 735742 [details] [diff] [review]
WIP

It may be helpful to ensure your future implementation passes

https://github.com/promises-aplus/promises-tests

and in particular the probably-done-but-not-yet-merged-to-master

https://github.com/promises-aplus/promises-tests/tree/spec-1.1

The intent of DOMFutures is to follow the Promises/A+ specification, i.e. any DOMFuture implementation should pass these tests.

Note that these tests cover the `then` method of a promise; other tests would be necessary to cover promise creation.
Assignee: nobody → amarchesini
I'm still not 100% sure about the raw pointer in the FutureResolver. The FutureResolver is 1:1 with the Future, and the Future keeps a reference of the resolver. Should this be enough?
Attachment #735742 - Attachment is obsolete: true
Attachment #740949 - Flags: review?(mounir)
Attachment #740949 - Flags: review?(bzbarsky)
I need to read through the spec carefully here, but the exception handling in the constructor doesn't make sense to me.  Why are we reporting it on the cx, exactly?  I thought the idea was basically to do something like this:

  try {
    // some stuff
  } catch (e) {
    reject(e);
  }

whereas what the code does will ... probably fatally assert when we try to run script on an JSContext which already has a pending exception.
Attached patch part 2 - then + catch (obsolete) — Splinter Review
This second patch implements then() and catch() but in a different way then the specs.

In the specs there is these complex FutureCallbackWrapper and FutureCallback for having nested future objects. In this implementation what I do is:

1. Then receives resolveCallback and rejectCallback
2. resolveCallback is appended to the list of resolve Callbacks
3. rejectCallback is appended to the list of reject Callbacks
3. return the same future object.

4. Catch receives a rejectCallback
5. it calls Then(null, rejectcallback)

With this I can do:

  var future = new Future(function(resolver) {
    resolver.resolve(42);
  });

  var resolve = function(what) { /* .. something.. */ };
  var reject = function(what) { /* .. something.. */ };

  future.then(resolve, reject);
  future.then(resolve, reject).then(resolve, reject).catch(reject);
  future.done(resolve.reject);

Probably I miss something, but I think with this approach we have the same results of what the specs say. The only difference is that here we can do:

  var future = new Future(...);
  var future2 = future.then(..);
  future === future2

In the specs future and future2 are not the same object. But I don't see why this shouldn't be. Thanks for feedback!
Attachment #740965 - Flags: feedback?(annevk)
Andrea,

Were you aware that the future returned by `future.then` has a new fulfillment value or rejection reason, depending on what happens inside the `acceptCallback` and `rejectCallback`? It looks like what you have implemented here, if I am understanding correctly, is just an event aggregator. For reasons why promises (aka futures) are not just event aggregators, please see http://domenic.me/2012/10/14/youre-missing-the-point-of-promises/.
You may find the following implementations helpful. Their `then` methods should have the same semantics as those of the DOM Futures spec, since both follow Promises/A+ in the behavior of `then`. They are some of the simpler Promises/A+ implementations out there.

https://github.com/novemberborn/legendary/blob/master/lib/Promise.js
https://github.com/then/promise/blob/master/index.js
https://github.com/briancavalier/avow/blob/master/avow.js

Others are available at https://github.com/promises-aplus/promises-spec/blob/master/implementations.md
Hi Domenic,
Thanks for your comments.
I appreciate your blogpost and the link you sent here. But reading the specs:
http://dom.spec.whatwg.org/#futures

I see something I don't get.

"The done(resolveCallback, rejectCallback) method must append resolveCallback and rejectCallback to the context object."

It means I can do:

var future = new Future(...)
future.done(function A(){}, function B() {});
future.done(function C(){}, function D() {});

This appends functions A and C in the resolveCallbacks list, and B and C in the reject list.
Here we have what you define as "event aggregators". but this doesn't match with then().
Then() returns a new Future object and the "parent" future is connected to the new one through the callbacks. I understand how the future objects can work "on cascade", but this doesn't match with the done() method. Can it be that we have a bug in the specs?
then() has to return a new future whose result will depend on the return value of the callbacks it is passed. There's no bug in the specification here.
Attachment #741174 - Flags: review?(bzbarsky)
Attachment #740949 - Attachment is obsolete: true
Attachment #740949 - Flags: review?(mounir)
Attachment #740949 - Flags: review?(bzbarsky)
Attachment #741181 - Flags: review?(mounir)
Attachment #741181 - Flags: review?(bzbarsky)
Attached patch part 2 - then + catch (obsolete) — Splinter Review
Thanks for helping me to understand the then().
This patch should implement /then/ and /catch/ properly.
Mochitests are included in the patch, so maybe someone can give me a feedback about if the behaviour is what's expected to be.

Open issues:
. the 'then' property in the resolve(obj)
. any/every/some
. a couple of raw pointers
. workers
Attachment #740965 - Attachment is obsolete: true
Attachment #740965 - Flags: feedback?(annevk)
Attachment #741182 - Flags: review?(mounir)
Attachment #741182 - Flags: review?(bzbarsky)
Attachment #741274 - Flags: review?(mounir)
Attachment #741274 - Flags: review?(bzbarsky)
Attached patch part 2 - then + catch (obsolete) — Splinter Review
mochitest should fully cover the implementation.
Attachment #741182 - Attachment is obsolete: true
Attachment #741182 - Flags: review?(mounir)
Attachment #741182 - Flags: review?(bzbarsky)
Attachment #741275 - Flags: review?(mounir)
Attachment #741275 - Flags: review?(bzbarsky)
Better JSCompartment management.
Attachment #741274 - Attachment is obsolete: true
Attachment #741274 - Flags: review?(mounir)
Attachment #741274 - Flags: review?(bzbarsky)
Attachment #741305 - Flags: review?(mounir)
Attachment #741305 - Flags: review?(bzbarsky)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
This patch implements:

var future = Future.accept(42).done(function(what) { ... });
var future = Future.reject(42).catch(function(what) { ... });
var future = Future.resolve({then: function(a, b) {...
Attachment #742286 - Flags: review?(mounir)
Comment on attachment 741181 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

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

As a general comment, it would be better you do not quote the specification in your code. I think the best thing to do would be to give links in the header. For example, you could point to the FutureResolver and Future part of the specifications in their class declaration.

I'm not sure what to do regarding .reject(). It would be better if it was taking only a DOMError but calling `throw foo;` would allow to pass anything to the error callbacks so realisticly forcing to use a DOMError doesn't make much sense...

::: dom/future/Future.cpp
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +// FutureTask -----------------------------------------------------------------

nit: I think you can get rid of the "----". It's not a common coding style in Gecko.

@@ +22,5 @@
> +  NS_DECL_NSIRUNNABLE
> +
> +  FutureTask(Future* aFuture, Future::FutureState aState)
> +  : mFuture(aFuture)
> +  , mState(aState)

nit: +2 spaces indentation

@@ +24,5 @@
> +  FutureTask(Future* aFuture, Future::FutureState aState)
> +  : mFuture(aFuture)
> +  , mState(aState)
> +  {
> +    MOZ_ASSERT(mState == Future::Accepted || mState == Future::Rejected);

You might want to MOZ_ASSERT that aFuture isn't null.

@@ +41,5 @@
> +
> +NS_IMETHODIMP
> +FutureTask::Run()
> +{
> +  NS_ABORT_IF_FALSE(mFuture, "future object is needed");

Why aborting? Can't we just early return?

@@ +46,5 @@
> +  mFuture->RunTask(mState);
> +  return NS_OK;
> +}
> +
> +// Future ---------------------------------------------------------------------

nit: I think you can get rid of the "----". It's not a common coding style in Gecko.

@@ +57,5 @@
> +NS_INTERFACE_MAP_END
> +
> +Future::Future(nsPIDOMWindow* aWindow)
> +: mWindow(aWindow)
> +, mState(Pending)

nit: +2 spaces indentation.

@@ +141,5 @@
> +{
> +  MOZ_ASSERT(mState == Accepted || mState == Rejected);
> +
> +  if (aSync) {
> +    RunTask(mState);

That is weird: why do you pass mState to a member function?

Also, that let me wonder if we should change the state of the Future just before we actually call the callbacks or when .resolve() or .reject() is called. I tend to think that we former is the best behaviour but the specification says that we should do the later.

::: dom/future/Future.h
@@ +24,5 @@
> +class FutureInit;
> +class AnyCallback;
> +class FutureResolver;
> +
> +class Future : public nsISupports,

could be MOZ_FINAL

@@ +47,5 @@
> +  }
> +
> +  void SetState(FutureState aState)
> +  {
> +    mState = aState;

You could probably have those setters private and have FutureResolver a friend of Future so it will have access to it but no one else.

@@ +61,5 @@
> +    mResult = aValue;
> +  }
> +
> +  void ScheduleTask(bool aSync);
> +  void RunTask(FutureState aState);

Why is RunTask public?

@@ +88,5 @@
> +  FutureState mState;
> +  nsRefPtr<FutureResolver> mResolver;
> +
> +  nsTArray<nsRefPtr<AnyCallback> > mResolveCallbacks;
> +  nsTArray< nsRefPtr<AnyCallback> > mRejectCallbacks;

nit: you have a space between nsTArray< and nsRefPtr

@@ +96,5 @@
> +
> +} // namespace dom
> +} // namespace mozilla
> +
> +#endif

nit:
#endif // mozilla_dom_Future_h

::: dom/future/FutureResolver.cpp
@@ +21,5 @@
> +NS_INTERFACE_MAP_END
> +
> +FutureResolver::FutureResolver(Future* aParent)
> +: mParent(aParent)
> +, mResolvedFlag(false)

nit: two characters indentation

@@ +111,5 @@
> +    // reject with the thrown exception and the synchronous flag if set.
> +    // TODO
> +  }
> +
> +  Accept(aCx, aValue, true);

I'm not sure I understand that part of the specification. We should delay the resolve of the Future until the passed Future is resolved, right? So, why calling Accept() in all cases? Shouldn't that happen only if the passed value wasn't a Future?

::: dom/future/FutureResolver.h
@@ +27,5 @@
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(FutureResolver)
> +
> +  FutureResolver(Future* aParent);
> +  virtual ~FutureResolver();

You can mark the class MOZ_FINAL so you don't have to declare this virtual dtor that you actually don't use.

@@ +42,5 @@
> +    Accept(aCx, aValue, false);
> +  }
> +
> +  void Accept(JSContext* aCx, const Optional<JS::Value >& aValue,
> +              bool aSynchronousFlag);

Can't you just have a default value for aSynchronousFlag? Seems like this what you are doing anyway, right?

This said, for ::Reject(), you might want to have an WebIDL method exposing only DOMError() and an internal method able to take any kind of JS::Value even if the method will have to check that it is DOMError or JSException.

@@ +56,5 @@
> +              bool aSynchronousFlag);
> +
> +private:
> +  // Weak Pointer
> +  Future* mParent;

Can you really keep a weak pointer? What would happen if this code is called:
var r;
var f = new Future(function(resolver) {
  r = resolver;
});
f = 0;
setTimeout(function() { r.resolve(42); }, 10000);

I believe we might end up in the situation where |f| is no longer alive but |r| is still alive and will then try to call .reject() and crash.

We should probably just early return in that case (and we should have the specification say that).

Also, FWIW, I would not call this |mParent| but |mFuture|. That would be clearer.

@@ +59,5 @@
> +  // Weak Pointer
> +  Future* mParent;
> +
> +  bool mResolvedFlag;
> +  bool mSynchronousFlag;

nit: FWIW, the suffix "flag" isn't really needed here.

@@ +65,5 @@
> +
> +} // namespace dom
> +} // namespace mozilla
> +
> +#endif

nit:
#endif // mozilla_dom_FutureResolver_h

::: dom/future/Makefile.in
@@ +9,5 @@
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS,
> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +# See the License for the specific language governing permissions and
> +# limitations under the License.

What's that header? It should be the usual:
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

@@ +21,5 @@
> +
> +LIBRARY_NAME     = domfuture_s
> +LIBXUL_LIBRARY = 1
> +FORCE_STATIC_LIB = 1
> +EXPORT_LIBRARY = 1

What does EXPORT_LIBRARY do?

@@ +24,5 @@
> +FORCE_STATIC_LIB = 1
> +EXPORT_LIBRARY = 1
> +ifndef _MSC_VER
> +FAIL_ON_WARNINGS := 1
> +endif # !_MSC_VER

It would be great to have this as FAIL_ON_WARNINGS on Windows too. Did you try on Windows?

@@ +41,5 @@
> +  FutureResolver.cpp \
> +  $(NULL)
> +
> +include $(topsrcdir)/config/config.mk
> +include $(topsrcdir)/ipc/chromium/chromium-config.mk

Do you need chromium-config.mk?

::: dom/future/moz.build
@@ +8,5 @@
> +
> +XPIDL_MODULE = 'dom_future'
> +
> +MODULE = 'dom'
> +

nit: no need for that empty line

::: dom/future/tests/test_future.html
@@ +21,5 @@
> +function singleFutureAccept() {
> +  ok(!!Future, "Future object should exist");
> +
> +  var future = new Future(function(resolver) {
> +    ok(!!resolver, "FutureResolver exists");

ok(resolver, ...); should be fine.

@@ +24,5 @@
> +  var future = new Future(function(resolver) {
> +    ok(!!resolver, "FutureResolver exists");
> +    ok("accept" in resolver, "FutureResolver.accept exists");
> +    ok("reject" in resolver, "FutureResolver.reject exists");
> +    ok("accept" in resolver, "FutureResolver.accept exists");

You meant to check for resolve() I guess?

::: dom/webidl/Future.webidl
@@ +11,5 @@
> + * and create derivative works of this document.
> + */
> +
> +interface FutureResolver {
> +  void accept(optional any value);

I think we shouldn't implement that for the moment. The use case for this seems pretty low: it's when you want to pass a Future to .resolve() but the Future should be a value instead of a Future that has to be solved to solve the value. This can be very easily worked around by having the Future object wrapped inside another object.

Also, the Q promise library do not have something similar.

@@ +13,5 @@
> +
> +interface FutureResolver {
> +  void accept(optional any value);
> +  void resolve(optional any value);
> +  void reject(optional any value);

I think reject() should take a DOMError.
I do not think we allow DOMError objects to be created from the content but we should make that possible (it is now in the DOM specifications).
Attachment #741181 - Flags: review?(mounir) → review-
I forgot to add that we should put this behind a pref and only enable this for Nightly/Aurora for the moment as an experimental featuer.
(In reply to Mounir Lamouri (:mounir) from comment #20)
> I'm not sure what to do regarding .reject(). It would be better if it was
> taking only a DOMError but calling `throw foo;` would allow to pass anything
> to the error callbacks so realisticly forcing to use a DOMError doesn't make
> much sense...

I way to handle this would be to only propagate exceptions when they are DOMException objects and reject with undefined otherwise. So an exception from the UA would be propagated but content would have to use DOMError.
(In reply to Mounir Lamouri (:mounir) from comment #21)
> I forgot to add that we should put this behind a pref and only enable this
> for Nightly/Aurora for the moment as an experimental featuer.
I could not agree more. Even the spec this will be eventually part of (ECMAScript/a W3C spec) remains unclear at this point. The name might even end up being "promise".
Anyway, implementation is an excellent idea to get more developer feedback, but this feature is definitely not ready for prime-time.
You need to be able to reject with arbitrary rejection reasons; restricting to `DOMError`s is very uncool.
> As a general comment, it would be better you do not quote the specification
> in your code. I think the best thing to do would be to give links in the
> header. For example, you could point to the FutureResolver and Future part
> of the specifications in their class declaration.

I think it's easier to understand what the code does. But I'm ok to remove.

> I'm not sure what to do regarding .reject(). It would be better if it was
> taking only a DOMError but calling `throw foo;` would allow to pass anything
> to the error callbacks so realisticly forcing to use a DOMError doesn't make
> much sense...

Anne, feedback about this?

> @@ +141,5 @@
> > +{
> > +  MOZ_ASSERT(mState == Accepted || mState == Rejected);
> > +
> > +  if (aSync) {
> > +    RunTask(mState);
> 
> That is weird: why do you pass mState to a member function?

Because RunTask is called by FutureTask too.

> @@ +111,5 @@
> > +    // reject with the thrown exception and the synchronous flag if set.
> > +    // TODO
> > +  }
> > +
> > +  Accept(aCx, aValue, true);
> 
> I'm not sure I understand that part of the specification. We should delay
> the resolve of the Future until the passed Future is resolved, right? So,
> why calling Accept() in all cases? Shouldn't that happen only if the passed
> value wasn't a Future?

Right. But this is implemented by a separated patch. In this patch resolve() == accept().

> Can you really keep a weak pointer? What would happen if this code is called:

good point. I'll test it.


> > +interface FutureResolver {
> > +  void accept(optional any value);
> 
> I think we shouldn't implement that for the moment. The use case for this
> seems pretty low: it's when you want to pass a Future to .resolve() but the
> Future should be a value instead of a Future that has to be solved to solve
> the value. This can be very easily worked around by having the Future object
> wrapped inside another object.
> 
> Also, the Q promise library do not have something similar.

Anne... feedback?
Comment on attachment 741275 [details] [diff] [review]
part 2 - then + catch

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

r- because I think the code readability should be improved and the weak pointers should be taken care of. Generally speaking that code was harder to understand than the one in the previous patch even if it is way more focused.

::: dom/future/Future.h
@@ +100,5 @@
>    FutureState mState;
>    nsRefPtr<FutureResolver> mResolver;
>  
> +  nsTArray<nsRefPtr<FutureCallback> > mResolveCallbacks;
> +  nsTArray< nsRefPtr<FutureCallback> > mRejectCallbacks;

nit: you have a space squating between nsTArray< and nsRefPtr ;)

::: dom/future/FutureCallback.h
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +// FutureCallback -------------------------------------------------------------

nit: please don't add those "----".

@@ +19,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +
> +  FutureCallback(Future* aFuture, Future* aNextFuture,
> +                 AnyCallback* aCallback, bool aResolve);

Why not having three sub classes of FutureCallback: WrapperFutureCallback, ResolveFutureCallback and RejectFutureCallback. That way, the code might be easier to understand and it will no longer depend on magic depending on the value of the passed attributes? It will require the callers to know if they have to call WrapperFutureCallback but it might be better that way IMO.

It would prevent having to deal with mType and simply have ::Call() doing different thing depending on the sub-class.

@@ +32,5 @@
> +  void CallReject(const Optional<JS::Value >& aValue);
> +
> +  // Raw pointers FIXME
> +  Future* mFuture;
> +  Future* mNextFuture;

Do you intend to use strong ref? Is that actually needed?

@@ +46,5 @@
> +
> +} // namespace dom
> +} // namespace mozilla
> +
> +#endif

nit:
#endif // mozilla_dom_FutureCallback_h

::: dom/future/tests/test_future.html
@@ +113,5 @@
> +  }, function(what) {
> +    ok(false, "Then.reject has been called");
> +  });
> +
> +  ok(future !== future2, "These 2 future objs are different");

isnot(future, future2, ...);

@@ +162,5 @@
> +    is(what, 42, "Value == 42");
> +    return what + 1;
> +  });
> +
> +  ok(future !== future2, "These 2 future objs are different");

isnot()

@@ +211,5 @@
> +    throw(what + 1);
> +  }).catch(function(what) {
> +    ok(true, "Catch has been called");
> +    is(what, 43, "Value == 43");
> +    return what + 1;

Could you add the same test when you actually call `throw what+1;` instead of `return`?

::: dom/webidl/Future.webidl
@@ +33,4 @@
>  
> +  [Creator]
> +  Future then(optional AnyCallback? resolveCallback = null,
> +              optional AnyCallback? rejectCallback = null);

Same for the resolveCallback here. Why doing: future.then();

@@ +37,3 @@
>  
> +  [Creator]
> +  Future catch(optional AnyCallback? rejectCallback = null);

I do not think the callback should be optional. It doesn't make much sense to do:
future.catch();
Attachment #741275 - Flags: review?(mounir) → review-
Comment on attachment 741305 [details] [diff] [review]
part 3 - Resolver.resolve({then: ... })

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

I'm not sure what this patch is exactly trying to do. I would prefer a patch that handles the case where a Future is passed to .resolve(). It would be a first step and would help having a nice design before having to worry with handling "thenable" objects.

::: dom/future/FutureCallback.cpp
@@ +86,5 @@
>  
>  void
>  FutureCallback::CallResolve(const Optional<JS::Value >& aValue)
>  {
> +  MOZ_ASSERT(mNextFuture);

nit: that should be in the previous patch.

@@ +96,5 @@
>  
>  void
>  FutureCallback::CallReject(const Optional<JS::Value >& aValue)
>  {
> +  MOZ_ASSERT(mNextFuture);

ditto

::: dom/future/tests/test_future.html
@@ +224,5 @@
> +  var future = new Future(function(resolver) {
> +    resolver.resolve({
> +    foo: 42,
> +    then: function(resolve, reject) {
> +       ok(true, "Then in JS has been called");

Usually .then in a JS future looks like:
then(function (value) {
  // do something
}, function (error) {
  // handle error
});

You seem to assume that the .then() method will handle the resolve itself. It doesn't look like this is a common pattern. Weirdly it seems that the specification might actually ask for that...

@@ +267,5 @@
> +    ok(true, "Catch called after a JS Then");
> +    is(what, 42, "Value == 42");
> +    runTest();
> +  });
> +}

Could you add a test that pass a Future to resolver.resolve(), not a plain JS object with a then method.
Could you also have tests that have .then being a number, a string and other values that are not a function?
Attachment #741305 - Flags: review?(mounir) → review-
Comment on attachment 742286 [details] [diff] [review]
part 4 - Future.accept, Future.reject, Future.resolve

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

I would prefer to leave that to when we are done with the basic set of features.
Attachment #742286 - Flags: review?(mounir)
> I believe we might end up in the situation where |f| is no longer alive but |r| is still 
> alive and will then try to call .reject() and crash.
>
> We should probably just early return in that case (and we should have the specification say 
> that).

I guess FutureResolver could have a nsCOMPtr<Future> and nullify it when .reject() or .resolve() is called given that we should no longer reject or resolve anything after that. So, if Future was kept alive just by the resolver, it will be freed just after.


> Also, that let me wonder if we should change the state of the Future just before we actually 
> call the callbacks or when .resolve() or .reject() is called. I tend to think that we former 
> is the best behaviour but the specification says that we should do the later.

I guess the flag is "the future is being resolved" more than "the future is already resolved". I guess if we want to have a Future.resolved method, it will have to use another flag that would be set asynchronously if the Future is resolved async.

> I do not think the callback should be optional. It doesn't make much sense to do:
> future.catch();
(and for other first argument being optional)

http://lists.w3.org/Archives/Public/www-dom/2013AprJun/0094.html

> I'm not sure what to do regarding .reject(). It would be better if it was
> taking only a DOMError but calling `throw foo;` would allow to pass anything
> to the error callbacks so realisticly forcing to use a DOMError doesn't make
> much sense...

After more thinking, I think we shouldn't force developers to use DOMError but the spec should require DOM APIs to use DOMError and ask a method returning a Future to always have the same type of error object to reduce issues, more details here:
http://lists.w3.org/Archives/Public/www-dom/2013AprJun/0095.html

> > +interface FutureResolver {
> > +  void accept(optional any value);
> 
> I think we shouldn't implement that for the moment. The use case for this
> seems pretty low: it's when you want to pass a Future to .resolve() but the
> Future should be a value instead of a Future that has to be solved to solve
> the value. This can be very easily worked around by having the Future object
> wrapped inside another object.

After reading the hundreds of emails in script-coord regarding this issue, I think the best thing to do is to not implement .accept() for the moment and stick .resolve() to have a special behaviour when another Future is passed to it.
That is for the first implementation. Implementing "thenable" support might be interesting but it is not clear yet if this is something people want and .accept() seems controversial. It will be better if the first patches only implement bits that are safe enough so we can have a good base to discuss further improvements.
Attachment #741181 - Attachment is obsolete: true
Attachment #741181 - Flags: review?(bzbarsky)
Attachment #741275 - Attachment is obsolete: true
Attachment #741275 - Flags: review?(bzbarsky)
Attachment #741305 - Attachment is obsolete: true
Attachment #741305 - Flags: review?(bzbarsky)
Attachment #742286 - Attachment is obsolete: true
Attachment #747464 - Flags: review?(mounir)
Attached patch part 2 - then + catch (obsolete) — Splinter Review
Attachment #747465 - Flags: review?(mounir)
Attachment #747503 - Flags: review?(mounir)
Attachment #747504 - Flags: review?(mounir)
there was a broken mochitest
Attachment #747504 - Attachment is obsolete: true
Attachment #747504 - Flags: review?(mounir)
Attachment #747507 - Flags: review?(mounir)
Comment on attachment 747464 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

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

Generally speaking this patch is good but I see three serious enough issues on various aspects of the code (memory leak, behaviour and design) that I would like to see fixed before putting any stamp on it.

1. Future and FutureResolver have a reference to each other so something like this would be leaking as far as I can tell:
new Future(function(resolver) {});
You should make sure that they are declared as a cycle.

2. Future::mState shouldn't reflect FutureResolver::mResolved. Those are different things. The later is whether .resolve() or .reject() has been called. After the first call, any other call should be ignored. However, the former should change only if the Future is actually resolved. In the current setup (no delayed resolve), it isn't much more than a code design problem because the behaviour should still be fine. However, in a world where you can delay a resolve until another Future is resolved, you might end up with calling .then() and have mState = Resolved at that moment and so call the callbacks you passed to .then() even if the Future is still waiting to be resolved.
You should fix that by having the task resolving the Future marking it as resolved just before calling the callbacks.
Note that this also applies to reject.

3. RunTask(), ScheduleTask() are a bit weird and the ownership of those methods isn't clear. In addition, I think there are some issues related to setting the states too early: I do not understand why the specification says that we should set the states sync when the resolve is async. Everything should be async in that case otherwise, it is a door open to a lot of edge case problems such as point 2.

I think, the code around Future::RunTask(), Future::ScheduleTask(), FutureResolver::Resolve(), FutureResolver::Reject() should be re-designed that way:
FutureResolver::Resolve() and FutureResolver::Reject(), when call async should start a task (defined in FutureResolver.cpp) that would call a method that would look like this:

FutureResolver::ResolveTask()
{
  mFuture->Resolved(value);
 
  mFuture = nullptr;
}

You could make this handle Reject() and Resolve() by passing a pointer to the appropriate Future:: function.

So, that way, Future::Resolve() and Future::Reject() would set the correct state, set the mResult and call the callbacks synchronously as they should. We get everything done at the same moment.

Regarding ::RunTask() and ::ScheduleTask(), I think you shouldn't pass this |aState| around and I wonder if things wouldn't be nicer like this:

enum RunTaskSync {
  SyncTask,
  AsyncTask
};
void Future::RunTask(RunTaskSync aSync) {
  if (aSync == AsyncTask) {
    nsRefPtr<FutureTask> task = new FutureTask(this, mState); // FutureTask will call RunTask(SyncTask).
    NS_DispatchToCurrentThread(task);
    return;
  }

  // What ::RunTask() is currently doing.
}

I think that way, you remove the "aSync" vs "async" confusion and you remove the ::ScheduleTask() vs ::RunTask() confusion.

::: dom/future/Future.cpp
@@ +29,5 @@
> +    MOZ_ASSERT(mState == Future::Resolved || mState == Future::Rejected);
> +    MOZ_COUNT_CTOR(FutureTask);
> +  }
> +
> +  virtual ~FutureTask()

Could you make the class MOZ_FINAL and remove the virtual keyword from that dtor?

@@ +43,5 @@
> +NS_IMETHODIMP
> +FutureTask::Run()
> +{
> +  mFuture->RunTask(mState);
> +  return NS_OK;

I think it would be more readable to have that inlined in the declaration.

@@ +117,5 @@
> +
> +  // If future's state is resolved, queue a task to process future's resolve
> +  // callbacks with future's result. If future's state is rejected, queue a task
> +  // to process future's reject callbacks with future's result.
> +  if (mState == Resolved || mState == Rejected) {

nit: I would do != Pending

@@ +123,5 @@
> +  }
> +}
> +
> +void
> +Future::ScheduleTask(bool aSync)

For readability reasons, I would change all those |aSync| to |aASync|. I multiple times read "async" instead of "aSync".

::: dom/future/Future.h
@@ +38,5 @@
> +    Rejected
> +  };
> +
> +  Future(nsPIDOMWindow* aWindow);
> +  virtual ~Future();

You don't need a virtual dtor if the class can't be inherited from.

@@ +43,5 @@
> +
> +  FutureState State() const
> +  {
> +    return mState;
> +  }

That method isn't used anywhere. I think you should remove it. If it is needed in a later patch, for ease of review, it would be better to add it in the patch that uses it.

@@ +48,5 @@
> +
> +  JS::Value Result() const
> +  {
> +    return mResult;
> +  }

ditto

@@ +94,5 @@
> +
> +  JS::Value mResult;
> +
> +  friend class FutureResolver;
> +  friend class FutureTask;

nit: I think having the friend declaration on top is way nicer for readability.

::: dom/future/FutureResolver.cpp
@@ +6,5 @@
> +
> +#include "mozilla/dom/FutureResolver.h"
> +#include "mozilla/dom/FutureBinding.h"
> +#include "mozilla/dom/Future.h"
> +#include "nsContentUtils.h"

Why do you need to include nsContentUtils.h?

@@ +34,5 @@
> +}
> +
> +void
> +FutureResolver::Resolve(JSContext* aCx, const Optional<JS::Value >& aValue,
> +                        bool aSynchronous)

The beginning of this review as general comments about ::Resolve() and ::Reject().

@@ +53,5 @@
> +  // future's resolve callbacks with value. Otherwise, the synchronous flag is
> +  // unset, queue a task to process future's resolve callbacks with value.
> +  mFuture->ScheduleTask(aSynchronous);
> +
> +  // Decrease the refcounter.

No need to state the obvious ;)

@@ +75,5 @@
> +  // value. Otherwise, the synchronous flag is unset, queue a task to process
> +  // future's reject callbacks with value.
> +  mFuture->ScheduleTask(aSynchronous);
> +
> +  // Decrease the refcounter.

ditto

::: dom/future/FutureResolver.h
@@ +7,5 @@
> +#ifndef mozilla_dom_FutureResolver_h
> +#define mozilla_dom_FutureResolver_h
> +
> +#include "mozilla/Attributes.h"
> +#include "mozilla/ErrorResult.h"

You probably don't need that header, do you? (ErrorResult.h)

@@ +42,5 @@
> +    Resolve(aCx, aValue, false);
> +  }
> +
> +  void Resolve(JSContext* aCx, const Optional<JS::Value >& aValue,
> +               bool aSynchronous);

I think you can do:
void Resolve(JSContext* aCx, const Optional<JS:Value>& aValue, bool aSynchronous = false);

Should provide the same thing as declaring those two methods.

@@ +50,5 @@
> +    Reject(aCx, aValue, false);
> +  }
> +
> +  void Reject(JSContext* aCx, const Optional<JS::Value >& aValue,
> +              bool aSynchronous);

ditto

@@ +55,5 @@
> +
> +private:
> +  nsRefPtr<Future> mFuture;
> +
> +  bool mResolved;

Maybe you could call this "mResolvePending" and comment that it means that .resolve() or .reject() has been called and no more calls should be allowed. In some situations, the pending resolve might never succeed too (when you resolve on a Future that never gets resolved).

::: dom/future/tests/test_future.html
@@ +77,5 @@
> +  SpecialPowers.gc();
> +  SpecialPowers.forceGC();
> +  SpecialPowers.forceCC();
> +
> +  resolver.resolve(42);

That's great that you tested this! :)

@@ +81,5 @@
> +  resolver.resolve(42);
> +}
> +
> +var tests = [ singleFutureResolve, singleFutureReject,
> +              futureException, futureGC ];

Could you add a test that checks the async behaviour?
Something like:

var global = "foo";
var f = new Future(function(r) {
  is(global, "foo");
  r.resolve(42);
  is(global, "foo");
  setTimeout(function() {
    is(global, "bar");
  }, 0);
}).done(function() {
  global = "bar";
});;
is(global, "foo");

::: dom/webidl/Future.webidl
@@ +7,5 @@
> + * http://dom.spec.whatwg.org/#futures
> + *
> + * © Copyright 2004-2011 Apple Computer, Inc., Mozilla Foundation, and
> + * Opera Software ASA. You are granted a license to use, reproduce
> + * and create derivative works of this document.

I do not think you need that for IDL coming from the DOM specification. The DOM specification doesn't have that boilerplate (contrary to the HTML spec).

I do not know if we should mention the Public Domain or the OWFA agreement though.
Attachment #747464 - Flags: review?(mounir) → review-
Comment on attachment 747507 [details] [diff] [review]
part 4 - Future.reject, Future.resolve

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

I would prefer if we can focus on the features of the three first patches for the moment and work on that in a follow-up bug. The three first patches are what is required to actually have people try Future. This code here is just syntax sugar. It's good to have but not mandatory for a first version.
Attachment #747507 - Flags: review?(mounir)
Comment on attachment 747465 [details] [diff] [review]
part 2 - then + catch

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

::: dom/future/Future.cpp
@@ +110,5 @@
> +
> +  future->mResolver = new FutureResolver(future);
> +
> +  nsRefPtr<FutureCallback> resolveCb =
> +    FutureCallback::Factory(this, future, aResolveCallback, true);

You could pass future->mResolver instead of future.

@@ +113,5 @@
> +  nsRefPtr<FutureCallback> resolveCb =
> +    FutureCallback::Factory(this, future, aResolveCallback, true);
> +
> +  nsRefPtr<FutureCallback> rejectCb =
> +    FutureCallback::Factory(this, future, aRejectCallback, false);

ditto

@@ +178,5 @@
>      mResolveCallbacks.Clear();
>    } else if(aState == Rejected) {
>      callbacks = mRejectCallbacks;
>      mRejectCallbacks.Clear();
>    }

Just as a note, could you write tests that would break if this isn't there? (ie. .Clear() being called after the loop.)

::: dom/future/Future.h
@@ +43,5 @@
>    virtual ~Future();
>  
> +  FutureResolver* Resolver() const
> +  {
> +    return mResolver;

I believe you can remove this.

::: dom/future/FutureCallback.cpp
@@ +4,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/dom/Future.h"
> +#include "FutureCallback.h"

You might want to include FutureResolver.h (that was include in the header but shouldn't).

@@ +40,5 @@
> +ResolveFutureCallback::Call(const Optional<JS::Value >& aValue)
> +{
> +  // Run resolver's algorithm with value and the synchronous flag set.
> +  SafeAutoJSContext cx;
> +  mFuture->Resolver()->Resolve(cx, aValue, true);

mNextResolver->Resolve(...);

@@ +61,5 @@
> +RejectFutureCallback::Call(const Optional<JS::Value >& aValue)
> +{
> +  // Run resolver's algorithm with value and the synchronous flag set.
> +  SafeAutoJSContext cx;
> +  mFuture->Resolver()->Reject(cx, aValue, true);

mNextResolver->Reject(...);

@@ +85,5 @@
> +WrapperFutureCallback::Call(const Optional<JS::Value >& aValue)
> +{
> +  ErrorResult rv;
> +
> +  if (!mNextFuture) {

In this method, just rename mNextFuture to mNextResolver and the class should have a |Future* mFuture;| member in addition of the inherited |mNextResolver;|.

::: dom/future/FutureCallback.h
@@ +7,5 @@
> +#ifndef mozilla_dom_FutureCallback_h
> +#define mozilla_dom_FutureCallback_h
> +
> +#include "mozilla/dom/Future.h"
> +#include "mozilla/dom/FutureResolver.h"

I believe you can remove those #include and forward declare instead.

@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class FutureCallback : public nsISupports

Could you put some comments saying what this class is about. Do the same for the specialized classes.

@@ +24,5 @@
> +  virtual void Call(const Optional<JS::Value >& aValue) = 0;
> +
> +  static FutureCallback*
> +  Factory(Future* aFuture, Future* aNextFuture,
> +          AnyCallback* aCallback, bool aResolve);

You actually don't care about the next future, you care about the next future *resolver*. You should probably change that method to take the resolver.

@@ +27,5 @@
> +  Factory(Future* aFuture, Future* aNextFuture,
> +          AnyCallback* aCallback, bool aResolve);
> +
> +protected:
> +  Future* mFuture;

I guess that should be:
FutureResolver* mNextResolver;

That way, it will prevent mFuture being used as the next future in some callbacks but as the "current" future in some others.

Also, don't you want to keep that thing alive? Otherwise, wouldn't this code crash?
var res;
var f = new Future(function(res) {
  res = r;
}).then();
gc(); cc();
res.resolve(42);

I'm not 100% sure though but even if it doesn't crash, that sounds scary to keep a raw ref.

@@ +34,5 @@
> +class WrapperFutureCallback MOZ_FINAL : public FutureCallback
> +{
> +public:
> +  WrapperFutureCallback(Future* aFuture, Future* aNextFuture,
> +                        AnyCallback* aCallback);

This ctor should be private, to enforce the Factory model.

@@ +35,5 @@
> +{
> +public:
> +  WrapperFutureCallback(Future* aFuture, Future* aNextFuture,
> +                        AnyCallback* aCallback);
> +  virtual ~WrapperFutureCallback();

nit: no need to make that dtor virtual.

@@ +40,5 @@
> +
> +  void Call(const Optional<JS::Value >& aValue) MOZ_OVERRIDE;
> +
> +private:
> +  Future* mNextFuture;

I guess this can safely stay a raw pointer because it should be the Future that is owning the callback so if this callback is called, that means this Future is still alive. Is that correct?

@@ +48,5 @@
> +class ResolveFutureCallback MOZ_FINAL : public FutureCallback
> +{
> +public:
> +  ResolveFutureCallback(Future* aFuture);
> +  virtual ~ResolveFutureCallback();

ditto for ctor and dtor

@@ +57,5 @@
> +class RejectFutureCallback MOZ_FINAL : public FutureCallback
> +{
> +public:
> +  RejectFutureCallback(Future* aFuture);
> +  virtual ~RejectFutureCallback();

ditto

@@ +65,5 @@
> +
> +} // namespace dom
> +} // namespace mozilla
> +
> +#endif

nit: #endif // mozilla_dom_FutureCallback_h

::: dom/future/tests/test_future.html
@@ +228,5 @@
> +              futureException, futureDoubleDone,
> +              futureDoneException, futureGC,
> +              futureThenCatchDone, futureRejectThenCatchDone,
> +              futureRejectThenCatchDone2,
> +              futureRejectThenCatchExceptionDone ];

You should add a test where .then(), .catch() and .done() are called after a future is resolved:
var global = 0;
var f = new Future(function(r) {
  r.resolve(42);
}).done(function() {
  f.then(function() {
    global++;
  });
  f.catch(function() {
    global++;
  });
  f.done(function() {
    global++;
  });
  setTimeout(function() {
    is(global, 2);
  }, 0);
});
// and the same with r.reject();
Attachment #747465 - Flags: review?(mounir) → review-
Comment on attachment 747503 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

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

This is a quick review because I think this patch will anyway be re-written to apply comments on part 1 and part 2.

::: dom/future/FutureResolver.cpp
@@ +61,5 @@
>  
>    mFuture->SetState(Future::Resolved);
>    mFuture->SetResult(aValue.WasPassed() ? aValue.Value() : JSVAL_VOID);
>  
>    mResolved = true;

mResolved should be set just after if (mResolved) {:
if (mResolved) {
  return;
}

mResolved = true;

Otherwise, you will end up with this to fail:
var f = new Future(function(r) {
  r.resolve(new Future(...));
  r.reject(42);
}).done(function() {
  ok(true, "should be resolved");
}, function() {
  ok(false, "shouldn't be rejected");
});
Attachment #747503 - Flags: review?(mounir) → review-
Attachment #747464 - Attachment is obsolete: true
Attachment #748058 - Flags: review?(mounir)
Attached patch part 2 - then + catch (obsolete) — Splinter Review
Attachment #747465 - Attachment is obsolete: true
Attachment #748420 - Flags: review?(mounir)
With this patch we support nested futures:

function futureLoop() {
  new Future(function(resolver) {
    resolver.resolve(new Future(function(r) {
      ok(true, "Nested future is executed");
      r.resolve(new Future(function(r) {
        ok(true, "Nested nested future is executed");
        r.resolve(42);
      }));
    }));
  }).then(function(value) {
    is(value, 42, "Nested nested future is executed and then == 42");
    runTest();
  }, function(value) {
     ok(false, "This is wrong");
  });
}

I think it's nice :)
Attachment #747503 - Attachment is obsolete: true
Attachment #748421 - Flags: review?(mounir)
Attachment #747507 - Flags: review?(mounir)
Attached patch part 5 - pref (obsolete) — Splinter Review
Future behind prefs
Attachment #748696 - Flags: review?(mounir)
Attached patch part 5 - pref (obsolete) — Splinter Review
Attachment #748696 - Attachment is obsolete: true
Attachment #748696 - Flags: review?(mounir)
Attachment #748698 - Flags: review?(mounir)
Using Futures in DataStore, I found that it's possible to create nasty loops.
Attachment #748058 - Attachment is obsolete: true
Attachment #748058 - Flags: review?(mounir)
Attachment #750931 - Flags: review?(mounir)
Attached patch part 2 - then + catch (obsolete) — Splinter Review
Attachment #748420 - Attachment is obsolete: true
Attachment #748420 - Flags: review?(mounir)
Attachment #750932 - Flags: review?(mounir)
Comment on attachment 750931 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

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

r/sr=me with my comments applied.

We need a review from bz before landing though.

::: dom/future/Future.cpp
@@ +117,5 @@
> +  // If future's state is resolved, queue a task to process future's resolve
> +  // callbacks with future's result. If future's state is rejected, queue a task
> +  // to process future's reject callbacks with future's result.
> +  if (mState != Pending) {
> +    nsRefPtr<FutureTask> task = new FutureTask(this, mState);

Don't pass mState here.

@@ +125,5 @@
> +
> +void
> +Future::RunTask(FutureState aState)
> +{
> +  SetState(aState);

So, the only reason why you have SetState() here is because this method is called by FutureResolver().
I think you shouldn't pass FutureState here because it makes things weird. The two solutions you have are:
- add Future::Resolve() and Future::Reject() methods to Future that would call SetState(state); RunTask();
- otherwise, call mFuture->SetStates(state); mFuture->RunTask(); from FutureResolver.

@@ +135,5 @@
> +  mRejectCallbacks.Clear();
> +
> +  Optional<JS::Value> value(mResult);
> +
> +  for(uint32_t i = 0; i < callbacks.Length(); ++i) {

nit: "for ("

::: dom/future/Future.h
@@ +35,5 @@
> +  enum FutureState {
> +    Pending,
> +    Resolved,
> +    Rejected
> +  };

nit: move the enum to the |private| section so it's clear it's not in this implicit private section by mistake.

::: dom/future/FutureResolver.cpp
@@ +76,5 @@
> +  }
> +
> +  // TODO: if the arg is a future?
> +
> +  mFuture->SetResult(aValue.WasPassed() ? aValue.Value() : JSVAL_VOID);

You could pass that to ScheduleTask() to set the result when the task is done but we might end up copying this value in the Runnable which could have some memory footprint for a quite low benefit.

@@ +107,5 @@
> +}
> +
> +void
> +FutureResolver::ScheduleTask(Future::FutureState aState,
> +                             FutureTaskSync aAsynchronous)

I would prefer to have
FutureResolver::RunTask(Future::FutureState aState, FutureTaskAsync aAsynchronous)
{
  if (aAsynchronous) {
    nsRefPtr<FutureResolverTask> task = new FutureResolverTask(this, aState);
    NS_DispatchTeCurrentThread(task);
    return;
  }

  mFuture->RunTask(aState);
  mFuture->nullptr;
}

That way, callers need to know if they want an async or sync task. There is no magic with ::RunTask() being sync and ::ScheduleTask() being one or the other.

::: dom/future/FutureResolver.h
@@ +25,5 @@
> +
> +  enum FutureTaskSync {
> +    SyncTask,
> +    AsyncTask
> +  };

nit: could you move that to the private section? better to be explicit.

::: dom/future/Makefile.in
@@ +24,5 @@
> +  $(NULL)
> +
> +CPPSRCS += \
> +  Future.cpp \
> +  FutureResolver.cpp \

Note: this might move soon to moz.build, keep an eye on that.

::: dom/future/tests/Makefile.in
@@ +11,5 @@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +MOCHITEST_FILES = \
> +  test_future.html \

nit: feel free to not add every tests in the same file. Having multiple test files is fine.

::: dom/future/tests/test_future.html
@@ +97,5 @@
> +  is(global, "foo", "Global should still be foo (2)");
> +}
> +
> +var tests = [ singleFutureResolve, singleFutureReject,
> +              futureException, futureGC, futureAsync ];

Could you add tests that pass a lot of different objects to Future.resolve() and Future.reject(). Feel free to create another test file for that.
Some ideas of things you could pass: undefined, null, {}, { "foo", "bar" }, document.createElement('input'), window, resolver (the resolver itself), DOMError, function() {}, Blob, Future - but somehow, this should be tested in a later patch. Feel free to add stuff.

::: dom/webidl/Future.webidl
@@ +17,5 @@
> +
> +[Constructor(FutureInit init)]
> +interface Future {
> +  void done(optional AnyCallback? resolveCallback = null,
> +            optional AnyCallback? rejectCallback = null);

That has slightly changed in the spec. Basically, you can't pass |null| but you can pass |undefined|: [TreatUndefinedAs=Missing] optional AnyCallback cb.
Could you open a follow-up to take care of that?
Attachment #750931 - Flags: review?(mounir)
Attachment #750931 - Flags: review?(bzbarsky)
Attachment #750931 - Flags: review+
Attachment #750931 - Attachment is obsolete: true
Attachment #750931 - Flags: review?(bzbarsky)
Attachment #751081 - Flags: review?(bzbarsky)
I'll add the tests in a separated patch.
Comment on attachment 750932 [details] [diff] [review]
part 2 - then + catch

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

I wonder if the FutureCallback Factory isn't making things worse regarding readability.

r- because I believe that the Factory mess is the reason why you are passing mResolver to create the FutureCallbacks of ::Done() even if you don't need it.

::: dom/future/Future.cpp
@@ +132,5 @@
> +  nsRefPtr<FutureCallback> resolveCb =
> +    FutureCallback::Factory(this, mResolver, aResolveCallback, false, Resolved);
> +
> +  nsRefPtr<FutureCallback> rejectCb =
> +    FutureCallback::Factory(this, mResolver, aRejectCallback, false, Rejected);

Why are you passing mResolver here? You don't want |this| to be resolved again, do you? I think you shouldn't create a FutureCallback if the callback isnt't specified because it is useless in that case, isn't it?

::: dom/future/Future.h
@@ +31,5 @@
>  {
>    friend class FutureTask;
>    friend class FutureResolver;
>    friend class FutureResolverTask;
> +  friend class FutureCallback;

Why does FutureCallback need to be a friend of Future?

::: dom/future/FutureCallback.cpp
@@ +140,5 @@
> +  if (aState == Future::Rejected) {
> +    return new RejectFutureCallback(aNextResolver);
> +  }
> +
> +  MOZ_NOT_REACHED("This should not happen");

Use MOZ_ASSERT instead of MOZ_NOT_REACHED().

::: dom/future/FutureCallback.h
@@ +27,5 @@
> +  virtual void Call(const Optional<JS::Value >& aValue) = 0;
> +
> +  static FutureCallback*
> +  Factory(Future* aFuture, FutureResolver* aNextResolver,
> +          AnyCallback* aCallback, bool aRejectIfThrows,

Could you make that bool an enum?
enum RejectThrow {
  RejectIfThrow,
  NotRejectIfThrow
};

::: dom/webidl/Future.webidl
@@ +21,5 @@
> +  Future then(optional AnyCallback? resolveCallback = null,
> +              optional AnyCallback? rejectCallback = null);
> +
> +  [Creator]
> +  Future catch(optional AnyCallback? rejectCallback = null);

Like for Part 1, the specification changed on us. Please file a follow-up to apply those changes.
Attachment #750932 - Flags: review?(mounir) → review-
Comment on attachment 748421 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

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

Could you explain why you are adding those Resolved() and Rejected() methods? They seem to be used to bypass the mResolvePending check. Why is that?
Attachment #748421 - Flags: review?(mounir)
Comment on attachment 748698 [details] [diff] [review]
part 5 - pref

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

Should get r+ with the comments applied but I would like to see the test change.

::: dom/future/Future.cpp
@@ +79,5 @@
>  
> +/* static */ bool
> +Future::PrefEnabled()
> +{
> +  return Preferences::GetBool("dom.future.enabled", true);

Please, default to "false".

::: dom/future/tests/test_future.html
@@ +16,5 @@
>  <pre id="test">
>  <script type="application/javascript"><!--
>  
> +futureEnabled = SpecialPowers.getBoolPref("dom.future.enabled");
> +SpecialPowers.setBoolPref("dom.future.enabled", true);

I believe that you can use: specialpowers.pushPrefEnv

Also, could you actually try that Future isn't available when the pref is disabled?

::: dom/webidl/Future.webidl
@@ +15,5 @@
>  callback FutureInit = void (FutureResolver resolver);
>  callback AnyCallback = any (optional any value);
>  
> +[PrefControlled,
> + Constructor(FutureInit init)]

nit: can't you have all of that in the same line?

::: modules/libpref/src/init/all.js
@@ +1808,5 @@
>  // If true, ArchiveReader will be enabled
>  pref("dom.archivereader.enabled", false);
>  
> +// If true, Future will be enabled
> +pref("dom.future.enabled", false);

I think you can do:
#ifdef RELEASE_BUILD
pref("dom.future.enabled", false);
#else
pref("dom.future.enabled", true);
#endif
Attachment #748698 - Flags: review?(mounir) → review-
Comment on attachment 747507 [details] [diff] [review]
part 4 - Future.reject, Future.resolve

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

So, the code looks good but I'm not sure we want that for our first implementation. I am not sure of much people want that and even less how much they would use .reject() (I could see a tiny use case for .resolve()).

I will let Jonas make the call whether we should that with the first batch of patches or not.

::: dom/future/tests/test_future.html
@@ +249,5 @@
>  }
>  
> +function futureReject() {
> +  var future = Future.reject(42).done(function(what) {
> +  }, function(what) {

Could you add a ok(false, "..."); in the success callback?

@@ +259,5 @@
> +function futureResolve() {
> +  var future = Future.resolve(42).done(function(what) {
> +    is(what, 42, "Value == 42");
> +    runTest();
> +  });

Same here for the reject callback.
Attachment #747507 - Flags: superreview?(jonas)
Attachment #747507 - Flags: review?(mounir)
Attachment #747507 - Flags: review+
Attached patch part 2 - then + catch (obsolete) — Splinter Review
I introduced a new SimpleWrapperFunctionCallback just for Done(). So I can pass a null NextResolver and the Wrapper code is easier to read. I also removed the RejectIfThrow enum,
Attachment #750932 - Attachment is obsolete: true
Attachment #751320 - Flags: review?(mounir)
> Could you explain why you are adding those Resolved() and Rejected()
> methods? They seem to be used to bypass the mResolvePending check. Why is
> that?

This is exactly the reason why we have it :)
This about this code:

var f = new Future(function(r) {
  r.resolve(new Future(function(rr) { rr.resolve(42); });
}).then(something);

The steps are:

1. |f| is resolved. The value is a new future.
2. The new future is resolved, The value is 42.
3. This 42 is used as new resolve value of |f|
4. something() is called with 42 as value.

So the FutureResolve of |f| receives 2 values and it Resolve() method is (just internally) called twice. This is the reason why we have 2 methods:

Resolve() - the public one, that is called from JS and it has a check to deny double calls.
Resolved() - the private one that can be called several times replacing the value of |f|.

I say 'several times' because this is allowed:


var f = new Future(function(r) {
  r.resolve(new Future(function(rr) {
    rr.resolve(new Future(function(rrr) {
      rrr.resolve(42);
    });
  });
}).then(something);
This patch is just rebased on top of the others.
Attachment #748421 - Attachment is obsolete: true
Attachment #751323 - Flags: review?(mounir)
Attachment #751323 - Attachment is obsolete: true
Attachment #751323 - Flags: review?(mounir)
Attachment #751326 - Flags: review?(mounir)
Attachment #747507 - Attachment is obsolete: true
Attachment #747507 - Flags: superreview?(jonas)
Attachment #751327 - Flags: superreview?(jonas)
Attached patch part 5 - pref (obsolete) — Splinter Review
Attachment #748698 - Attachment is obsolete: true
Attachment #751329 - Flags: review?(mounir)
Attached patch part 6 - tests (obsolete) — Splinter Review
Here additional tests.
Attachment #751330 - Flags: review?(mounir)
Comment on attachment 751320 [details] [diff] [review]
part 2 - then + catch

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

r/sr=me

Flagging bz for the other review.
Attachment #751320 - Flags: review?(mounir)
Attachment #751320 - Flags: review?(bzbarsky)
Attachment #751320 - Flags: review+
Comment on attachment 751081 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

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

::: dom/future/Future.h
@@ +63,5 @@
> +  };
> +
> +  void SetState(FutureState aState)
> +  {
> +    mState = aState;

Could you add:
  MOZ_ASSERT(mState, Pending);
before the assignment so we are sure we never call that method twice.
Comment on attachment 751326 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

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

r/sr=me, with the naming issue addressed.

Also, as a side note, it is a bit weird that FutureCallback::Factory() takes a Future* as a first parameter, has a MOZ_ASSERT that makes it mandatory but doesn't use it in all situations like in the code you've just added in FutureResolver (you call FutureCallback::Factory(mFuture, mFuture->mResolver, ...); but mFuture is there only to prevent the assert). Could you fix that? (in any patch, and no need to ask for another review)

::: dom/future/FutureResolver.h
@@ +53,5 @@
>                FutureTaskSync aSync = AsyncTask);
>  
>  private:
> +  void Resolved(JSContext* aCx, const Optional<JS::Value >& aValue,
> +                FutureTaskSync aSync = AsyncTask);

I think the naming is unfortunate here. "Resolved()" is a name that, one would except, return whether the Future is resolved or not. Maybe you could rename the method called from script (like ResolveFromContent() or whatever) and name this method Resolve()?

@@ +56,5 @@
> +  void Resolved(JSContext* aCx, const Optional<JS::Value >& aValue,
> +                FutureTaskSync aSync = AsyncTask);
> +
> +  void Rejected(JSContext* aCx, const Optional<JS::Value >& aValue,
> +                FutureTaskSync aSync = AsyncTask);

ditto
Attachment #751326 - Flags: review?(mounir)
Attachment #751326 - Flags: review?(bzbarsky)
Attachment #751326 - Flags: review+
Comment on attachment 751329 [details] [diff] [review]
part 5 - pref

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

r=me with the comments applied

::: dom/future/tests/test_future.html
@@ +397,5 @@
>  }
>  
> +if (!SpecialPowers.getBoolPref("dom.future.enabled")) {
> +  ok(!("Future" in window), "Future object should not exist if disabled by pref");
> +}

Why not this:
  var p = SpecialPowers.getBoolPref("dom.future.enabled");
  SpecialPowers.setBoolPref("dom.future.enabled", false);
  ok(!("Future" in window), "Future object should not exist if disabled by pref");
  SpecialPowers.setBoolPref("dom.future.enabled", p);

or:
  // We assume that the pref is disabled by default for the moment.
  is(SpecialPowers.getBoolPref("dom.future.enabled"), false);
  ok(!("Future" in window), "Future object should not exist if disabled by pref");
Attachment #751329 - Flags: review?(mounir) → review+
Comment on attachment 751330 [details] [diff] [review]
part 6 - tests

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

r=me with the comments applied.

::: dom/future/tests/test_resolve.html
@@ +25,5 @@
> +  {},
> +  { a: 42 },
> +  [ 1, 2, 3, 4, null, true, "hello world" ],
> +  function() {},
> +  window,

Could you add:
  undefined,
  e10,
  document.createElement('input'),
  new Date(),

@@ +55,5 @@
> +}
> +
> +if (!SpecialPowers.getBoolPref("dom.future.enabled")) {
> +  ok(!("Future" in window), "Future object should not exist if disabled by pref");
> +}

No need for that.
Attachment #751330 - Flags: review?(mounir) → review+
Status: NEW → ASSIGNED
Attachment #751081 - Attachment is obsolete: true
Attachment #751081 - Flags: review?(bzbarsky)
Attachment #751691 - Flags: review?(bzbarsky)
Attachment #751326 - Attachment is obsolete: true
Attachment #751326 - Flags: review?(bzbarsky)
Attachment #751692 - Flags: review?(bzbarsky)
Attached patch part 5 - pref (obsolete) — Splinter Review
Attachment #751329 - Attachment is obsolete: true
Attached patch part 6 - tests (obsolete) — Splinter Review
Attachment #751330 - Attachment is obsolete: true
Blocks: 875289
Blocks: 875299
Comment on attachment 751327 [details] [diff] [review]
part 4 - Future.reject, Future.resolve

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

I'm going to punt this over to David Herman as a feedback request. David, would be great if you could put the feedback from TC39 in this bug. That way we can make sure to implement a compatible subset of the current TC39 suggestion (or is "agreement" the right noun to use).
Attachment #751327 - Flags: superreview?(jonas) → feedback?(dherman)
Comment on attachment 741174 [details] [diff] [review]
part 0 - ErrorResult::StealJSException

>+  MOZ_ASSERT(!mMightHaveUnreportedJSException,

That assert makes no sense.  At the very least, it's backwards.

But what this should _really_ be asserting, presumably is IsJSException(), and relying on the rest of the invariants to then hold.

That said, this also leaves the ErrorResult with an mJSException set such that it will try to report it ... but now not rooted.  Which is really bad.

I sort of hope the later patches don't actually use this function.  ;)
Attachment #741174 - Flags: review?(bzbarsky) → review-
Comment on attachment 751691 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

One caveat: I can check whether we're matching the spec, but whether the spec makes sense... <sigh>.

>+++ b/dom/future/Future.cpp
>+Future::Constructor(const GlobalObject& aGlobal, JSContext* aCx,
>+  ErrorResult rv;

We should just use aRv here, and make StealJSException reset state back to "no exception thrown" correctly.

>+    JS::Value exn = rv.StealJSException(aCx);
>+    Optional<JS::Value> value(exn);

How about:

  Optional<JS::Handle<JS::Value> > value(cx);
  rv.StealJSException(cx, &value.Value());

and make StealJSException take a JS::MutableHandle<JS::Value>.  Should help avoid some rooting issues.

>+Future::Done(AnyCallback* aResolveCallback, AnyCallback* aRejectCallback)

I assume this is just working around the lack of [TreatUndefinedAs=Missing], right?  If so, please make sure a followup is filed and mentioned in the comments in the IDL at least.

>+Future::AppendCallbacks(AnyCallback* aResolveCallback,

Why is this separate from its only caller, Done?  Just for later patches?

>+Future::RunTask()

Should MOZ_ASSERT(mState != mPending).

>+  Optional<JS::Value> value(mResult);

Again, you presumably want an optional handle here.

>+++ b/dom/future/Future.h
>+  void SetState(FutureState aState)

Should assert aState != pending.

>+  void SetResult(const JS::Value& aValue)

JS::Handle<JS::Value>, please.  And also, need to HOLD_JS_OBJECTS when this happens, or just in the future constructor for simplicity.

>+  void RunTask();

Document which part of the spec this corresponds to?

>+  FutureState mState;
>+  nsRefPtr<FutureResolver> mResolver;

Please put mState after all the pointer members.  And probably after the jsval too.

>+  JS::Value mResult;

This needs to be traced in your CC code.  Otherwise it can die under you.

>+++ b/dom/future/FutureResolver.cpp
>+  FutureResolverTask(FutureResolver* aResolver, const JS::Value& aValue,

Should take a Handle, but see more below.

>+  JS::Value mValue;

Needs to be added as a root in the constructor, removed in the destructor.  Or traced, I suppose.  But rooting seems simpler, using nsContentUtils::GetSafeJSContext to unroot for now, I guess.

>+  RunTask(aValue.WasPassed() ? aValue.Value() : JSVAL_VOID,

It's not clear to me what the right behavior is here.  If we were called with no argument, should we really pass "undefined" as an explicit argument to our callbacks?  Or call them with no argument?  The spec is underdefined here; see http://lists.w3.org/Archives/Public/www-dom/2013AprJun/0155.html

>+FutureResolver::RunTask(const JS::Value& aValue,

Again, handle.

I did not carefully review the test; let me know if you want me to.

r=me with the above addressed, though I'd like to see the interdiff.
Attachment #751691 - Flags: review?(bzbarsky) → review+
Comment on attachment 751320 [details] [diff] [review]
part 2 - then + catch

>+++ b/dom/future/Future.cpp
>+Future::Then(AnyCallback* aResolveCallback, AnyCallback* aRejectCallback)
>+  nsRefPtr<Future> future = new Future(GetParentObject());
>+  future->mResolver = new FutureResolver(future);

This pattern pops up a few times now.  It might be worth adding a static function on Future that does the resolver-creation and returns the already_AddRefed<Future> already with a resolver...

>+  nsRefPtr<FutureCallback> resolveCb =
>+    FutureCallback::Factory(this, future->mResolver, aResolveCallback,

I'm not sure that first argument is right.

The only places it ends up used are SimpleWrapperFutureCallback and WrapperFutureCallback.  But this is not going to end up as a SimpleWrapperFutureCallback.  And for the WrapperFutureCallback, that's implementing the concept of "future wrapper callback" from the spec.  And we seem to be passing its mFuture as the thisObj.  But in the spec, the thisObj for a future wrapper callback invocation is the future of the resolver being wrapped.  So in this case that would be "future", not "this".  And more to the point, should be derivable from the resolver so shouldn't need to be stored.

In other words, it should be ok to pass null here for the first argument and in the factory method assert that exactly one of resolver and future is nonnull.

That said, the factory method is a bit weird.  It either takes a future and a non-null callback and produces a SimpleWrapperFutureCallback or it takes a resolver and an optional callback and a Task an produces a WrapperFutureCallback or ResolveFutureCallback or RejectFutureCallback.  It might make sense to have two separate Factory methods for the "future and callback" and "resolver and task and maybe callback" cases, since they have pretty different arguments.

>+++ b/dom/future/FutureCallback.cpp
>+ResolveFutureCallback::ResolveFutureCallback(FutureResolver* aResolver)

This needs to be cycle-collected, so it doesn't leak in a cycle through mResolver.

>+ResolveFutureCallback::Call(const Optional<JS::Value >& aValue)
>+  SafeAutoJSContext cx;
>+  mResolver->Resolve(cx, aValue, FutureResolver::SyncTask);

You need to enter aValue's compartment as needed here, no?  And it needs to be a handle.

All three of these comments also apply to RejectFutureCallback.

>+WrapperFutureCallback::WrapperFutureCallback(Future* aFuture,

This also needs to be cycle collected.

>+WrapperFutureCallback::Call(const Optional<JS::Value >& aValue)

Handle.

>+  SafeAutoJSContext cx;

Again, you need to enter compartments on this below as needed.

>+  JSAutoRequest ar(cx);

Why is this needed here but not in RejectFutureCallback and ResolveFutureCallback?  Seems like it's needed there too, if it's needed at all.

>+  JS::Value ret = mCallback->Call(mFuture, aValue, rv,

This needs to be rooted.

>+SimpleWrapperFutureCallback::SimpleWrapperFutureCallback(Future* aFuture,

Again, needs to be CCed.

>+SimpleWrapperFutureCallback::Call(const Optional<JS::Value >& aValue)

Handle.

>+++ b/dom/future/FutureCallback.h
>+  static FutureCallback*
>+  Factory(Future* aFuture, FutureResolver* aNextResolver,

Document that this returns a refcounted object with a refcount of 0?

>+// WrapperFutureCallback execs a JS Callback with a value and then, the return

That comma should be between "value" and "and".

>+++ b/dom/future/tests/test_future.html
>+  window.onerror = function(e) {
>+    ok(true, "window.onerror has been called!");
>+    window.onerror = null;

Mochitests use window.onerror to catch unexpected exceptions in tests.  You want to restore it once your custom handling is done, instead of clobbering it.

r=me with the above fixed, but I _definitely_ want to see the updated version with the CC done right.
Attachment #751320 - Flags: review?(bzbarsky) → review+
Comment on attachment 751692 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

I don't understand the Resolve/ResolveInternal and Reject/RejectInternal split.  We want to be checking the "resolved flag" thing on all invocations of the resolve algorithm, no?

>+++ b/dom/future/FutureCallback.cpp
> FutureCallback::Factory(Future* aFuture, FutureResolver* aNextResolver,

The changes here don't make sense either: the current code never passes a null aFuture, right?

>+++ b/dom/future/FutureResolver.cpp
>+FutureResolver::ResolveInternal(JSContext* aCx,
>+    JSObject* valueObj = &aValue.Value().toObject();

You want a Rooted here.

>+    nsresult rv = UnwrapObject<Future>(aCx, valueObj, nextFuture);
>+    if (NS_SUCCEEDED(rv) && nextFuture) {

If NS_SUCCEEDED(rv), then nextFuture will definitely not be null.

>+        FutureCallback::Factory(mFuture, mFuture->mResolver, nullptr,
>+        FutureCallback::Factory(mFuture, mFuture->mResolver, nullptr,

mFuture->mResolver seems like a complicated way to write "this". ;)

This is not following the spec in terms of calling then() on valueObj, but I gather we're not convinced about that part of the spec yet?

r-, because the *Internal stuff seems wrong....
Attachment #751692 - Flags: review?(bzbarsky) → review-
By the way, I suspect these patches haven't been merged to tip in a bit, since the Optional<Value> stuff shouldn't even be compiling in binding signatures nowadays...
Oh, one more comment on part 1: if I have a no longer pending future and I call done() on it twice, we'll post two FutureTasks, but only the first one will do anything useful.  Might make sense to skip posting a new FutureTask if there is one live already (though we'll then need to keep track of that).
Attachment #741174 - Attachment is obsolete: true
Attachment #756610 - Flags: review?(bzbarsky)
> I assume this is just working around the lack of [TreatUndefinedAs=Missing],
> right?  If so, please make sure a followup is filed and mentioned in the
> comments in the IDL at least.

Yep. already created: 875289


> Why is this separate from its only caller, Done?  Just for later patches?

right.
Interdiff... I forgot to generate it, sorry.
Attachment #751691 - Attachment is obsolete: true
Attachment #756614 - Flags: review?(bzbarsky)
sometimes, hg qref helps.
Attachment #756614 - Attachment is obsolete: true
Attachment #756614 - Flags: review?(bzbarsky)
Attachment #756615 - Flags: review?(bzbarsky)
Comment on attachment 756610 [details] [diff] [review]
part 0 - ErrorResult::StealJSException

This needs documentation.  In particular, you need to document in the header the interplay with WouldReportJSException() (see the existing documentation like that for ReportJSException, which you can probably just modify to say "ReportJSException or StealJSException").

You should also document (and assert) that this should only be called when IsJSException() and document that it resets the state to a non-failed state.

r=me with that.
Attachment #756610 - Flags: review?(bzbarsky) → review+
Comment on attachment 756615 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

Per IRC discussion, this should also traverse/unlink mResolveCallbacks and mRejectCallbacks.

You also need to initialize mResult to UndefinedValue() in the Future constructor, because tracing uninitialized memory is not great.

>+  if (aRv.Failed() && aRv.IsJSException()) {

You don't even need the Failed() check.  IsJSException() implies Failed().

>+    Optional<JS::Handle<JS::Value> > value(aCx, JS::UndefinedValue());

You don't need the second argument.  Just pass aCx and it will do the right thing.  More importantly, you don't _want_ the second argument, because it will show up as a static analysis failure.  We really need to fix that.  :(

>+    aRv = NS_OK;

Take that out.  StealJSException did it for you.

This can still have two FutureTasks for the same future in flight at once.  Followup on addressing that, I guess, with the bug# in code comments here?

>+  Optional<JS::Handle<JS::Value> > value(nsContentUtils::GetSafeJSContext(),
>+                                         mResult);

This might end a static analysis fail, but I'll deal with it later if so...

>+  void SetResult(const JS::Handle<JS::Value>& aValue)

  void SetResult(JS::Handle<JS::Value> aValue)

A handle is already a fancy reference; this is like passing a pointer as "Foo* const&".  ;)

>+  void RunTask();

Still need to document this.

>+  FutureResolverTask(FutureResolver* aResolver,
>+                     const JS::Value& aValue,

Still needs to become a handle.  Otherwise it will add static rooting analysis failures.

>+    JS_AddValueRoot(nsContentUtils::GetSafeJSContext(), &mValue);

Ah, if you're using the safe js context here, you want to use JS_AddNamedValueRootRT instead, or enter a request on that context, or something.

>+    JS::Rooted<JS::Value> value(nsContentUtils::GetSafeJSContext(), mValue);
>+    mResolver->RunTask(value, mState, FutureResolver::SyncTask);

  mResolver->RunTask(JS::Handle<JS::Value>::fromMarkedLocation(&mValue),
                     mState, FutureResolver::SyncTask);

>+FutureResolver::RunTask(const JS::Handle<JS::Value>& aValue,

Again, handles are passed by value.

You still need comments in the IDL pointing to the followup bugs to make it match spec.

r=me with those issues addressed.  But please do address them!
Attachment #756615 - Flags: review?(bzbarsky) → review+
Attachment #756610 - Attachment is obsolete: true
Comment on attachment 756894 [details] [diff] [review]
part 0 - ErrorResult::StealJSException

No need to copy the ThrowJSException comments.  Just change "ReportJSException" to "ReportJSException or StealJSException" in the comment above, and drop everything in the new comment starting with "The contract".
Attachment #756894 - Attachment is obsolete: true
Attachment #756615 - Attachment is obsolete: true
> >+    Optional<JS::Handle<JS::Value> > value(aCx, JS::UndefinedValue());
> 
> You don't need the second argument.  Just pass aCx and it will do the right
> thing.  More importantly, you don't _want_ the second argument, because it
> will show up as a static analysis failure.  We really need to fix that.  :(

The second argument is needed. 
https://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingDeclarations.h#304

> This can still have two FutureTasks for the same future in flight at once. 
> Followup on addressing that, I guess, with the bug# in code comments here?

I fixed it with a boolean. I think it's enough.

> Still needs to become a handle.  Otherwise it will add static rooting
> analysis failures.
> 
> >+    JS_AddValueRoot(nsContentUtils::GetSafeJSContext(), &mValue);

Another glance... ?
Attachment #756944 - Attachment is obsolete: true
> The second argument is needed. 

Hmm.  We should add a one-argument constructor there...

That last attachement is just a WIP that doesn't address all the comments from comment 82, right?
can you check this one-argument constructor?
For the rest I think I covered all your comments.
Attachment #756943 - Attachment is obsolete: true
Attachment #757092 - Flags: review?(bzbarsky)
Comment on attachment 757092 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

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

::: dom/future/Makefile.in
@@ +25,5 @@
> +
> +CPPSRCS += \
> +  Future.cpp \
> +  FutureResolver.cpp \
> +  $(NULL)

All of these should go into moz.build.
Another review just for this one-argument param, please.
Attachment #756945 - Attachment is obsolete: true
Attachment #757092 - Attachment is obsolete: true
Attachment #757092 - Flags: review?(bzbarsky)
Attachment #757301 - Flags: review?(bzbarsky)
Attached patch part 2 - then + catch (obsolete) — Splinter Review
Attachment #751320 - Attachment is obsolete: true
Attachment #757303 - Flags: review?(bugs)
> I don't understand the Resolve/ResolveInternal and Reject/RejectInternal
> split.  We want to be checking the "resolved flag" thing on all invocations
> of the resolve algorithm, no?

Take a look at comment 54.
Attached patch part 2 - then + catch (obsolete) — Splinter Review
Attachment #757303 - Attachment is obsolete: true
Attachment #757303 - Flags: review?(bugs)
Attachment #757311 - Flags: review?(bugs)
Attachment #751692 - Attachment is obsolete: true
Attachment #757312 - Flags: review?(bzbarsky)
> Take a look at comment 54.

Per IRC discussion, comment 54 and the patch do not match the spec at all.  Andrea will talk to Anne about whether this is a bug in the spec or in our code; I can't tell because of editorial issues with the spec (like not explaining what the goal is).
Blocks: web-crypto
Comment on attachment 757311 [details] [diff] [review]
part 2 - then + catch

r+ for the CC part. 
But I don't understand all the stuff here.
Use of JSAutoRequest is probably wrong, you want AutoSafeJSContext I think.
Attachment #757311 - Flags: review?(bugs) → review+
Attached patch interdiff - part 1 (obsolete) — Splinter Review
Attachment #757423 - Flags: review?(bzbarsky)
Attached patch interdiff - part 2 (obsolete) — Splinter Review
Attachment #757424 - Flags: review?(bzbarsky)
Comment on attachment 757423 [details] [diff] [review]
interdiff - part 1

>+++ b/dom/bindings/BindingDeclarations.h
>+    Optional_base<JS::Handle<T>, JS::Rooted<T> >(cx, JSVAL_VOID)

This will fail the moment someone tries to use this with Optional<Handle<JSObject*> >.

What you want instead is to use the no-argument constructor on Optional_base and then this->Construct(cx).

>+++ b/dom/future/Future.h

>+  void SetResult(const JS::Handle<JS::Value> aValue)

Drop the const, since passing by value.

>+++ b/dom/future/FutureResolver.cpp
>   FutureResolverTask(FutureResolver* aResolver,
>+    JS_AddNamedValueRootRT(JS_GetRuntime(cx), &mValue, "mValue");

"FutureResolverTask.mValue" for the last argument, please.

>+FutureResolver::RunTask(const JS::Handle<JS::Value> aValue,

Drop the const.

r=me
Attachment #757423 - Flags: review?(bzbarsky) → review+
Comment on attachment 757301 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

r=me
Attachment #757301 - Flags: review?(bzbarsky) → review+
Comment on attachment 757424 [details] [diff] [review]
interdiff - part 2

>+++ b/dom/future/Future.cpp
>+  mResolver = new FutureResolver(this);

This is somewhat suspect in the "run virtual functions before the constructor is done" sense and the "addref before constructor is done" sense...

I guess it's ok as long as we're very careful with this constructor.

>+  nsRefPtr<FutureCallback> resolveCb;
>+  if (aResolveCallback) {
>+    resolveCb = FutureCallback::CallbackFactory(nullptr, future->mResolver,
>+                                                aResolveCallback);
>+  } else {
>+    resolveCb = FutureCallback::TaskFactory(future->mResolver,
>+                                            FutureCallback::Resolve);
>+  }

So what I was thinking this would do is something like:

  nsRefPtr<FutureCallback> resolveCb = 
    FutureCallback::CallbackFactory(future->mResolver, aResolveCallback, 
                                    FutureCallback::Resolve);

which would do the null-check on aResolveCallback and return either a WrapperFutureCallback or a ResolveFutureCallback or a RejectFutureCallback.  And then we'd have another method that does the "future and callback, not resolver" thing, which would always create a SimpleWrapperFutureCallback (and could in fact just be the relevant constructor, I'd think).

That would avoid having all these duplicated nullchecks while separating out clearly the "we plan to call something on the given resolver" and "we just plan to call this callback" cases.  It would also avoid all the nullptr arguments....

>+++ b/dom/future/FutureCallback.cpp
>+ResolveFutureCallback::Call(const Optional<JS::Handle<JS::Value> >& aValue)
>+{
>+  // Run resolver's algorithm with value and the synchronous flag set.
>+  JSContext* cx = nsContentUtils::GetSafeJSContext();
>+  JSAutoRequest ar(cx);

You stil need to enter the right compartment as needed.  And you probably need to push the cx on the stack... 

What's probably better here is an AutoJSContext and then entering the right compartment.

>+RejectFutureCallback::Call(const Optional<JS::Handle<JS::Value> >& aValue)

Similar here.

>+WrapperFutureCallback::Call(const Optional<JS::Handle<JS::Value> >& aValue)

And here.

>+    Optional<JS::Handle<JS::Value> > value(cx, JS::UndefinedValue());

Should be able to drop the JS::UndefinedValue bit here, since we have a one-arg form of the constructor.

>+  Optional<JS::Handle<JS::Value> > value(cx, ret);

Fwiw, you could just make "ret" be of type Optional<JS::Handle<JS::Value> >, I expect, and then you wouldn't need to make this new object here.

>+FutureCallback::CallbackFactory(Future* aFuture, FutureResolver* aNextResolver,
>+  MOZ_ASSERT((!aFuture && aNextResolver) || (aFuture && !aNextResolver));

See, this sort of assert tells me "you want two methods".
Attachment #757424 - Flags: review?(bzbarsky) → review-
Comment on attachment 757312 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

I'm going to wait until you sort out the "internal" stuff with Anne.
Attachment #757312 - Flags: review?(bzbarsky)
The semantics of futures/promises was discussed at the last TC39 meeting.  Here's a report on what was agreed to, and what is still open.  I also wrote something about this discussion here: http://esdiscuss.org/topic/theparadoxofpartialparametricity#content-28

First, most of the discussion was based on Alex Russell's Promise.idl here: https://github.com/slightlyoff/Futures/blob/master/Promise.idl and Mark Miller's presentation here: http://wiki.ecmascript.org/lib/exe/fetch.php?id=strawman%3Apromises&cache=cache&media=strawman:promisesvsmonads2.pdf 

Most fundamentally, there's agreement [1] that it should be possible to create promises for promises, and also that using `.then()` should allow programmers to ignore the existence of promises for promises.

The remaining questions are about the details of the API: are there two methods on promises, .then() and .chain(), or just .then()? How are promises for promises created? When recursive resolution happens, is it on the input or output of .then()?

Sadly, this probably doesn't give anyone enough information to help with implementation.

[1] Really, there's a realization that consensus is impossible without including both.
Attached patch interdiff - part 2 b (obsolete) — Splinter Review
I still don't know if the compartments and the JSContext* are properly used. Reading other components, it seems that we don't enter to a compartment explicitly when AutoJSContext is used.
Attachment #757424 - Attachment is obsolete: true
Attachment #757876 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #105)
> Comment on attachment 757312 [details] [diff] [review]
> part 3 - Resolver.resolve(new Future(...))
> 
> I'm going to wait until you sort out the "internal" stuff with Anne.

I spoke with Anne on IRC. The spec is not clear at all about how many times Resolver.resolve() can be called. It can be called many times but only the first time it does something. So this implementation is right.

Probably we will have several follow ups:

. the API is going to be renamed 'Promise'
. we still want the 'thenable' objects
. accept() (currently not implemented) is needed and it's called fulfill().
. done() is going to be removed or renamed finally()
Comment on attachment 757876 [details] [diff] [review]
interdiff - part 2 b

> I still don't know if the compartments and the JSContext* are properly used.

Then how about actually applying my review comments, maybe?

> Reading other components, it seems that we don't enter to a compartment
> explicitly when AutoJSContext is used.

It REALLY DEPENDS ON WHAT YOU DO WITH IT.  In your case, you're calling methods that take a JSContext and a value.  You have to ensure they're in the same compartment, period.  If you don't, you can get very nice runtime fatal asserts in the JS engine when you try to do something with the value.
Attachment #757876 - Flags: review?(bzbarsky) → review-
> So this implementation is right.

Great.  You've raised a spec issue, right?

> Probably we will have several follow ups:

Sure.
Attached patch interdiff - part 2 c (obsolete) — Splinter Review
Attachment #757876 - Attachment is obsolete: true
Attachment #757926 - Flags: review?(bzbarsky)
Blocks: 879245
I filed a follow up for the then-able objects in Resolver.resolve().
Attachment #757312 - Attachment is obsolete: true
Attachment #757929 - Flags: review?(bzbarsky)
Attachment #757929 - Flags: review?(bzbarsky)
Depends on: 878849
Comment on attachment 757926 [details] [diff] [review]
interdiff - part 2 c

r=me modulo the brokenness pointed out in bug 878849 (which you can't do anything about), but don't you still need review on "part 2 b"?
Attachment #757926 - Flags: review?(bzbarsky) → review+
> r=me modulo the brokenness pointed out in bug 878849 (which you can't do
> anything about), but don't you still need review on "part 2 b"?

No thanks. "part 2 c" is "part 2 b" + your comments. Let me upload here the full patch.
Attached patch part 2 - then + catch (obsolete) — Splinter Review
Attachment #757311 - Attachment is obsolete: true
Attachment #757423 - Attachment is obsolete: true
Attachment #757926 - Attachment is obsolete: true
This patch (rebased on the latest changes) contains the output of the discussion with annevk.
Attachment #757929 - Attachment is obsolete: true
Attachment #758322 - Flags: review?(bzbarsky)
Attached patch interdiff - part 2 (obsolete) — Splinter Review
new interdiff for part 2.
Attachment #758567 - Flags: review?(bzbarsky)
Comment on attachment 758567 [details] [diff] [review]
interdiff - part 2

Thank you for the interdiff!

r=me
Attachment #758567 - Flags: review?(bzbarsky) → review+
Comment on attachment 758322 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

r=me
Attachment #758322 - Flags: review?(bzbarsky) → review+
So it sounds to me like the only truly stable part of the API here is the fact that a return Promise object has a .then() function, and that .then() function takes two callback arguments, one which is called on success, and one which is called on error. The success callback is never called with a Promise as the argument.

What's not stable is the Promise constructor(s). I.e. what are the names of the functions on the PromiseResolver interface, what are the "static" functions on the Promise constructor function, and what is the exact behavior of these functions.

Likewise, there are no more stable functions than .then() on the promise instances themselves.

So I have a bit of a crazy idea. How about we implement what is looking to us like "the best" API around promise construction. But the only thing we ship past aurora is the ability for internal APIs to create Promise instances and return them.

I.e. we pref of both the global Promise() interface-object, and the constructors that go on it. And we for now don't implement any other functions than .then() on promise instances.
We don't really have a good mechanism for preffing off the interface object but handing out objects with that interface, actually.

What we could do instead is make the interface [NoInterfaceObject] (and constructor-less) until we're ready...
I would like to land what we have here behind a pref. I'm already renaming and implementing all the changes from what is proposed here to what the spec wants in a separated bug: Bug 875289.
So I talked to billm about the compartment-entering stuff.

I think the right way to do this is to ensure that aCx is never used in reject() and accept() (via not giving it a name and adding a comment to that effect) and pass a null cx from internal callers that don't actually have a cx to pass.  And document the whole setup.

A followup for that is probably fine.
Attached patch interdiff for CC (obsolete) — Splinter Review
Attachment #760968 - Flags: review?(bugs)
Attachment #760968 - Flags: review?(bugs) → review+
Attachment #757301 - Attachment is obsolete: true
Attachment #758567 - Attachment is obsolete: true
Attachment #760968 - Attachment is obsolete: true
Attachment #760970 - Attachment is obsolete: true
Attachment #758317 - Attachment is obsolete: true
Any changes in the webIDL will be covered in a separated bug.
Attachment #751327 - Attachment is obsolete: true
Attachment #751327 - Flags: feedback?(dherman)
Attached patch part 5 - pref (obsolete) — Splinter Review
Attachment #751695 - Attachment is obsolete: true
Attached patch part 6 - testsSplinter Review
Attachment #751697 - Attachment is obsolete: true
all the patches are rebased.
Keywords: checkin-needed
Green on try.
Keywords: checkin-needed
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/6a04dea9817f - you were indeed green(ish) on try, for those things which have debug tests. Those things don't include Android, which seems to have been fine anyway, or b2g, which was not, failing with the unfortunate sounding https://tbpl.mozilla.org/php/getParsedLog.php?id=24040902&tree=Mozilla-Inbound
Comment on attachment 761003 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

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

::: dom/future/tests/test_future.html
@@ +18,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +function singleFutureResolve() {
> +  ok(Future, "Future object should exist");

This looks like it'll throw a ReferenceError instead of failing gracefully. ok(window.Future) or something like that...

Though those tests should be written using testharness.js and submitted to the spec's test suite, really.
Blocks: 882076
> This looks like it'll throw a ReferenceError instead of failing gracefully.
> ok(window.Future) or something like that...
> 
> Though those tests should be written using testharness.js and submitted to
> the spec's test suite, really.

Correct. I'll do that once Promise renaming will be reviewed.
No longer blocks: 882076
Blocks: 882076
Attached patch part 5 - prefSplinter Review
Attachment #761006 - Attachment is obsolete: true
Keywords: checkin-needed
Depends on: 883683
Blocks: 885333
Depends on: 887687
Blocks: 899557
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.