Closed Bug 839838 Opened 11 years ago Closed 10 years ago

Add .then() method to DOMRequest

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mounir, Assigned: ehsan.akhgari)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [doc: see comment 28])

Attachments

(1 file, 4 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
Still a few stuff to figure out:
- 'success' error should be send before or after the chaining (callbacks at all)?
- for the moment, when an error happen, the only way to stop calling errors callbacks is to return a DOMRequest (Alex's API doesn't match that behaviour);
- there are still some leaks :(
Attachment #712219 - Flags: review?(jonas)
Comment on attachment 712219 [details] [diff] [review]
Patch

Talked with mounir about this and I think we agreed to wait an see where DOMFuture goes before making a move here.
Attachment #712219 - Flags: review?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #1)
> Talked with mounir about this and I think we agreed to wait an see where
> DOMFuture goes before making a move here.
That's a fair decision.
What are the particular points you're waiting on?
Is there any way to help move things forward on the spec side?
Depends on: promises
Should this be closed as WONTFIX in favour of Futures?
It might still be good to add to ease migration.
I believe that if we end up implementing something to ease migration, we should probably have something like:

interface DOMRequest : Future {
  readonly attribute any       result;
  readonly attribute DOMError  error;
  readonly attribute DOMString status; (or whatever the same is)

  + events
};

It would be safer and easier than improving DOMRequest to be compatible with Future.
Depends on: 875289, 882076
Depends on: 884754
Un-assigning myself because my experiment might unlikely be what we will be doing. I think that a better solution to implement this would be to make DOMRequest inherits from Promises C++ implementation and add GetResult() GetError() and GetStatus() methods and the event handling in the sub-class. That should be easier than adding .then() and all the other Promises properties to DOMRequest and more future-friendly.
Assignee: mounir → nobody
I would be very unusual to add event handling in the subclass.
EventTarget is in general the first interface in the prototype chain.
But perhaps you're talking about C++ side only.

Could we wrap a promise object inside DOMRequest, and then just forward method calls to the inner
object?
(In reply to Olli Pettay [:smaug] from comment #7)
> Could we wrap a promise object inside DOMRequest, and then just forward
> method calls to the inner object?
I wouldn't recommand that approach. The long term goal would be to get rid of DOMRequest. I drafted a plan at https://groups.google.com/d/msg/mozilla.dev.webapi/4O_ffcuLjdE/e77aUBlhv_IJ
Note a couple of messages later Jérémie Patonnier's idea to make using Promise methods/props instead of DOMRequest a Marketplace criteria to accept an app (something that can't be done on the web :-) )
I'd be interested to know if the inclusion of thenables in the promise spec affects this bug in any way by simplifying it. Can DOMRequest just act as a thenable?

David, I'd appreciate it if you could summarize the conclusions derived from here and the mailing list, since all the suggestions seem to be 'maybe we could do this' and not conclusive (I'm not saying thats a bad thing.)

