Bug 145282

Created attachment 253541 [details] Patch

Comment on attachment 253541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253541&action=review > Source/WebCore/Modules/mediastream/MediaDevices.h:59 > + typedef std::function<void(NavigatorUserMediaError*)> RejectCallback; Can we resolve/reject with nullptr for getUserMedia? I guess it is not the case but I am not sure. We may want to pass parameters as references (& or Ref<>&&) if possible, probably once PassRefPtr<>, RefPtr<>, Ref<> cleaning is done. > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:172 > + RefPtr<MediaStream> stream = MediaStream::create(*m_scriptExecutionContext, privateStream); Can stream be null? Can we use a Ref<> instead? > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:187 > RefPtr<NavigatorUserMediaError> error = NavigatorUserMediaError::create(NavigatorUserMediaError::constraintNotSatisfiedErrorName(), constraintName); Can error be null? Ca we use a Ref<> instead? > LayoutTests/fast/mediastream/MediaDevices-getUserMedia-expected.txt:-8 > -PASS navigator.mediaDevices.webkitGetUserMedia({audio:true}); threw exception TypeError: navigator.mediaDevices.webkitGetUserMedia is not a function. (In 'navigator.mediaDevices.webkitGetUserMedia({audio:true})', 'navigator.mediaDevices.webkitGetUserMedia' is undefined). This last test case is strange. Is it what we want to test? I changed it to "navigator.mediaDevices.getUserMedia({audivide:true})"

Comment on attachment 253541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253541&action=review > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:83 > + errorCallback->handleEvent(error); With the proposed changes, the rejectCallback may be called synchronously now. The promise implementation ensures that JS callbacks are called asynchronously. But for webkitGetUserMedia, after looking at http://w3c.github.io/mediacapture-main/getusermedia.html#navigatorusermedia-interface-extensions, this is probably an issue. We may emulate asynchronous promise callback execution with something like: RefPtr<NavigatorUserMediaError> protectedError = error; scriptExecutionContext()->postTask([errorCallback, protectedError] { errorCallback->handleEvent(protectedError.get()); }); Or think of a better way to do the same thing (handling at promise level just like the spec says). successCallback might need to have the same treatment for consistency purposes. Also, this might need a dedicated test case, since it seems the tests do not capture that. Rethinking about it, splitting the patch in two might make sense. I will wait for some feedback before doing anything, maybe I am overlooking things.

(In reply to comment #2) > Comment on attachment 253541 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253541&action=review > > > Source/WebCore/Modules/mediastream/MediaDevices.h:59 > > + typedef std::function<void(NavigatorUserMediaError*)> RejectCallback; > > Can we resolve/reject with nullptr for getUserMedia? > I guess it is not the case but I am not sure. > We may want to pass parameters as references (& or Ref<>&&) if possible, > probably once PassRefPtr<>, RefPtr<>, Ref<> cleaning is done. No. Always a MediaSteram or an error. > > > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:172 > > + RefPtr<MediaStream> stream = MediaStream::create(*m_scriptExecutionContext, privateStream); > > Can stream be null? Can we use a Ref<> instead? > > > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:187 > > RefPtr<NavigatorUserMediaError> error = NavigatorUserMediaError::create(NavigatorUserMediaError::constraintNotSatisfiedErrorName(), constraintName); > > Can error be null? Ca we use a Ref<> instead? > I believe the answer is no to both questions above. > > LayoutTests/fast/mediastream/MediaDevices-getUserMedia-expected.txt:-8 > > -PASS navigator.mediaDevices.webkitGetUserMedia({audio:true}); threw exception TypeError: navigator.mediaDevices.webkitGetUserMedia is not a function. (In 'navigator.mediaDevices.webkitGetUserMedia({audio:true})', 'navigator.mediaDevices.webkitGetUserMedia' is undefined). > > This last test case is strange. Is it what we want to test? > I changed it to "navigator.mediaDevices.getUserMedia({audivide:true})" MediaDevices.getUserMedia() is a rather late addition to the Media Capture and Streams spec [1]. It's sibling, on Navigator, has been along for quite some time, so when we decided to expose the Promised-based version on the MediaDevices object, we said that implementations should not vendor prefix it since the functionality is rather mature. So this test is intended to check that. [1] https://w3c.github.io/mediacapture-main/

> > Can error be null? Ca we use a Ref<> instead? > > > > I believe the answer is no to both questions above. I realize my answer was a bit ambiguous. :) It can't be null, and yes, we could use Ref<> instead.

