Last Comment Bug 856410 - (promises) Implement promises
(promises)
: Implement promises
Status: RESOLVED FIXED
[Async]
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla24
Assigned To: Andrea Marchesini (:baku)
:
Mentors:
http://dom.spec.whatwg.org/#promises
: 839064 (view as bug list)
Depends on: 878849 956343 883683 887687 897913 904143 939906 942255 958684 1008467 1009569 1013625
Blocks: 854627 web-crypto 885333 899557 1038557 839838 875018 875289 875299 inter-app-comm-api 879245 882076 915233 939909 1035060
  Show dependency treegraph
 
Reported: 2013-03-31 06:03 PDT by Anne (:annevk)
Modified: 2014-07-14 23:49 PDT (History)
43 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (11.61 KB, patch)
2013-04-10 06:48 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 1 - WebIDL + constructor + Done (23.58 KB, patch)
2013-04-23 11:37 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 2 - then + catch (7.52 KB, patch)
2013-04-23 12:21 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 0 - ErrorResult::StealJSException (2.03 KB, patch)
2013-04-23 22:54 PDT, Andrea Marchesini (:baku)
bzbarsky: review-
Details | Diff | Review
part 1 - WebIDL + constructor + Done (23.89 KB, patch)
2013-04-23 23:27 PDT, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
part 2 - then + catch (19.74 KB, patch)
2013-04-23 23:31 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 3 - Resolver.resolve({then: ... }) (10.35 KB, patch)
2013-04-24 06:02 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 2 - then + catch (20.36 KB, patch)
2013-04-24 06:03 PDT, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
part 3 - Resolver.resolve({then: ... }) (9.86 KB, patch)
2013-04-24 07:25 PDT, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
part 4 - Future.accept, Future.reject, Future.resolve (6.56 KB, patch)
2013-04-26 03:12 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 1 - WebIDL + constructor + Done (19.99 KB, patch)
2013-05-09 08:54 PDT, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
part 2 - then + catch (17.48 KB, patch)
2013-05-09 08:55 PDT, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
part 3 - Resolver.resolve(new Future(...)) (3.61 KB, patch)
2013-05-09 10:20 PDT, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
part 4 - Future.reject, Future.resolve (5.18 KB, patch)
2013-05-09 10:20 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 4 - Future.reject, Future.resolve (4.75 KB, patch)
2013-05-09 10:24 PDT, Andrea Marchesini (:baku)
mounir: review+
Details | Diff | Review
part 1 - WebIDL + constructor + Done (20.82 KB, patch)
2013-05-10 10:43 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 2 - then + catch (20.23 KB, patch)
2013-05-11 04:52 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 3 - Resolver.resolve(new Future(...)) (9.14 KB, patch)
2013-05-11 05:28 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 5 - pref (4.33 KB, patch)
2013-05-13 01:49 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 5 - pref (5.59 KB, patch)
2013-05-13 01:51 PDT, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
part 1 - WebIDL + constructor + Done (20.87 KB, patch)
2013-05-17 00:57 PDT, Andrea Marchesini (:baku)
mounir: review+
Details | Diff | Review
part 2 - then + catch (20.27 KB, patch)
2013-05-17 00:59 PDT, Andrea Marchesini (:baku)
mounir: review-
Details | Diff | Review
part 1 - WebIDL + constructor + Done (20.80 KB, patch)
2013-05-17 09:58 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 2 - then + catch (21.44 KB, patch)
2013-05-18 01:49 PDT, Andrea Marchesini (:baku)
mounir: review+
bzbarsky: review+
Details | Diff | Review
part 3 - Resolver.resolve(new Future(...)) (8.92 KB, patch)
2013-05-18 01:58 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 3 - Resolver.resolve(new Future(...)) (8.91 KB, patch)
2013-05-18 02:04 PDT, Andrea Marchesini (:baku)
mounir: review+
Details | Diff | Review
part 4 - Future.reject, Future.resolve (5.38 KB, patch)
2013-05-18 02:07 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 5 - pref (5.94 KB, patch)
2013-05-18 02:15 PDT, Andrea Marchesini (:baku)
mounir: review+
Details | Diff | Review
part 6 - tests (2.20 KB, patch)
2013-05-18 02:27 PDT, Andrea Marchesini (:baku)
mounir: review+
Details | Diff | Review
part 1 - WebIDL + constructor + Done (20.83 KB, patch)
2013-05-20 07:41 PDT, Andrea Marchesini (:baku)
bzbarsky: review+
Details | Diff | Review
part 3 - Resolver.resolve(new Future(...)) (9.98 KB, patch)
2013-05-20 07:42 PDT, Andrea Marchesini (:baku)
bzbarsky: review-
Details | Diff | Review
part 5 - pref (6.04 KB, patch)
2013-05-20 07:44 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 6 - tests (2.30 KB, patch)
2013-05-20 07:45 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 0 - ErrorResult::StealJSException (2.14 KB, patch)
2013-05-31 09:42 PDT, Andrea Marchesini (:baku)
bzbarsky: review+
Details | Diff | Review
part 1 - WebIDL + constructor + Done (22.13 KB, patch)
2013-05-31 09:47 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 1 - WebIDL + constructor + Done (22.24 KB, patch)
2013-05-31 09:48 PDT, Andrea Marchesini (:baku)
bzbarsky: review+
Details | Diff | Review
part 0 - ErrorResult::StealJSException (2.69 KB, patch)
2013-05-31 18:15 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 0 - ErrorResult::StealJSException (2.77 KB, patch)
2013-06-01 02:14 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 1 - WebIDL + constructor + Done (22.83 KB, patch)
2013-06-01 02:14 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 1 - WebIDL + constructor + Done (22.95 KB, patch)
2013-06-01 02:20 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 1 - WebIDL + constructor + Done (23.71 KB, patch)
2013-06-02 02:01 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 1 - WebIDL + constructor + Done (23.66 KB, patch)
2013-06-03 02:53 PDT, Andrea Marchesini (:baku)
bzbarsky: review+
Details | Diff | Review
part 2 - then + catch (25.13 KB, patch)
2013-06-03 02:55 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 0 - ErrorResult::StealJSException (2.77 KB, patch)
2013-06-03 02:55 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 2 - then + catch (26.27 KB, patch)
2013-06-03 03:17 PDT, Andrea Marchesini (:baku)
bugs: review+
Details | Diff | Review
part 3 - Resolver.resolve(new Future(...)) (9.76 KB, patch)
2013-06-03 03:18 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
interdiff - part 1 (9.66 KB, patch)
2013-06-03 08:20 PDT, Andrea Marchesini (:baku)
bzbarsky: review+
Details | Diff | Review
interdiff - part 2 (16.56 KB, patch)
2013-06-03 08:21 PDT, Andrea Marchesini (:baku)
bzbarsky: review-
Details | Diff | Review
interdiff - part 2 b (7.67 KB, patch)
2013-06-04 03:42 PDT, Andrea Marchesini (:baku)
bzbarsky: review-
Details | Diff | Review
interdiff - part 2 c (3.74 KB, patch)
2013-06-04 06:56 PDT, Andrea Marchesini (:baku)
bzbarsky: review+
Details | Diff | Review
part 3 - Resolver.resolve(new Future(...)) (9.22 KB, patch)
2013-06-04 07:03 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 2 - then + catch (27.10 KB, patch)
2013-06-04 17:19 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 3 - Resolver.resolve(new Future(...)) (9.05 KB, patch)
2013-06-04 17:24 PDT, Andrea Marchesini (:baku)
bzbarsky: review+
Details | Diff | Review
interdiff - part 2 (9.60 KB, patch)
2013-06-05 07:07 PDT, Andrea Marchesini (:baku)
bzbarsky: review+
Details | Diff | Review
interdiff for CC (1.71 KB, patch)
2013-06-11 09:06 PDT, Andrea Marchesini (:baku)
bugs: review+
Details | Diff | Review
part 1 - WebIDL + constructor + Done (23.75 KB, patch)
2013-06-11 09:08 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 1 - WebIDL + constructor + Done (23.75 KB, patch)
2013-06-11 09:52 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 2 - then + catch (27.12 KB, patch)
2013-06-11 09:53 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 4 - Future.reject, Future.resolve (5.45 KB, patch)
2013-06-11 09:55 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 5 - pref (5.99 KB, patch)
2013-06-11 09:56 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 6 - tests (2.16 KB, patch)
2013-06-11 09:57 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
part 5 - pref (6.53 KB, patch)
2013-06-12 06:41 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review