As prickly as this issue is, DOMRequests suck IMHO and if we can get rid of them faster, that's awesome!
Flags: needinfo?(bruant.d)
(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #9)
> I'd be interested to know if the inclusion of thenables in the promise spec
> affects this bug in any way by simplifying it. Can DOMRequest just act as a
> thenable?
I don't think it affects this bug and the most likely scenario so far which is DOMRequest inheriting from Promise.

> David, I'd appreciate it if you could summarize the conclusions derived from
> here and the mailing list, since all the suggestions seem to be 'maybe we
> could do this' and not conclusive (I'm not saying thats a bad thing.)
I don't think there is a conclusion yet. I proposed a plan in the mailing-list (see comment #8), Jérémie Patonnier seemed to agree and I haven't seen oppositions yet.
Of course, this plan requires coordination from several teams (dev-engage for the hacks posts, marketplace for the extra review work, engineering to make DOMRequest inherit from Promise, doc to make the doc changes), so that's some work. I'm happy to do the coordination, MozHacks and doc part, but I'd need a signal that engineering and Marketplace are in.
Needinfo'ing smaug for the engineering part (unless you can make that call, Nikhil?).
Who's the right person to contact on the marketplace/app-review side?
Flags: needinfo?(bruant.d) → needinfo?(bugs)
(In reply to David Bruant from comment #10)
> Who's the right person to contact on the marketplace/app-review side?
needinfo'ing Lisa Brewster since she's listed as Apps reviews Lead in https://wiki.mozilla.org/Marketplace/Reviewers/Apps
Lisa: as an attempt to get rid of DOMRequest, one thing that could help would be to reject apps that do use DOMRequest (an easy alternative will be provided and documented). Do you think it's possible to include such a criteria when it comes to rejecting apps eventually?
Flags: needinfo?(adora)
I don't know what info you're asking. But anyhow, how could https://groups.google.com/forum/#!msg/mozilla.dev.webapi/4O_ffcuLjdE/e77aUBlhv_IJ work? DOMRequest inherits EventTarget. DOMRequest inheriting EventTarget and Promise at the same time? We don't have multiple inheritance.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #12)
> I don't know what info you're asking.
More or less whether this bug is something that Gecko folks are interested in doing and how.

> But anyhow, how could
> https://groups.google.com/forum/#!msg/mozilla.dev.webapi/4O_ffcuLjdE/
> e77aUBlhv_IJ work? DOMRequest inherits EventTarget. DOMRequest inheriting
> EventTarget and Promise at the same time? We don't have multiple inheritance.
An earlier version of Promises did inherit from EventTarget [1]. But of course that's not the case anymore...
Alright. Adding a single .then method it is, I guess?
Support of thenables will indeed be of help.

[1] https://github.com/slightlyoff/Promises/blob/84c03cb0ecf40b1c6dd7edb1604b0ede2fbf3c35/DOMFuture.idl#L259
Adding Andrew (app review tech lead) and Mark (owner of the app validator who can hopefully enforce this request during app submission instead of wasting our reviewers' precious human brains :)
Flags: needinfo?(adora)
I can add the then().
This then() will need to have the same semantics as Promise.then (returned value of onFulfill is cast to a Promise).

I am not aware of the __proto__ intricacies involved, so it will be fantastic if David can write a test suite.

Note that DOM Promises don't support subclassing.
Assignee: nobody → nsm.nikhil
(In reply to David Bruant from comment #11)
> (In reply to David Bruant from comment #10)
> > Who's the right person to contact on the marketplace/app-review side?
> needinfo'ing Lisa Brewster since she's listed as Apps reviews Lead in
> https://wiki.mozilla.org/Marketplace/Reviewers/Apps
> Lisa: as an attempt to get rid of DOMRequest, one thing that could help
> would be to reject apps that do use DOMRequest (an easy alternative will be
> provided and documented). Do you think it's possible to include such a
> criteria when it comes to rejecting apps eventually?

Rejecting apps would be an extreme step if the DOMRequest API is still valid and available and I can only see it being appropriate if there is a security or particularly bad performance issue. What's the issue with DOMRequest exactly?

We could only apply such a rule to packaged apps btw, the code for hosted apps isn't validated (and could change at any point anyway)
And would make it more difficult (even if possible) to do apps for different FxOS versions.
We should not use DOMRequest for anything new. It might make sense to add a then() method on it (returning a promise) to make transition easier, but other than that I don't think we should do much about it for now. DOMRequest itself is not really at fault, it's the APIs that emit it.
(In reply to Andrew Williamson [:eviljeff] from comment #16)
> Rejecting apps would be an extreme step if the DOMRequest API is still valid
> and available and I can only see it being appropriate if there is a security
> or particularly bad performance issue. What's the issue with DOMRequest
> exactly?
It's a bad API that composes poorly with other APIs. I don't think there'll be security issues. There is no particularly bad performance issues with DOMRequest per se, but it encourages a bad way of programming asynchronously (callback-based) which leads to poor programming with async stuffs which leads to perf issues. It's indirect, but it can matter in big enough applications. Arguably, code using DOMRequest is harder to maintain, maybe to an extent that hurts perf and security.
But I understand it might not be good enough a reason to reject apps with DOMRequest.

> We could only apply such a rule to packaged apps btw, the code for hosted
> apps isn't validated (and could change at any point anyway)
Of course. But I think no non-privileged APIs uses DOMRequest, I'll need to check.


(In reply to Julien Wajsberg [:julienw] from comment #17)
> And would make it more difficult (even if possible) to do apps for different
> FxOS versions.
Mozilla could ship a polyfill so that in old FirefoxOS phones, methods returning DOMRequest returns Promises instead.
Platform fragmentation is a problem we know how to solve ;-)
I think once the changes have landed in a branch (fx1.4?) we could add a validation warning that DOMRequest is deprecated and they should do XXX instead.  Then during the code review for privileged apps the reviewer could feed it back to the developer.

If, at some future point, old FxOS versions are polyfilled as suggested, and its still coming up as an issue we can revisit if rejecting is necessary - but once the docs, APIs, template apps ,etc are updated I'd imagine most developers will use the new method anyway, especially if its as bad to use as you say.
I think we should add a .then() function to DOMRequest and then do nothing more for now.

Most of the APIs that we have that return DOMRequests are in need of getting specced and cleaned up anyway. So those APIs will likely change in backwards incompatible ways at that time. Forcing people to switch from DOMRequest to Promise but otherwise keep the current API is mainly busy work with little value otherwise.

By adding DOMRequest.then() we enable developers that want to use the better Promise patterns to do so. But we don't force people to update their code if they don't care.

DOMRequest.then() should return a Promise though. And obviously should conform to the Promise spec's definition of the .then() function.
Whiteboard: [mentor=nsm.nikhil@gmail.com][lang=c++]
Assignee: nsm.nikhil → nobody
Mentor: nsm.nikhil
Whiteboard: [mentor=nsm.nikhil@gmail.com][lang=c++] → [lang=c++]
Assignee: nobody → ehsan.akhgari
This is implemented by creating a Promise object internally and
forwarding the .then() call to it. Any further callbacks passed to
future .then() calls will be added as callbacks on the same internal
promise object. We also take care of resolving or rejecting the promise
if the success/error event of the DOMRequest object has been fired
before .then() is called.
Attachment #8496432 - Flags: review?(jonas)
Mentor: nsm.nikhil
Whiteboard: [lang=c++]
Attachment #712219 - Attachment is obsolete: true
Julian pointed out an important problem. Sometimes we use DOMRequest as a cursor. See specifically nsIDOMRequestService.createCursor and DOMCursor.

The easiest solution seems to be to make DOMCursors simply not have a .then() function. So perhaps make DOMCursor not subclass DOMRequest but instead copy its interface?
(In reply to Jonas Sicking (:sicking) from comment #23)
> Julian pointed out an important problem. Sometimes we use DOMRequest as a
> cursor. See specifically nsIDOMRequestService.createCursor and DOMCursor.
> 
> The easiest solution seems to be to make DOMCursors simply not have a
> .then() function. So perhaps make DOMCursor not subclass DOMRequest but
> instead copy its interface?

Sure.  I'll use a shared interface to avoid the copying though.
This is implemented by creating a Promise object internally and
forwarding the .then() call to it. Any further callbacks passed to
future .then() calls will be added as callbacks on the same internal
promise object. We also take care of resolving or rejecting the promise
if the success/error event of the DOMRequest object has been fired
before .then() is called.
Attachment #8496432 - Attachment is obsolete: true
Attachment #8496432 - Flags: review?(jonas)
Attachment #8496454 - Flags: review?(jonas)
Comment on attachment 8496454 [details] [diff] [review]
Implement DOMRequest.then; r=sicking

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

Drive by review.

::: dom/base/DOMRequest.cpp
@@ +119,5 @@
>  
>    FireEvent(NS_LITERAL_STRING("success"), false, false);
> +
> +  if (mPromise) {
> +    AutoSafeJSContext cx;

DOMRequest may be used on workers (IDB), so either use ThreadsafeAutoSafeJSContext, or AutoJSAPI, both of which do the right thing.

@@ +121,5 @@
> +
> +  if (mPromise) {
> +    AutoSafeJSContext cx;
> +    JS::Rooted<JS::Value> result(cx, mResult);
> +    mPromise->MaybeResolve(result);

Since you have a JS::Value, you could use the 2 arg form of MaybeResolve(cx, jsval) to avoid conversion attempts. You will have to be sure the value is in the correct compartment (in this case it should be).

@@ +220,5 @@
> +  if (aRv.Failed()) {
> +    return nullptr;
> +  }
> +
> +  if (mDone) {

Not sure why you are handling this manually. If the promise is already resolved/rejected, it will deal with then() callbacks correctly.
Comment on attachment 8496454 [details] [diff] [review]
Implement DOMRequest.then; r=sicking

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

Thanks for the drive-by!

::: dom/base/DOMRequest.cpp
@@ +220,5 @@
> +  if (aRv.Failed()) {
> +    return nullptr;
> +  }
> +
> +  if (mDone) {

Note that I do this on mPromise, not the newly created promise.  This is where we would resolve/reject mPromise itself.  I rely on what you're saying to take care of things for the newly created promise.
Attachment #8496454 - Flags: review?(jonas)
This is implemented by creating a Promise object internally and
forwarding the .then() call to it. Any further callbacks passed to
future .then() calls will be added as callbacks on the same internal
promise object. We also take care of resolving or rejecting the promise
if the success/error event of the DOMRequest object has been fired
before .then() is called.
Attachment #8497837 - Flags: review?(jonas)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #27)
> Comment on attachment 8496454 [details] [diff] [review]
> Implement DOMRequest.then; r=sicking
> 
> Review of attachment 8496454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the drive-by!
> 
> ::: dom/base/DOMRequest.cpp
> @@ +220,5 @@
> > +  if (aRv.Failed()) {
> > +    return nullptr;
> > +  }
> > +
> > +  if (mDone) {
> 
> Note that I do this on mPromise, not the newly created promise.  This is
> where we would resolve/reject mPromise itself.  I rely on what you're saying
> to take care of things for the newly created promise.

Makes sense. Could you add a comment before landing? it isn't entirely obvious. I think what you are saying is 'mPromise' is only set to a valid Promise when somebody expresses interest for the first time by calling then(). At that point, the DOMRequest might already be 'resolved', so you need to 'bring the promise in sync' with the state of the DOMRequest. In that case, couldn't you move this block to inside the |if (!mPromise)| check?
Comment on attachment 8497837 [details] [diff] [review]
Implement DOMRequest.then; r=sicking

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

::: dom/base/DOMRequest.cpp
@@ +119,5 @@
>  
>    FireEvent(NS_LITERAL_STRING("success"), false, false);
> +
> +  if (mPromise) {
> +    AutoSafeJSContext cx;

Did you miss this? :)
Comment on attachment 8496454 [details] [diff] [review]
Implement DOMRequest.then; r=sicking

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

r- for the context thing.

::: dom/base/DOMRequest.cpp
@@ +119,5 @@
>  
>    FireEvent(NS_LITERAL_STRING("success"), false, false);
> +
> +  if (mPromise) {
> +    AutoSafeJSContext cx;

Why don't you need to grab the right JS context here?

@@ +220,5 @@
> +  if (aRv.Failed()) {
> +    return nullptr;
> +  }
> +
> +  if (mDone) {

Alternatively don't create the promise lazily. I'm fine either way.
Keywords: dev-doc-needed
Whiteboard: [doc: see comment 28]
Attachment #8496454 - Attachment is obsolete: true
This is implemented by creating a Promise object internally and
forwarding the .then() call to it. Any further callbacks passed to
future .then() calls will be added as callbacks on the same internal
promise object. We also take care of resolving or rejecting the promise
if the success/error event of the DOMRequest object has been fired
before .then() is called.
Attachment #8497837 - Attachment is obsolete: true
Attachment #8497837 - Flags: review?(jonas)
Comment on attachment 8499571 [details] [diff] [review]
Implement DOMRequest.then; r=sicking

Addressed the review comments.  I kept the creation of the promise lazy, I think with the comments that I added things are more clear.
Attachment #8499571 - Flags: review?(jonas)
Comment on attachment 8499571 [details] [diff] [review]
Implement DOMRequest.then; r=sicking

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

r=me for the other pieces.

::: dom/base/DOMRequest.cpp
@@ +121,5 @@
> +
> +  if (mPromise) {
> +    ThreadsafeAutoJSContext cx;
> +    JS::Rooted<JS::Value> result(cx, mResult);
> +    mPromise->MaybeResolve(cx, result);

Someone that knows these things better than me should review the JSContext stuff here.
Attachment #8499571 - Flags: review?(jonas) → review+
Comment on attachment 8499571 [details] [diff] [review]
Implement DOMRequest.then; r=sicking

(In reply to Jonas Sicking (:sicking) from comment #34)
> ::: dom/base/DOMRequest.cpp
> @@ +121,5 @@
> > +
> > +  if (mPromise) {
> > +    ThreadsafeAutoJSContext cx;
> > +    JS::Rooted<JS::Value> result(cx, mResult);
> > +    mPromise->MaybeResolve(cx, result);
> 
> Someone that knows these things better than me should review the JSContext
> stuff here.

Boris, can you review those bits please?  Thanks!
Attachment #8499571 - Flags: review?(bzbarsky)
Comment on attachment 8499571 [details] [diff] [review]
Implement DOMRequest.then; r=sicking

>+  mResult = JSVAL_VOID;

  mResult.setUndefined();

But why do you need to do that at all?

>+    ThreadsafeAutoJSContext cx;

No, please don't.  This should work fine:

  mPromise->MaybeResolve(result);

>+        JS::Rooted<JS::Value> result(aCx, mResult);

Likewise, please just call the single-argument version of MaybeResolve.  The two-argument one would involve you wrapping mResult into the compartment of aCx and all that jazz, which you don't want to deal with.

>+  nsRefPtr<Promise> promise = mPromise->Then(aCx, aResolveCallback,
>+                                             aRejectCallback, aRv);

Why not just:

  return mPromise->Then(aCx, aResolveCallback, aRejectCallback, aRv);

?

>-    AutoSafeJSContext cx;
>+    ThreadsafeAutoJSContext cx;

This is changing behavior for mainthread.  Please use ThreadsafeAutoSafeJSContext for now, I guess.

Rejecting things with DOMError is kinda weird, since it's not an Error subtype, but I guess it's preexisting.  Is there value in filing a bug on making it be one, more or less?  Specifically:

1)  Mark it as [ExceptionClass]
2)  Mark its name/message as [LenientThis]
3)  Add a readonly [Replaceable] "stack" property (that returns empty string, I
    guess).
4)  Add an overload of single-argument MaybeReject that takes a const
    nsRefPtr<DOMError>&.
5)  Remove the DOMError overload of MaybeRejectBrokenly.
Attachment #8499571 - Flags: review?(bzbarsky) → review+
We should remove DOMError from Gecko and replace its use with DOMException. But obviously not in this bug.

Bz, do you think making sure that we reject with an Error is something that's blocking this bug?
(In reply to Boris Zbarsky [:bz] from comment #36)
> Comment on attachment 8499571 [details] [diff] [review]
> Implement DOMRequest.then; r=sicking
> 
> >+  mResult = JSVAL_VOID;
> 
>   mResult.setUndefined();
> 
> But why do you need to do that at all?

I'm just moving this code from the header to the cpp...

> >+    ThreadsafeAutoJSContext cx;
> 
> No, please don't.  This should work fine:
> 
>   mPromise->MaybeResolve(result);
> 
> >+        JS::Rooted<JS::Value> result(aCx, mResult);
> 
> Likewise, please just call the single-argument version of MaybeResolve.  The
> two-argument one would involve you wrapping mResult into the compartment of
> aCx and all that jazz, which you don't want to deal with.

I don't understand what you're suggesting exactly.  The single argument version of MaybeResolve only works for stuff that ToJSValue supports, right?  mResult is a JS::Heap<JS::Value> and AFAICT that is not supported by ToJSValue.  Also, that single argument version seems to just call MaybeSomething which does all of what you say we should avoid here, so I'm puzzled.
Flags: needinfo?(bzbarsky)
> Bz, do you think making sure that we reject with an Error is something that's blocking
> this bug?

Absolutely not.  That's why I asked a question instead of saying to fix it.  ;)

> mResult is a JS::Heap<JS::Value> and AFAICT that is not supported by ToJSValue.

Ah, I see.  We have a ToJSValue overload for JS::Handle<JS::Value> but not for Heap<Value> or Rooted<Value>.  We should just add those, so you can MaybeResolve(mResult).  They'll look like a copy/paste of http://hg.mozilla.org/mozilla-central/file/4bad24a306b2/dom/bindings/ToJSValue.h#l231 but with a different type for aArgument.

> which does all of what you say we should avoid here

Not quite.  It does things like enter the right compartments, wrap things into those compartments, etc.  Things that you can't easily do in this code, actually.
Flags: needinfo?(bzbarsky)
See Also: → 1080883
(In reply to Wes Kocher (:KWierso) from comment #42)
> This also broke Mnw tests:
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=2866750&repo=mozilla-inbound

This was caused by <http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/head.js?rev=34dbc25304b9#1320>.

Now that DOMRequest is a thenable, this promise will be resolved to the result of the DOMRequest.

Filed bug 1080883 to fix the API design.
(In reply to Jan de Mooij [:jandem] from comment #45)
> Sorry I backed this out for rooting analysis bustage:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/16a8a5c8b96a
> 
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> inbound-linux64-br-haz/20141011064725/hazards.txt.gz
> 
> I looked at it briefly but I didn't see an obviously-correct fix.

Hmm.  Boris, it seems like passing a JS::Heap to MaybeResolve is not the right thing to do.  What should I do instead?
Flags: needinfo?(bzbarsky)
Pass a JS::Heap reference?

For that matter, you don't want to pass JS::Rooted by value either; passing it by reference should be ok.
Flags: needinfo?(bzbarsky)
https://hg.mozilla.org/mozilla-central/rev/a14b8bf72b16
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1084381
Blocks: 1084389
Blocks: 1085199
Blocks: 1087319
Blocks: 1087321
Blocks: 1087324
You need to log in before you can comment on or make changes to this bug.