Closed Bug 882076 Opened 6 years ago Closed 6 years ago

Allow Promise to invoke C++ handlers on resolution

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: baku, Assigned: nsm)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Currently it's hard to connect an object to a Promise chain. The purpose of this bug is to make it easier.
Attached patch patch (obsolete) — Splinter Review
Attachment #761351 - Flags: review?(mounir)
I'm going to use this patch for DataStore: bug 871445
No longer depends on: promises
Depends on: promises
(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.
That meant to be a joke Andrea ;)
Blocks: 839838
Depends on: 884754
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.
Attachment #761351 - Flags: review?(mounir)
Blocks: 885333
Blocks: 910387
Attached patch patch (obsolete) — Splinter Review
Bug 911213 is needed for this patch.
Attachment #761351 - Attachment is obsolete: true
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?
Attachment #798536 - Flags: review?(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?
Attachment #798536 - Flags: review?(mounir) → feedback?(amarchesini)
Mounir... I wrote this patch :) I think this patch is beautiful!
(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.
(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. :)
Attachment #798536 - Flags: feedback?(amarchesini)
Andrea, are you still using this patch? I will need something similar for ServiceWorkers and it has to be in C++.
Flags: needinfo?(amarchesini)
No longer blocks: 910387
No longer blocks: 910412
(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.
Flags: needinfo?(amarchesini)
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.
Renamed Runnable to NativeHandler to avoid confusion with nsRunnables.

baku for overall review.
mccr8 for cycle collection bits.
Attachment #8333622 - Flags: review?(continuation)
Attachment #8333622 - Flags: review?(amarchesini)
Assignee: amarchesini → nsm.nikhil
Summary: Make Promise usable from C++ → Allow Promise to invoke C++ handlers on resolution
Comment on attachment 8333622 [details] [diff] [review]
C++ callbacks to DOM Promises.

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

r=me for CC
Attachment #8333622 - Flags: review?(continuation) → review+
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;

>>&
Attachment #8333622 - Flags: review?(amarchesini) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/a6bb8146e7bc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.