Description Anne (:annevk) 2013-03-31 06:03:23 PDT
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.
Comment 1 Anne (:annevk) 2013-03-31 06:05:04 PDT
*** Bug 839064 has been marked as a duplicate of this bug. ***
Comment 2 Olli Pettay [:smaug] 2013-03-31 08:04:32 PDT
Would be good to have some API which is using Futures and then implement Futures and that API.
Comment 3 Anne (:annevk) 2013-04-09 09:06:51 PDT
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.
Comment 4 Andrea Marchesini (:baku) 2013-04-10 06:48:35 PDT
Created attachment 735742 [details] [diff] [review]
WIP
Comment 5 Domenic Denicola 2013-04-10 07:10:52 PDT
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.
Comment 6 Andrea Marchesini (:baku) 2013-04-23 11:37:28 PDT
Created attachment 740949 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

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?
Comment 7 Boris Zbarsky [:bz] 2013-04-23 11:45:23 PDT
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.
Comment 8 Andrea Marchesini (:baku) 2013-04-23 12:21:02 PDT
Created attachment 740965 [details] [diff] [review]
part 2 - then + catch

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!
Comment 9 Domenic Denicola 2013-04-23 12:28:01 PDT
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/.
Comment 10 Domenic Denicola 2013-04-23 12:42:30 PDT
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
Comment 11 Andrea Marchesini (:baku) 2013-04-23 13:59:56 PDT
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?
Comment 12 Anne (:annevk) 2013-04-23 14:13:47 PDT
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.
Comment 13 Andrea Marchesini (:baku) 2013-04-23 22:54:33 PDT
Created attachment 741174 [details] [diff] [review]
part 0 - ErrorResult::StealJSException
Comment 14 Andrea Marchesini (:baku) 2013-04-23 23:27:47 PDT
Created attachment 741181 [details] [diff] [review]
part 1 - WebIDL + constructor + Done
Comment 15 Andrea Marchesini (:baku) 2013-04-23 23:31:26 PDT
Created attachment 741182 [details] [diff] [review]
part 2 - then + catch

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
Comment 16 Andrea Marchesini (:baku) 2013-04-24 06:02:15 PDT
Created attachment 741274 [details] [diff] [review]
part 3 - Resolver.resolve({then: ... })
Comment 17 Andrea Marchesini (:baku) 2013-04-24 06:03:59 PDT
Created attachment 741275 [details] [diff] [review]
part 2 - then + catch

