Summary: | Refactor AudioContext implementation to enable automatic binding generation of promise-based methods | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, darin, ggaren, jer.noble, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
youenn fablet
2015-05-20 14:14:19 PDT
Created attachment 253461 [details]
Example of AudioContext refactoring
(In reply to comment #1) > Created attachment 253461 [details] > Example of AudioContext refactoring This refactoring illustrates option 1. Checking Mozilla and Blink code, they went with option 3, which gets the code closer to the spec. As said before, this has some benefit like rejecting with exception codes or other specific values, resolving with no value by calling resolve()... To be noted also that the ReadableStream implementation will need a way to call "then" on JS promise values, as well as promises created from WebCore APIs. webkitGetUserMedia is already a consumer of getUserMedia promise. Option 3 would solve the potential issue of callbacks that are sometimes called synchronously sometimes asynchronously. Any thoughts? Checking Mozilla and Blink code, they went with option 3, which gets the code closer to the spec. As said before, this has some benefit like rejecting with exception codes or other specific values, resolving with no value by calling resolve()... To be noted also that the ReadableStream implementation will need a way to call "then" on JS promise values, as well as promises created from WebCore APIs. webkitGetUserMedia is already a consumer of getUserMedia promise. Option 3 would solve the potential issue of callbacks that are sometimes called synchronously sometimes asynchronously. Any thoughts? Created attachment 254482 [details]
DOMPromise option
Attachment 254482 [details] did not pass style-queue:
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:64: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:78: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
Total errors found: 2 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 254482 [details] DOMPromise option View in context: https://bugs.webkit.org/attachment.cgi?id=254482&action=review > Source/WebCore/bindings/js/JSDOMPromise.h:84 > +template <typename Value, typename Error> > +class DOMPromise { > +public: > + DOMPromise(DeferredWrapper&& wrapper) : m_wrapper(WTF::move(wrapper)) { } > + > + DOMPromise(const DOMPromise&)= delete; > + DOMPromise& operator=(DOMPromise const&) = delete; > + > + void resolve(const Value& value) { m_wrapper.resolve<Value>(value); } > + void reject(const Error& error) { m_wrapper.reject<Error>(error); } > +private: > + DeferredWrapper m_wrapper; > +}; > + > +template <typename Error> > +class DOMPromise<std::nullptr_t, Error> { > +public: > + DOMPromise(DeferredWrapper&& wrapper) : m_wrapper(WTF::move(wrapper)) { } > + > + void resolve() { m_wrapper.resolve<std::nullptr_t>(nullptr); } > + void reject(const Error& error) { m_wrapper.reject<Error>(error); } > +private: > + DeferredWrapper m_wrapper; > +}; I don't think this template specialization adds much. I guess the point is to be able to call resolve() without supplying an argument? But couldn't the caller supply nullptr? > > +template <typename Error>
> > +class DOMPromise<std::nullptr_t, Error> {
> > +public:
> > + DOMPromise(DeferredWrapper&& wrapper) : m_wrapper(WTF::move(wrapper)) { }
> > +
> > + void resolve() { m_wrapper.resolve<std::nullptr_t>(nullptr); }
> > + void reject(const Error& error) { m_wrapper.reject<Error>(error); }
> > +private:
> > + DeferredWrapper m_wrapper;
> > +};
>
> I don't think this template specialization adds much. I guess the point is
> to be able to call resolve() without supplying an argument? But couldn't the
> caller supply nullptr?
Depending on its template type, DOMPromise<V,E>::resolve(nullptr) may resolve with jsUndefined() or jsNull().
This specialization (that may be used for ReadableStreamReader::closed) disambiguates the two cases. It also makes the C++ code a bit closer to JS equivalent code.
Hence my slight preference to keep it.
Fact is also that most promises are resolved with values passed by references and not pointers. This makes the jsNull() case not probable in practice.
So, if you still prefer to remove it, I am also fine going that way.
Comment on attachment 254482 [details] DOMPromise option View in context: https://bugs.webkit.org/attachment.cgi?id=254482&action=review > Source/WebCore/bindings/js/JSDOMPromise.cpp:64 > + // FIXME: We shoud clear m_globalObject and m_deferred here. Would be nice to say why. > Source/WebCore/bindings/js/JSDOMPromise.h:53 > + void callFunction(JSC::ExecState*, JSC::JSValue function, JSC::JSValue resolution); Consider changing these all to take JSC::ExecState& and naming the state as "state" instead of "exec"? > Source/WebCore/bindings/js/JSDOMPromise.h:54 > + void resolve(JSC::ExecState*exec, JSC::JSValue resolution) { callFunction(exec, m_deferred->resolve(), resolution); } Missing space before "exec". > Source/WebCore/bindings/js/JSDOMPromise.h:71 > +private: Please leave a blank line before "private". > Source/WebCore/bindings/js/JSDOMPromise.h:82 > +private: Please leave a blank line before "private". >> Source/WebCore/bindings/js/JSDOMPromise.h:84 >> +}; > > I don't think this template specialization adds much. I guess the point is to be able to call resolve() without supplying an argument? But couldn't the caller supply nullptr? I agree with Geoff. Thanks for the reviews. (In reply to comment #9) > Comment on attachment 254482 [details] > DOMPromise option > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254482&action=review > > > Source/WebCore/bindings/js/JSDOMPromise.cpp:64 > > + // FIXME: We shoud clear m_globalObject and m_deferred here. > > Would be nice to say why. > > > Source/WebCore/bindings/js/JSDOMPromise.h:53 > > + void callFunction(JSC::ExecState*, JSC::JSValue function, JSC::JSValue resolution); > > Consider changing these all to take JSC::ExecState& and naming the state as > "state" instead of "exec"? OK. I guess globalExec should also be renamed globalExecState then. > > Source/WebCore/bindings/js/JSDOMPromise.h:54 > > + void resolve(JSC::ExecState*exec, JSC::JSValue resolution) { callFunction(exec, m_deferred->resolve(), resolution); } > > Missing space before "exec". OK > > > Source/WebCore/bindings/js/JSDOMPromise.h:71 > > +private: > > Please leave a blank line before "private". > > > Source/WebCore/bindings/js/JSDOMPromise.h:82 > > +private: > > Please leave a blank line before "private". OK > > >> Source/WebCore/bindings/js/JSDOMPromise.h:84 > >> +}; > > > > I don't think this template specialization adds much. I guess the point is to be able to call resolve() without supplying an argument? But couldn't the caller supply nullptr? > > I agree with Geoff. I'll remove it then. Created attachment 254632 [details]
Patch for landing (removed DOMPromise std::nullptr_t specialization)
Comment on attachment 254632 [details] Patch for landing (removed DOMPromise std::nullptr_t specialization) Clearing flags on attachment: 254632 Committed r185407: <http://trac.webkit.org/changeset/185407> All reviewed patches have been landed. Closing bug. |