(In reply to comment #3) > Comment on attachment 253541 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253541&action=review > > > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:83 > > + errorCallback->handleEvent(error); > > With the proposed changes, the rejectCallback may be called synchronously > now. > The promise implementation ensures that JS callbacks are called > asynchronously. > But for webkitGetUserMedia, after looking at > http://w3c.github.io/mediacapture-main/getusermedia.html#navigatorusermedia- > interface-extensions, this is probably an issue. > > We may emulate asynchronous promise callback execution with something like: > RefPtr<NavigatorUserMediaError> protectedError = error; > scriptExecutionContext()->postTask([errorCallback, protectedError] { > errorCallback->handleEvent(protectedError.get()); > }); > Or think of a better way to do the same thing (handling at promise level > just like the spec says). > > successCallback might need to have the same treatment for consistency > purposes. The invocations of resolveCallback and rejectCallback in UserMediaRequest.cpp (:172, 192, 205) are wrapped in callOnMainThread(). Does that address your concern?

Comment on attachment 253541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=253541&action=review >>> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:83 >>> + errorCallback->handleEvent(error); >> >> With the proposed changes, the rejectCallback may be called synchronously now. >> The promise implementation ensures that JS callbacks are called asynchronously. >> But for webkitGetUserMedia, after looking at http://w3c.github.io/mediacapture-main/getusermedia.html#navigatorusermedia-interface-extensions, this is probably an issue. >> >> We may emulate asynchronous promise callback execution with something like: >> RefPtr<NavigatorUserMediaError> protectedError = error; >> scriptExecutionContext()->postTask([errorCallback, protectedError] { >> errorCallback->handleEvent(protectedError.get()); >> }); >> Or think of a better way to do the same thing (handling at promise level just like the spec says). >> >> successCallback might need to have the same treatment for consistency purposes. >> >> Also, this might need a dedicated test case, since it seems the tests do not capture that. >> Rethinking about it, splitting the patch in two might make sense. >> >> I will wait for some feedback before doing anything, maybe I am overlooking things. > > The invocations of resolveCallback and rejectCallback in UserMediaRequest.cpp (:172, 192, 205) are wrapped in callOnMainThread(). Does that address your concern? Yes and no ;) Using callOnMainThread would solve the concern for webkitGetUserMedia (although postTask would be better as it is what promise implementation uses for calling JS callbacks). But it creates an issue for MediaDevices.getUserMedia() as the postTask/callOnMainThread call is not needed at all to resolve/reject promise. The JS promise implementation is already doing that work. This makes things less efficient and less consistent. I will split that as a separate patch since it is less straightforward than I thought it would be. Let's first reject exceptions. >>> LayoutTests/fast/mediastream/MediaDevices-getUserMedia-expected.txt:-8 >>> -PASS navigator.mediaDevices.webkitGetUserMedia({audio:true}); threw exception TypeError: navigator.mediaDevices.webkitGetUserMedia is not a function. (In 'navigator.mediaDevices.webkitGetUserMedia({audio:true})', 'navigator.mediaDevices.webkitGetUserMedia' is undefined). >> >> This last test case is strange. Is it what we want to test? >> I changed it to "navigator.mediaDevices.getUserMedia({audivide:true})" > > MediaDevices.getUserMedia() is a rather late addition to the Media Capture and Streams spec [1]. It's sibling, on Navigator, has been along for quite some time, so when we decided to expose the Promised-based version on the MediaDevices object, we said that implementations should not vendor prefix it since the functionality is rather mature. So this test is intended to check that. > > [1] https://w3c.github.io/mediacapture-main/ OK. Asserting that navigator.mediaDevices.webkitGetUserMedia is undefined might be clearer then?

Created attachment 253586 [details] Removing async promise resolution/rejection changes

> > The invocations of resolveCallback and rejectCallback in UserMediaRequest.cpp (:172, 192, 205) are wrapped in callOnMainThread(). Does that address your concern? > > Yes and no ;) > Using callOnMainThread would solve the concern for webkitGetUserMedia > (although postTask would be better as it is what promise implementation uses > for calling JS callbacks). > > But it creates an issue for MediaDevices.getUserMedia() as the > postTask/callOnMainThread call is not needed at all to resolve/reject > promise. > The JS promise implementation is already doing that work. > This makes things less efficient and less consistent. You can see an illustration of the issue in the -expected.txt generated by the latest patch. The order of the callback execution is not really what we would expect. First, all promise rejection callbacks are called asynchronously which is good. But the empty dictionary test is displayed before the {audio:true} test for instance. Reason is the custom binding is implemented with just one postTask. The rejection done in UserMediaRequest is implemented with one callOnnMainThread followed by one postTask.