mochitest should fully cover the implementation.
Comment 18 Andrea Marchesini (:baku) 2013-04-24 07:25:03 PDT
Created attachment 741305 [details] [diff] [review]
part 3 - Resolver.resolve({then: ... })

Better JSCompartment management.
Comment 19 Andrea Marchesini (:baku) 2013-04-26 03:12:40 PDT
Created attachment 742286 [details] [diff] [review]
part 4 - Future.accept, Future.reject, Future.resolve

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) {...
Comment 20 Mounir Lamouri (:mounir) 2013-05-07 07:09:11 PDT
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).
Comment 21 Mounir Lamouri (:mounir) 2013-05-07 07:18:49 PDT
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.
Comment 22 Mounir Lamouri (:mounir) 2013-05-07 07:22:09 PDT
(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.
Comment 23 David Bruant 2013-05-07 07:28:21 PDT
(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.
Comment 24 Domenic Denicola 2013-05-07 07:55:16 PDT
You need to be able to reject with arbitrary rejection reasons; restricting to `DOMError`s is very uncool.
Comment 25 Andrea Marchesini (:baku) 2013-05-07 07:58:51 PDT
> 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 26 Mounir Lamouri (:mounir) 2013-05-07 08:30:40 PDT
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();
Comment 27 Mounir Lamouri (:mounir) 2013-05-07 08:57:41 PDT
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?
Comment 28 Mounir Lamouri (:mounir) 2013-05-07 08:58:35 PDT
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.
Comment 29 Mounir Lamouri (:mounir) 2013-05-08 10:56:48 PDT
> 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.
Comment 30 Andrea Marchesini (:baku) 2013-05-09 08:54:41 PDT
Created attachment 747464 [details] [diff] [review]
part 1 - WebIDL + constructor + Done
Comment 31 Andrea Marchesini (:baku) 2013-05-09 08:55:55 PDT
Created attachment 747465 [details] [diff] [review]
part 2 - then + catch
Comment 32 Andrea Marchesini (:baku) 2013-05-09 10:20:08 PDT
Created attachment 747503 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))
Comment 33 Andrea Marchesini (:baku) 2013-05-09 10:20:50 PDT
Created attachment 747504 [details] [diff] [review]
part 4 - Future.reject, Future.resolve
Comment 34 Andrea Marchesini (:baku) 2013-05-09 10:24:05 PDT
Created attachment 747507 [details] [diff] [review]
part 4 - Future.reject, Future.resolve

