Allow Promise to invoke C++ handlers on resolution

RESOLVED FIXED in mozilla28

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: baku, Assigned: nsm)

Tracking

(Blocks: 1 bug)

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Currently it's hard to connect an object to a Promise chain. The purpose of this bug is to make it easier.
(Reporter)

Comment 1

5 years ago
Created attachment 761351 [details] [diff] [review]
patch
Attachment #761351 - Flags: review?(mounir)
(Reporter)

Comment 2

5 years ago
I'm going to use this patch for DataStore: bug 871445
(Reporter)

Updated

5 years ago
No longer depends on: 856410
(Reporter)

Updated

5 years ago
Depends on: 856410
(Reporter)

Comment 4

5 years ago
(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 ;)
(Reporter)

Updated

5 years ago
Blocks: 839838
(Reporter)

Updated

5 years ago
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)
(Reporter)

Updated

5 years ago
Blocks: 885333
(Reporter)

Updated

5 years ago
Blocks: 910387
(Reporter)

Comment 7

5 years ago
Created attachment 798536 [details] [diff] [review]
patch

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)
(Reporter)

Comment 10

5 years ago
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. :)
(Reporter)

Updated

5 years ago
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
(Reporter)

Comment 14

5 years ago
(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.
Blocks: 898524
Created attachment 8333622 [details] [diff] [review]
C++ callbacks to DOM Promises.

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
Attachment #798536 - Attachment is obsolete: true
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+
(Reporter)

Comment 18

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.