(In reply to comment #7) > OK. > Asserting that navigator.mediaDevices.webkitGetUserMedia is undefined might > be clearer then? Yes, that's better. (In reply to comment #9) > > > The invocations of resolveCallback and rejectCallback in UserMediaRequest.cpp (:172, 192, 205) are wrapped in callOnMainThread(). Does that address your concern? > > > > Yes and no ;) > > Using callOnMainThread would solve the concern for webkitGetUserMedia > > (although postTask would be better as it is what promise implementation uses > > for calling JS callbacks). > > > > But it creates an issue for MediaDevices.getUserMedia() as the > > postTask/callOnMainThread call is not needed at all to resolve/reject > > promise. > > The JS promise implementation is already doing that work. > > This makes things less efficient and less consistent. > > You can see an illustration of the issue in the -expected.txt generated by > the latest patch. > The order of the callback execution is not really what we would expect. > > First, all promise rejection callbacks are called asynchronously which is > good. > > But the empty dictionary test is displayed before the {audio:true} test for > instance. > > Reason is the custom binding is implemented with just one postTask. > The rejection done in UserMediaRequest is implemented with one > callOnnMainThread followed by one postTask. The double scheduling is not ideal indeed. I believe your proposal to move the scheduling needed for the callback case into the lambda functions does the trick right?

(In reply to comment #10) > > Reason is the custom binding is implemented with just one postTask. > > The rejection done in UserMediaRequest is implemented with one > > callOnnMainThread followed by one postTask. > > The double scheduling is not ideal indeed. I believe your proposal to move > the scheduling needed for the callback case into the lambda functions does > the trick right? Yes. I filed bug 145308 for that purpose.

As a side note, the spec http://w3c.github.io/mediacapture-main/archives/20150324/getusermedia.html calls somehow for that double scheduing. In particular, it says that step 3 should be done asynchronously. Might be worth some spec editing maybe. Should I file a github issue?

Please, file an issue.

Comment on attachment 253586 [details] Removing async promise resolution/rejection changes View in context: https://bugs.webkit.org/attachment.cgi?id=253586&action=review > Source/WebCore/Modules/mediastream/MediaDevices.h:59 > + typedef std::function<void(MediaStream*)> ResolveCallback; > + typedef std::function<void(NavigatorUserMediaError*)> RejectCallback; Why pointers and not references? Can these ever be null?

Comment on attachment 253586 [details] Removing async promise resolution/rejection changes View in context: https://bugs.webkit.org/attachment.cgi?id=253586&action=review >> Source/WebCore/Modules/mediastream/MediaDevices.h:59 >> + typedef std::function<void(NavigatorUserMediaError*)> RejectCallback; > > Why pointers and not references? Can these ever be null? Thanks for reminding me of this point. I checked with Adam and he said references are fine. I will clean this when landing.

(In reply to comment #13) > Please, file an issue. Filed https://github.com/w3c/mediacapture-main/issues/174 for that purpose.

Created attachment 253858 [details] Patch for landing

Comment on attachment 253858 [details] Patch for landing Clearing flags on attachment: 253858 Committed r184984: <http://trac.webkit.org/changeset/184984>

All reviewed patches have been landed. Closing bug.