there was a broken mochitest
Comment 35 Mounir Lamouri (:mounir) 2013-05-10 06:45:10 PDT
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.
Comment 36 Mounir Lamouri (:mounir) 2013-05-10 06:53:34 PDT
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.
Comment 37 Mounir Lamouri (:mounir) 2013-05-10 08:26:02 PDT
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();
Comment 38 Mounir Lamouri (:mounir) 2013-05-10 08:29:53 PDT
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");
});
Comment 39 Andrea Marchesini (:baku) 2013-05-10 10:43:16 PDT
Created attachment 748058 [details] [diff] [review]
part 1 - WebIDL + constructor + Done
Comment 40 Andrea Marchesini (:baku) 2013-05-11 04:52:41 PDT
Created attachment 748420 [details] [diff] [review]
part 2 - then + catch
Comment 41 Andrea Marchesini (:baku) 2013-05-11 05:28:41 PDT
Created attachment 748421 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

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 :)
Comment 42 Andrea Marchesini (:baku) 2013-05-13 01:49:31 PDT
Created attachment 748696 [details] [diff] [review]
part 5 - pref

Future behind prefs
Comment 43 Andrea Marchesini (:baku) 2013-05-13 01:51:41 PDT
Created attachment 748698 [details] [diff] [review]
part 5 - pref
Comment 44 Andrea Marchesini (:baku) 2013-05-17 00:57:43 PDT
Created attachment 750931 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

Using Futures in DataStore, I found that it's possible to create nasty loops.
Comment 45 Andrea Marchesini (:baku) 2013-05-17 00:59:21 PDT
Created attachment 750932 [details] [diff] [review]
part 2 - then + catch
Comment 46 Mounir Lamouri (:mounir) 2013-05-17 08:37:05 PDT
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?
Comment 47 Andrea Marchesini (:baku) 2013-05-17 09:58:30 PDT
Created attachment 751081 [details] [diff] [review]
part 1 - WebIDL + constructor + Done
Comment 48 Andrea Marchesini (:baku) 2013-05-17 09:59:47 PDT
I'll add the tests in a separated patch.
Comment 49 Mounir Lamouri (:mounir) 2013-05-17 12:02:34 PDT
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.
Comment 50 Mounir Lamouri (:mounir) 2013-05-17 12:07:05 PDT
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?
Comment 51 Mounir Lamouri (:mounir) 2013-05-17 12:12:46 PDT
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
Comment 52 Mounir Lamouri (:mounir) 2013-05-17 12:29:48 PDT
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.
Comment 53 Andrea Marchesini (:baku) 2013-05-18 01:49:07 PDT
Created attachment 751320 [details] [diff] [review]
part 2 - then + catch

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,
Comment 54 Andrea Marchesini (:baku) 2013-05-18 01:54:47 PDT
> 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);
Comment 55 Andrea Marchesini (:baku) 2013-05-18 01:58:42 PDT
Created attachment 751323 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

