Bug 146200 – MediaDevices.getUserMedia should migrate from callbacks to DOMPromise

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

[youenn fablet]

DOMPromise is introduced to simplify Promise-based APIs.
MediaDevices.getUserMedia could use it.
Since webkitGetUserMedia is a client of MediaDevices.getUserMedia and uses a typed callback approach, DOMPromise could be accommodated to also enable callback style.
This might be beneficial for any Promise API clients: Promise/Callback APIs, ReadableStream::tee or MSE appendStream.

[youenn fablet]

Created attachment 255347 [details]
Patch

[WebKit Commit Bot]

Attachment 255347 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:91:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 255347 [details]
Patch

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

> Source/WebCore/Modules/mediastream/MediaDevices.h:59
> +    typedef DOMPromiseWithCallback<RefPtr<MediaStream>, RefPtr<NavigatorUserMediaError>> Promise;

Why are the two arguments to this RefPtr instead of Ref or a plain old reference (MediaStream&)?

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:67
> +        ScriptExecutionContext* context = navigator->frame()->document();
> +        ASSERT(context);

Instead of capturing a pointer to the navigator, I think we should capture a pointer to the document, since the only thing we do with the navigator is get to its document.

The assertion isn’t that helpful (why can we assert the document is not null later?) and using ScriptExecutionContext as the type for the local variable doesn’t seem great. We should just write:

    RefPtr<Document> protectedDocument = navigator->frame()->document();

And then capture protectedDocument instead of navigator.

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:69
> +        RefPtr<MediaStream> mediaStream = stream;

protectedStream, not mediaStream (see my comment in the other bug about this)

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:79
> +            RefPtr<NavigatorUserMediaError> eventError = error;

protectedError, not eventError (see my comment in the other bug about this)

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:86
> +    RefPtr<UserMediaRequest> request = UserMediaRequest::create(navigator->frame()->document(), userMedia, options, MediaDevices::Promise(WTF::move(resolveCallback), WTF::move(rejectCallback)), ec);

I think auto would be better here than RefPtr<UserMediaRequest>.

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:88
>          ec = NOT_SUPPORTED_ERR;

Is this right? UserMediaRequest might fail and set "ec", then we overwrite the more specific ec with just NOT_SUPPORTED_ERR? Do we have test coverage?

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:56
> +// Declarations for resolving/rejecting promises

Not a helpful comment, I don’t think.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:58
> +JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, MediaStream*);
> +JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, NavigatorUserMediaError*);

Could make the arguments references instead of pointers, unless these actually need to be null.

I don’t understand why these functions are declared here. If they are local to this file they should be marked "static" to get internal linkage, or use an anonymous namespace to accomplish the same thing. But if they are used outside this file, then the declarations should be in a header.

> Source/WebCore/bindings/js/JSDOMPromise.h:88
> +template <typename Value, typename Error>

I typically omit the space after "template".

> Source/WebCore/bindings/js/JSDOMPromise.h:91
> +    DOMPromiseWithCallback(DeferredWrapper&& wrapper) : m_wrapper(WTF::move(wrapper)) { }

Probably should mark this explicit.

> Source/WebCore/bindings/js/JSDOMPromise.h:92
> +    DOMPromiseWithCallback(std::function<void(const Value&)>&& resolve, std::function<void(const Error&)>&& reject)

Since std::function is a move-only type, these arguments could just be std::function; no real need for them to be std:function&&.

> Source/WebCore/bindings/js/JSDOMPromise.h:99
> +    DOMPromiseWithCallback(DOMPromiseWithCallback&& promise)

Probably should mark this explicit.

> Source/WebCore/bindings/js/JSDOMPromise.h:105
> +    DOMPromiseWithCallback(const DOMPromiseWithCallback&)= delete;
> +    DOMPromiseWithCallback& operator=(DOMPromiseWithCallback const&) = delete;

Since the class already has data members of move only type (std::function) there won’t be a copy constructor created, just a move constructor. So deleting here is unnecessary. Please omit these lines of code.

> Source/WebCore/bindings/js/JSDOMPromise.h:226
> +template <typename Value, typename Error>

I typically omit the space after "template".

> Source/WebCore/bindings/js/JSDOMPromise.h:227
> +void DOMPromiseWithCallback<Value, Error>::resolve(const Value& value)

This simple function possibly should be marked inline.

> Source/WebCore/bindings/js/JSDOMPromise.h:237
> +template <typename Value, typename Error>

I typically omit the space after "template".

> Source/WebCore/bindings/js/JSDOMPromise.h:238
> +void DOMPromiseWithCallback<Value, Error>::reject(const Error& error)

This simple function possibly should be marked inline.

[youenn fablet]

Comment on attachment 255347 [details]
Patch

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

