Bug 145223 – Refactor AudioContext implementation to enable automatic binding generation of promise-based methods

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

It would be good to have automatic generation of binding code for promise-based APIs.
A first step might be to refactor existing promise based APIs such as AudioContext, getUserMedia or ReadableStreamReader to align the custom binding code with what would be automatically generated.

I can see 3 approaches:
- Making resolve/reject promise callbacks take a single parameter, passed to the DeferredWrapper without any change.
- Beef-up the IDL promise to type the reject/resolve promise values
- Bite the bullet and make DeferredWrapper a first class citizen of WebCore (maybe renaming it then?). Use directly its API and remove callbacks, at least at binding level.

Option 1 is the natural step given the current code since DeferredWrapper is currently contained to custom binding code.

Option 2 is similar to option 1, a bit more complex at code generation probably, and extending the IDL language.
It would allow having better callback prototypes, like callbacks taking no parameters (or several) for instance.

Option 3 is probably the way to go in the end.
The binding code would be straightforward.
It would handle more nicely rejection of promise with different types (like exceptions or specific application values).
We would no longer be restricted by resolve/reject overloading rules (current reject(int) probably means reject(ExceptionCode)).

The downsides of option 3 compared to callback-based APIs is that callbacks are probably more reusable.
This may be useful (highly speculative) when implementing ReadableStream/MSE link for instance.

[youenn fablet]

Created attachment 253461 [details]
Example of AudioContext refactoring

[youenn fablet]

(In reply to comment #1)
> Created attachment 253461 [details]
> Example of AudioContext refactoring

This refactoring illustrates option 1.

[youenn fablet]

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?

[youenn fablet]

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?

[youenn fablet]

Created attachment 254482 [details]
DOMPromise option

[WebKit Commit Bot]

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.

[Geoffrey Garen]

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?

[youenn fablet]

> > +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.

[Darin Adler]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 254632 [details]
Patch for landing (removed DOMPromise std::nullptr_t specialization)

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.