This patch is just rebased on top of the others.
Comment 56 Andrea Marchesini (:baku) 2013-05-18 02:04:20 PDT
Created attachment 751326 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))
Comment 57 Andrea Marchesini (:baku) 2013-05-18 02:07:28 PDT
Created attachment 751327 [details] [diff] [review]
part 4 - Future.reject, Future.resolve
Comment 58 Andrea Marchesini (:baku) 2013-05-18 02:15:46 PDT
Created attachment 751329 [details] [diff] [review]
part 5 - pref
Comment 59 Andrea Marchesini (:baku) 2013-05-18 02:27:20 PDT
Created attachment 751330 [details] [diff] [review]
part 6 - tests

Here additional tests.
Comment 60 Mounir Lamouri (:mounir) 2013-05-20 06:44:57 PDT
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.
Comment 61 Mounir Lamouri (:mounir) 2013-05-20 06:48:22 PDT
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 62 Mounir Lamouri (:mounir) 2013-05-20 06:58:14 PDT
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
Comment 63 Mounir Lamouri (:mounir) 2013-05-20 07:02:23 PDT
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");
Comment 64 Mounir Lamouri (:mounir) 2013-05-20 07:05:45 PDT
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.
Comment 65 Andrea Marchesini (:baku) 2013-05-20 07:41:22 PDT
Created attachment 751691 [details] [diff] [review]
part 1 - WebIDL + constructor + Done
Comment 66 Andrea Marchesini (:baku) 2013-05-20 07:42:42 PDT
Created attachment 751692 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))
Comment 67 Andrea Marchesini (:baku) 2013-05-20 07:44:21 PDT
Created attachment 751695 [details] [diff] [review]
part 5 - pref
Comment 68 Andrea Marchesini (:baku) 2013-05-20 07:45:39 PDT
Created attachment 751697 [details] [diff] [review]
part 6 - tests
Comment 69 Andrea Marchesini (:baku) 2013-05-20 09:35:38 PDT
https://tbpl.mozilla.org/?tree=Try&rev=fb51e5d391b1
Comment 70 Jonas Sicking (:sicking) 2013-05-28 15:20:32 PDT
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).
Comment 71 Boris Zbarsky [:bz] 2013-05-30 14:15:07 PDT
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.  ;)
Comment 72 Boris Zbarsky [:bz] 2013-05-30 22:07:54 PDT
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.
Comment 73 Boris Zbarsky [:bz] 2013-05-30 22:52:46 PDT
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.
Comment 74 Boris Zbarsky [:bz] 2013-05-30 23:08:44 PDT
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....
Comment 75 Boris Zbarsky [:bz] 2013-05-30 23:11:34 PDT
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...
Comment 76 Boris Zbarsky [:bz] 2013-05-31 01:07:34 PDT
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).
Comment 77 Andrea Marchesini (:baku) 2013-05-31 09:42:44 PDT
Created attachment 756610 [details] [diff] [review]
part 0 - ErrorResult::StealJSException
Comment 78 Andrea Marchesini (:baku) 2013-05-31 09:46:28 PDT
> 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.
Comment 79 Andrea Marchesini (:baku) 2013-05-31 09:47:40 PDT
Created attachment 756614 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

Interdiff... I forgot to generate it, sorry.
Comment 80 Andrea Marchesini (:baku) 2013-05-31 09:48:41 PDT
Created attachment 756615 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