>> Source/WebCore/Modules/mediastream/MediaDevices.h:59
>> +    typedef DOMPromiseWithCallback<RefPtr<MediaStream>, RefPtr<NavigatorUserMediaError>> Promise;
> 
> Why are the two arguments to this RefPtr instead of Ref or a plain old reference (MediaStream&)?

Thanks for the review.
Yes, a Ref should be used.

>> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:67
>> +        ASSERT(context);
> 
> Instead of capturing a pointer to the navigator, I think we should capture a pointer to the document, since the only thing we do with the navigator is get to its document.
> 
> The assertion isn’t that helpful (why can we assert the document is not null later?) and using ScriptExecutionContext as the type for the local variable doesn’t seem great. We should just write:
> 
>     RefPtr<Document> protectedDocument = navigator->frame()->document();
> 
> And then capture protectedDocument instead of navigator.

OK

>> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:69
>> +        RefPtr<MediaStream> mediaStream = stream;
> 
> protectedStream, not mediaStream (see my comment in the other bug about this)

OK

>> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:88
>>          ec = NOT_SUPPORTED_ERR;
> 
> Is this right? UserMediaRequest might fail and set "ec", then we overwrite the more specific ec with just NOT_SUPPORTED_ERR? Do we have test coverage?

This patch does not change that behavior.
The current behavior is probably spec conformant. Butit would probably just be better to set ec to NOT_SUPPORTED_ERR within UserMediaRequest::create.

@adam, any thought on that?

>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:58
>> +JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, NavigatorUserMediaError*);
> 
> Could make the arguments references instead of pointers, unless these actually need to be null.
> 
> I don’t understand why these functions are declared here. If they are local to this file they should be marked "static" to get internal linkage, or use an anonymous namespace to accomplish the same thing. But if they are used outside this file, then the declarations should be in a header.

These routines are defined in automatically generated files (JSXXX.h).
I will add the necessary "JSMediaStream.h" and "JSNavigatorUserMediaError.h" #include.
It seems a bit unfortunate to include these files just for these standalone routines.

>> Source/WebCore/bindings/js/JSDOMPromise.h:88
>> +template <typename Value, typename Error>
> 
> I typically omit the space after "template".

OK

>> Source/WebCore/bindings/js/JSDOMPromise.h:91
>> +    DOMPromiseWithCallback(DeferredWrapper&& wrapper) : m_wrapper(WTF::move(wrapper)) { }
> 
> Probably should mark this explicit.

I am not sure, JSMediaDevices.getUserMedia expects a DOMPromiseWithCallback but the JSMediaDevices automatically generated code gives it a DeferredWrapper.

>> Source/WebCore/bindings/js/JSDOMPromise.h:92
>> +    DOMPromiseWithCallback(std::function<void(const Value&)>&& resolve, std::function<void(const Error&)>&& reject)
> 
> Since std::function is a move-only type, these arguments could just be std::function; no real need for them to be std:function&&.

OK

>> Source/WebCore/bindings/js/JSDOMPromise.h:99
>> +    DOMPromiseWithCallback(DOMPromiseWithCallback&& promise)
> 
> Probably should mark this explicit.

OK

>> Source/WebCore/bindings/js/JSDOMPromise.h:105
>> +    DOMPromiseWithCallback& operator=(DOMPromiseWithCallback const&) = delete;
> 
> Since the class already has data members of move only type (std::function) there won’t be a copy constructor created, just a move constructor. So deleting here is unnecessary. Please omit these lines of code.

OK

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 255347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255347&action=review
> 
> > Source/WebCore/Modules/mediastream/MediaDevices.h:59
> > +    typedef DOMPromiseWithCallback<RefPtr<MediaStream>, RefPtr<NavigatorUserMediaError>> Promise;
> 
> Why are the two arguments to this RefPtr instead of Ref or a plain old
> reference (MediaStream&)?
> 
> > Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:67
> > +        ScriptExecutionContext* context = navigator->frame()->document();
> > +        ASSERT(context);
> 
> Instead of capturing a pointer to the navigator, I think we should capture a
> pointer to the document, since the only thing we do with the navigator is
> get to its document.
> 
> The assertion isn’t that helpful (why can we assert the document is not null
> later?) and using ScriptExecutionContext as the type for the local variable
> doesn’t seem great. We should just write:
> 
>     RefPtr<Document> protectedDocument = navigator->frame()->document();
> 
> And then capture protectedDocument instead of navigator.

I am not sure that we should protect the document here.
UserMediaRequest is observing context destruction and will cancel itself if that happens.
I plan to update the patch by capturing an unprotected document.

[Adam Bergkvist]

Comment on attachment 255347 [details]
Patch

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

> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:78
> +        if (errorCallback) {

The errorCallback is mandatory nowadays so this check isn't necessary. Blame me for not updating the code in UserMediaRequest and the idl file. :/ Let's create an issue on what's left of that mistake when this lands.

>>> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:88
>>>          ec = NOT_SUPPORTED_ERR;
>> 
>> Is this right? UserMediaRequest might fail and set "ec", then we overwrite the more specific ec with just NOT_SUPPORTED_ERR? Do we have test coverage?
> 
> This patch does not change that behavior.
> The current behavior is probably spec conformant. Butit would probably just be better to set ec to NOT_SUPPORTED_ERR within UserMediaRequest::create.
> 
> @adam, any thought on that?

NOT_SUPPORTED_ERR (neither audio nor video is requested) is the only possible error at this point. I agree that we can do this code better. I noted that ec is set to TYPE_MISMATCH_ERR a few calls down the line, but that isn't, and shouldn't, be surfaced.

[youenn fablet]

(In reply to comment #6)
> Comment on attachment 255347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=255347&action=review
> 
> > Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:78
> > +        if (errorCallback) {
> 
> The errorCallback is mandatory nowadays so this check isn't necessary. Blame
> me for not updating the code in UserMediaRequest and the idl file. :/ Let's
> create an issue on what's left of that mistake when this lands.
> 
> >>> Source/WebCore/Modules/mediastream/NavigatorUserMedia.cpp:88
> >>>          ec = NOT_SUPPORTED_ERR;
> >> 
> >> Is this right? UserMediaRequest might fail and set "ec", then we overwrite the more specific ec with just NOT_SUPPORTED_ERR? Do we have test coverage?
> > 
> > This patch does not change that behavior.
> > The current behavior is probably spec conformant. Butit would probably just be better to set ec to NOT_SUPPORTED_ERR within UserMediaRequest::create.
> > 
> > @adam, any thought on that?
> 
> NOT_SUPPORTED_ERR (neither audio nor video is requested) is the only
> possible error at this point. I agree that we can do this code better. I
> noted that ec is set to TYPE_MISMATCH_ERR a few calls down the line, but
> that isn't, and shouldn't, be surfaced.

Most of this code is actually shared between NavigatorUserMedia::webkitGetUserMedia and MediaDevices::getUserMedia.
Except callbacks, it might be shared within UserMediaRequest::create, which could be renamed UserMediaRequest::start().

@adam, let me know if you would like to look at it first.
Otherwise, I can have a quick stab at this refactoring after landing this one.

[Adam Bergkvist]

Feel free to take a stab at it.

[youenn fablet]

Created attachment 255409 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 255409 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMPromise.h:91:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to comment #9)
> Created attachment 255409 [details]
> Patch for landing

Missing improvement pieces:
- Using Ref<> within DOMPromiseWithCallback and not RefPtr<> (It probably requires modifying JSDOMBinding.h)
- Refactoring to share code between webkitGetUserMedia and MediaDevices.getUserMedia
- Making errorCallback parameter mandatory at the IDL level

I plan to tackle the first two points.

[WebKit Commit Bot]

Comment on attachment 255409 [details]
Patch for landing

Clearing flags on attachment: 255409

Committed r185873: <http://trac.webkit.org/changeset/185873>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #5)
> I am not sure that we should protect the document here.
> UserMediaRequest is observing context destruction and will cancel itself if
> that happens.
> I plan to update the patch by capturing an unprotected document.

What happens to the promise in the case? How airtight is the guarantee that the lambdas won’t ever be called in that case? I am usually quite reluctant to rely on high level guarantees for object lifetime; it’s best to have the lifetime guarantees be as local and as airtight as possible.

[youenn fablet]

(In reply to comment #14)
> (In reply to comment #5)
> > I am not sure that we should protect the document here.
> > UserMediaRequest is observing context destruction and will cancel itself if
> > that happens.
> > I plan to update the patch by capturing an unprotected document.
> 
> What happens to the promise in the case? How airtight is the guarantee that
> the lambdas won’t ever be called in that case? I am usually quite reluctant
> to rely on high level guarantees for object lifetime; it’s best to have the
> lifetime guarantees be as local and as airtight as possible.

For each of the 3 possible methods that reject/resolve promises, the context status is checked first.

If we capture a Document reference within the callbacks, the document will not be destroyed as long as the request is pending.

A solution might be to create a context destruction observer that gets captured by the lambdas. This is safer but is also redundant with what does UserMediaRequest. Note that this observer could wrap the postTask functionality.

As of pending promises in case of destroyed document, cancelling the UserMediaRequest should lead to releasing the request from below, thus releasing the promise and its strong references.

We could clear the promise within cancel.
In the 3 reject/resolve promises methods, the context status check would be replaced by a check on the promise (was it cleared or not.).
Same behavior as currently but maybe more readable.