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.
Created attachment 288812 [details] Patch
Created attachment 288817 [details] Adding binding test
(In reply to comment #2) > Created attachment 288817 [details] > Adding binding test Patch only updates getUserMedia SubtleCrypto should follow with the same routines.
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?
Created attachment 288933 [details] Patch for landing
Comment on attachment 288933 [details] Patch for landing Clearing flags on attachment: 288933 Committed r205953: <http://trac.webkit.org/changeset/205953>
All reviewed patches have been landed. Closing bug.
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