sometimes, hg qref helps.
Comment 81 Boris Zbarsky [:bz] 2013-05-31 11:15:18 PDT
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.
Comment 82 Boris Zbarsky [:bz] 2013-05-31 11:49:32 PDT
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!
Comment 83 Andrea Marchesini (:baku) 2013-05-31 18:15:53 PDT
Created attachment 756894 [details] [diff] [review]
part 0 - ErrorResult::StealJSException
Comment 84 Boris Zbarsky [:bz] 2013-05-31 19:18:59 PDT
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".
Comment 85 Andrea Marchesini (:baku) 2013-06-01 02:14:01 PDT
Created attachment 756943 [details] [diff] [review]
part 0 - ErrorResult::StealJSException
Comment 86 Andrea Marchesini (:baku) 2013-06-01 02:14:30 PDT
Created attachment 756944 [details] [diff] [review]
part 1 - WebIDL + constructor + Done
Comment 87 Andrea Marchesini (:baku) 2013-06-01 02:19:36 PDT
> >+    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... ?
Comment 88 Andrea Marchesini (:baku) 2013-06-01 02:20:04 PDT
Created attachment 756945 [details] [diff] [review]
part 1 - WebIDL + constructor + Done
Comment 89 Boris Zbarsky [:bz] 2013-06-01 06:06:27 PDT
> 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?
Comment 90 Andrea Marchesini (:baku) 2013-06-02 02:01:59 PDT
Created attachment 757092 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

can you check this one-argument constructor?
For the rest I think I covered all your comments.
Comment 91 :Ms2ger 2013-06-02 02:12:06 PDT
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.
Comment 92 Andrea Marchesini (:baku) 2013-06-03 02:53:37 PDT
Created attachment 757301 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

Another review just for this one-argument param, please.
Comment 93 Andrea Marchesini (:baku) 2013-06-03 02:55:13 PDT
Created attachment 757303 [details] [diff] [review]
part 2 - then + catch
Comment 94 Andrea Marchesini (:baku) 2013-06-03 02:55:38 PDT
Created attachment 757304 [details] [diff] [review]
part 0 - ErrorResult::StealJSException
Comment 95 Andrea Marchesini (:baku) 2013-06-03 03:16:41 PDT
> 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.
Comment 96 Andrea Marchesini (:baku) 2013-06-03 03:17:27 PDT
Created attachment 757311 [details] [diff] [review]
part 2 - then + catch
Comment 97 Andrea Marchesini (:baku) 2013-06-03 03:18:05 PDT
Created attachment 757312 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))
Comment 98 Boris Zbarsky [:bz] 2013-06-03 08:06:13 PDT
> 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).
Comment 99 Olli Pettay [:smaug] 2013-06-03 08:13:09 PDT
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.
Comment 100 Andrea Marchesini (:baku) 2013-06-03 08:20:18 PDT
Created attachment 757423 [details] [diff] [review]
interdiff - part 1
Comment 101 Andrea Marchesini (:baku) 2013-06-03 08:21:01 PDT
Created attachment 757424 [details] [diff] [review]
interdiff - part 2
Comment 102 Boris Zbarsky [:bz] 2013-06-03 09:49:17 PDT
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
Comment 103 Boris Zbarsky [:bz] 2013-06-03 09:50:39 PDT
Comment on attachment 757301 [details] [diff] [review]
part 1 - WebIDL + constructor + Done

