Closed
Bug 882076
Opened 10 years ago
Closed 9 years ago
Allow Promise to invoke C++ handlers on resolution
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: baku, Assigned: nsm)
References
(Blocks 1 open bug)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 2 obsolete files)
8.79 KB,
patch
|
baku
:
review+
mccr8
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Attachment #761351 -
Flags: review?(mounir)
Reporter | ||
Comment 2•10 years ago
|
||
I'm going to use this patch for DataStore: bug 871445
Comment 3•10 years ago
|
||
You mean http://en.cppreference.com/w/cpp/thread/promise ? :)
Reporter | ||
Comment 4•10 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.
Comment 5•10 years ago
|
||
That meant to be a joke Andrea ;)
Comment 6•10 years ago
|
||
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 | ||
Comment 7•10 years ago
|
||
Bug 911213 is needed for this patch.
Attachment #761351 -
Attachment is obsolete: true
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
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•10 years ago
|
||
Mounir... I wrote this patch :) I think this patch is beautiful!
Comment 11•10 years ago
|
||
(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•10 years ago
|
||
(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•9 years ago
|
Attachment #798536 -
Flags: feedback?(amarchesini)
Assignee | ||
Comment 13•9 years ago
|
||
Andrea, are you still using this patch? I will need something similar for ServiceWorkers and it has to be in C++.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(amarchesini)
Reporter | ||
Comment 14•9 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)
Assignee | ||
Comment 15•9 years ago
|
||
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: NavigationController
Assignee | ||
Comment 16•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: amarchesini → nsm.nikhil
Assignee | ||
Updated•9 years ago
|
Summary: Make Promise usable from C++ → Allow Promise to invoke C++ handlers on resolution
Assignee | ||
Updated•9 years ago
|
Attachment #798536 -
Attachment is obsolete: true
Comment 17•9 years ago
|
||
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•9 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+
Assignee | ||
Comment 19•9 years ago
|
||
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
Assignee | ||
Comment 20•9 years ago
|
||
(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•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a6bb8146e7bc
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•9 years ago
|
Whiteboard: [qa-]
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•