Bug 161961 – callPromiseFunction should be made usable for custom binding code

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

[youenn fablet]

Custom binding code is not always handling promise correctly as can be seen from SubtleCrypto and getUserMedia, especially the handling of exceptions.
Having a wrapper function would be useful for these.

[youenn fablet]

Created attachment 288812 [details]
Patch

[youenn fablet]

Created attachment 288817 [details]
Adding binding test

[youenn fablet]

(In reply to comment #2)
> Created attachment 288817 [details]
> Adding binding test

Patch only updates getUserMedia
SubtleCrypto should follow with the same routines.

[Darin Adler]

Comment on attachment 288817 [details]
Adding binding test

View in context: https://bugs.webkit.org/attachment.cgi?id=288817&action=review

> Source/WebCore/bindings/js/JSDOMPromise.h:162
> +template<PromiseFunction promiseFunction, bool canBeCalledInWorker = false>

The use of a boolean here probably makes this hard to read at call sites. Is there a better way do this? We can use our typical enum approach, for example.

> Source/WebCore/bindings/js/JSDOMPromise.h:189
> +template<BindingPromiseFunction bindingPromiseFunction, bool canBeCalledInWorker>

Ditto.

> Source/WebCore/bindings/js/JSMediaDevicesCustom.cpp:386
> +    return callPromiseFunction<JSMediaDevicesGetUserMediaPromiseFunction, false>(state);

The "false" here is completely unclear.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3414
>  static EncodedJSValue ${functionName}Promise(ExecState*, Ref<DeferredWrapper>&&);
>  ${functionReturn} ${functionName}(ExecState* state)

I’d like to see a blank line between these two lines.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3420
>  static inline EncodedJSValue ${functionName}Promise(ExecState* state, Ref<DeferredWrapper>&&  deferredWrapper)

There is an extra space here after the "&&".

> LayoutTests/fast/mediastream/MediaDevices-getUserMedia.html:140
> +                testPassed(" navigator.mediaDevices.getUserMedia() rejected with error: " + error);

Why the leading space here?

> LayoutTests/fast/mediastream/MediaDevices-getUserMedia.html:143
>                  testPassed(" navigator.mediaDevices.getUserMedia({}) rejected with error: " + error);

Why the leading space here?

[youenn fablet]

Created attachment 288933 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 288933 [details]
Patch for landing

Clearing flags on attachment: 288933

Committed r205953: <http://trac.webkit.org/changeset/205953>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Thanks for the review.

(In reply to comment #4)
> Comment on attachment 288817 [details]
> Adding binding test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288817&action=review
> 
> > Source/WebCore/bindings/js/JSDOMPromise.h:162
> > +template<PromiseFunction promiseFunction, bool canBeCalledInWorker = false>
> 
> The use of a boolean here probably makes this hard to read at call sites. Is
> there a better way do this? We can use our typical enum approach, for
> example.

Done with a PromiseExecutionScope enum class.

> The "false" here is completely unclear.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3414
> >  static EncodedJSValue ${functionName}Promise(ExecState*, Ref<DeferredWrapper>&&);
> >  ${functionReturn} ${functionName}(ExecState* state)
> 
> I’d like to see a blank line between these two lines.

Done

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3420
> >  static inline EncodedJSValue ${functionName}Promise(ExecState* state, Ref<DeferredWrapper>&&  deferredWrapper)
> 
> There is an extra space here after the "&&".

Done

> > LayoutTests/fast/mediastream/MediaDevices-getUserMedia.html:140
> > +                testPassed(" navigator.mediaDevices.getUserMedia() rejected with error: " + error);
> 
> Why the leading space here?
> 
> > LayoutTests/fast/mediastream/MediaDevices-getUserMedia.html:143
> >                  testPassed(" navigator.mediaDevices.getUserMedia({}) rejected with error: " + error);
> 
> Why the leading space here?

Fixed