882076 - Allow Promise to invoke C++ handlers on resolution

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Currently it's hard to connect an object to a Promise chain. The purpose of this bug is to make it easier.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Andrea Marchesini [:baku]]

I'm going to use this patch for DataStore: bug 871445

Comment 3

[Mounir Lamouri (:mounir)]

You mean http://en.cppreference.com/w/cpp/thread/promise ? :)

Comment 4

[Andrea Marchesini [:baku]]

(In reply to Mounir Lamouri (:mounir) from comment #3)
> You mean http://en.cppreference.com/w/cpp/thread/promise ? :)

hehe, not really. I tried to write a cpp unit test but it's hard because nsString cannot be used in that kind of unit tests and replacing nsString.h with nsStringGlue.h makes the patch huge and complex.

Comment 5

[Mounir Lamouri (:mounir)]

That meant to be a joke Andrea ;)

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 761351 [details] [diff] [review]
patch

Seems like this is mostly needed for DataStore implementation but we do not know yet if the implementation will end up being done in C++ or JS so delaying the review until we have a clear story.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

Bug 911213 is needed for this patch.

Comment 8

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 798536 [details] [diff] [review]
patch

Andrea, thanks.

Mounir, FileSystem API will be implemented by C++ and use Promise. Could you start reviewing the patch or help to find any other solution for C++ to use Promise?

Comment 9

[Mounir Lamouri (:mounir)]

Comment on attachment 798536 [details] [diff] [review]
patch

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

Andrea, could you give a feedback regarding that patch?

Comment 10

[Andrea Marchesini [:baku]]

Mounir... I wrote this patch :) I think this patch is beautiful!

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to Andrea Marchesini (:baku) from comment #10)
> Mounir... I wrote this patch :) I think this patch is beautiful!

The fact that the review came from Yuan was confusing, sorry.

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Mounir Lamouri (:mounir) from comment #11)
> (In reply to Andrea Marchesini (:baku) from comment #10)
> > Mounir... I wrote this patch :) I think this patch is beautiful!
> 
> The fact that the review came from Yuan was confusing, sorry.
It's my fault. Andrea wrote the patch, I thougth he forgot to set the review flag and set it for him. :)

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Andrea, are you still using this patch? I will need something similar for ServiceWorkers and it has to be in C++.

Comment 14

[Andrea Marchesini [:baku]]

(In reply to Nikhil Marathe [:nsm] from comment #13)
> Andrea, are you still using this patch? I will need something similar for
> ServiceWorkers and it has to be in C++.

No, I'm not using this patch. But if you need it, it can be a good starting point.

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

The various ServiceWorkerScope events are passed Promises which affect the implementation in C++. This patch is necessary to add handlers for when the Promises are resolved/rejected by content JS.

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: C++ callbacks to DOM Promises.

Renamed Runnable to NativeHandler to avoid confusion with nsRunnables.

baku for overall review.
mccr8 for cycle collection bits.

Comment 17

[Andrew McCreight [:mccr8]]

Comment on attachment 8333622 [details] [diff] [review]
C++ callbacks to DOM Promises.

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

r=me for CC

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8333622 [details] [diff] [review]
C++ callbacks to DOM Promises.

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

lgtm

::: dom/promise/Promise.cpp
@@ +486,5 @@
>  void
> +Promise::AppendNativeHandler(PromiseNativeHandler* aRunnable)
> +{
> +  nsRefPtr<PromiseCallback> resolveCb =
> +  new NativePromiseCallback(aRunnable, Resolved);

<space><space>new

@@ +489,5 @@
> +  nsRefPtr<PromiseCallback> resolveCb =
> +  new NativePromiseCallback(aRunnable, Resolved);
> +
> +  nsRefPtr<PromiseCallback> rejectCb =
> +  new NativePromiseCallback(aRunnable, Rejected);

ditto

::: dom/promise/PromiseCallback.cpp
@@ +257,5 @@
> +  MOZ_COUNT_DTOR(NativePromiseCallback);
> +}
> +
> +void
> +NativePromiseCallback::Call(const Optional<JS::Handle<JS::Value> >& aValue)

>>&

::: dom/promise/PromiseCallback.h
@@ +118,5 @@
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(NativePromiseCallback,
> +                                           PromiseCallback)
> +
> +  void Call(const Optional<JS::Handle<JS::Value> >& aValue) MOZ_OVERRIDE;

>>&

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Sorry, but I pushed and forgot to fix the nits. Will fix them in Bug 939322.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6bb8146e7bc

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Nikhil Marathe [:nsm] from comment #19)
> Sorry, but I pushed and forgot to fix the nits. Will fix them in Bug 939322.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a6bb8146e7bc

I mean Bug 939332

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6bb8146e7bc