r=me
Comment 104 Boris Zbarsky [:bz] 2013-06-03 10:13:24 PDT
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".
Comment 105 Boris Zbarsky [:bz] 2013-06-03 10:14:29 PDT
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.
Comment 106 Sam Tobin-Hochstadt [:samth] 2013-06-04 00:48:05 PDT
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.
Comment 107 Andrea Marchesini (:baku) 2013-06-04 03:42:18 PDT
Created attachment 757876 [details] [diff] [review]
interdiff - part 2 b

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.
Comment 108 Andrea Marchesini (:baku) 2013-06-04 03:45:51 PDT
(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 109 Boris Zbarsky [:bz] 2013-06-04 05:57:54 PDT
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.
Comment 110 Boris Zbarsky [:bz] 2013-06-04 06:12:19 PDT
> So this implementation is right.

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

> Probably we will have several follow ups:

Sure.
Comment 111 Andrea Marchesini (:baku) 2013-06-04 06:56:29 PDT
Created attachment 757926 [details] [diff] [review]
interdiff - part 2 c
Comment 112 Andrea Marchesini (:baku) 2013-06-04 07:03:31 PDT
Created attachment 757929 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

I filed a follow up for the then-able objects in Resolver.resolve().
Comment 113 Boris Zbarsky [:bz] 2013-06-04 13:43:07 PDT
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"?
Comment 114 Andrea Marchesini (:baku) 2013-06-04 17:19:00 PDT
> 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.
Comment 115 Andrea Marchesini (:baku) 2013-06-04 17:19:57 PDT
Created attachment 758317 [details] [diff] [review]
part 2 - then + catch
Comment 116 Andrea Marchesini (:baku) 2013-06-04 17:24:20 PDT
Created attachment 758322 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

This patch (rebased on the latest changes) contains the output of the discussion with annevk.
Comment 117 Andrea Marchesini (:baku) 2013-06-05 07:07:50 PDT
Created attachment 758567 [details] [diff] [review]
interdiff - part 2

new interdiff for part 2.
Comment 118 Boris Zbarsky [:bz] 2013-06-06 00:01:36 PDT
Comment on attachment 758567 [details] [diff] [review]
interdiff - part 2

Thank you for the interdiff!

r=me
Comment 119 Boris Zbarsky [:bz] 2013-06-06 00:06:02 PDT
Comment on attachment 758322 [details] [diff] [review]
part 3 - Resolver.resolve(new Future(...))

r=me
Comment 120 Jonas Sicking (:sicking) 2013-06-06 23:09:15 PDT
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.
Comment 121 Boris Zbarsky [:bz] 2013-06-07 00:57:55 PDT
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...
Comment 122 David Bruant 2013-06-07 10:00:47 PDT
Some renaming recently occurred in agreement-land
https://github.com/whatwg/dom/commit/7a741b953990a89c6d27532928fe7817c3b25528
More context: http://infrequently.org/2013/06/sfuturepromiseg/
Comment 123 Andrea Marchesini (:baku) 2013-06-10 01:27:49 PDT
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.
Comment 124 Boris Zbarsky [:bz] 2013-06-10 13:58:38 PDT
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.
Comment 125 Andrea Marchesini (:baku) 2013-06-11 09:06:24 PDT
Created attachment 760968 [details] [diff] [review]
interdiff for CC
Comment 126 Andrea Marchesini (:baku) 2013-06-11 09:08:29 PDT
Created attachment 760970 [details] [diff] [review]
part 1 - WebIDL + constructor + Done
Comment 127 Andrea Marchesini (:baku) 2013-06-11 09:52:34 PDT
Created attachment 761003 [details] [diff] [review]
part 1 - WebIDL + constructor + Done
Comment 128 Andrea Marchesini (:baku) 2013-06-11 09:53:48 PDT
Created attachment 761004 [details] [diff] [review]
part 2 - then + catch
Comment 129 Andrea Marchesini (:baku) 2013-06-11 09:55:27 PDT
Created attachment 761005 [details] [diff] [review]
part 4 - Future.reject, Future.resolve

Any changes in the webIDL will be covered in a separated bug.
Comment 130 Andrea Marchesini (:baku) 2013-06-11 09:56:21 PDT
Created attachment 761006 [details] [diff] [review]
part 5 - pref
Comment 131 Andrea Marchesini (:baku) 2013-06-11 09:57:23 PDT
Created attachment 761007 [details] [diff] [review]
part 6 - tests
Comment 132 Andrea Marchesini (:baku) 2013-06-11 09:58:05 PDT
all the patches are rebased.
Comment 133 Andrea Marchesini (:baku) 2013-06-11 10:39:03 PDT
https://tbpl.mozilla.org/?tree=Try&rev=b20581b9efbb
Comment 134 Andrea Marchesini (:baku) 2013-06-11 13:04:40 PDT
Green on try.
Comment 136 Phil Ringnalda (:philor) 2013-06-11 21:27:02 PDT
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 137 :Ms2ger 2013-06-12 02:19:25 PDT
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.
Comment 138 Andrea Marchesini (:baku) 2013-06-12 03:05:06 PDT
https://tbpl.mozilla.org/?tree=Try&rev=e2c3656d649d
Comment 139 Andrea Marchesini (:baku) 2013-06-12 03:11:10 PDT
> 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.
Comment 140 Andrea Marchesini (:baku) 2013-06-12 06:41:48 PDT
Created attachment 761410 [details] [diff] [review]
part 5 - pref

Note You need to log in before you can comment on or make changes to this bug.