In order to provide the user as much flexibility as possible when selecting his/her device of preference for capture, MediaStreams MUST have an extendable, adaptable and easily connected interface from WebKit2 to Safari.
<rdar://problem/21983345>
Created attachment 257487 [details] Patch
Created attachment 257849 [details] Patch
Created attachment 257850 [details] Patch
Comment on attachment 257850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257850&action=review > Source/WebCore/platform/mock/UserMediaClientMock.h:48 > + m_request->userMediaAccessGranted(WTF::emptyString(), WTF::emptyString()); This doesn't seem right. I think you will need to modify UserMediClientMock to allow tests to specify the video and/or audio UIDs that are passed through this. > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:158 > if (!_request) > return; > > - _request->userMediaAccessGranted(); > + _request->userMediaAccessGranted(WTF::emptyString(), WTF::emptyString()); > + RemoveRequestFromMap(_request.get()); > +#endif Shouldn't this method be deleted? > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:57 > // FIXME(147062): Need to add in the support for Safari to pass strings given from user's decision on what piece of media to open Is this FIXME still needed?
Created attachment 257961 [details] Patch
Comment on attachment 257487 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257487&action=review > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:57 > // FIXME(147062): Need to add in the support for Safari to pass strings given from user's decision on what piece of media to open Do we file this as well? We need everything in this patch to solve this problem, but this problem is solved with this patch. Obsolete the bug?
Created attachment 257968 [details] Patch
Created attachment 257970 [details] Patch
Created attachment 257973 [details] Patch
Created attachment 257975 [details] Patch
Created attachment 257978 [details] Patch
Created attachment 257983 [details] Patch
Comment on attachment 257983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257983&action=review Looks good generally with the caveats mentioned, but I am not a WK2 reviewer so you will need someone to give the official r+. > Source/WebCore/platform/mock/UserMediaClientMock.h:48 > + m_request->userMediaAccessGranted(WTF::emptyString(), WTF::emptyString()); As I noted before, this doesn't seem right. I think you will need to modify UserMediClientMock to allow tests to specify the video and/or audio UIDs that are passed through this. It is fine to do this in a follow-up, but please file a bug and add a FIXME. > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:156 > + _request->userMediaAccessGranted(WTF::emptyString(), WTF::emptyString()); You didn't add this, but it seems strange to call userMediaAccessGranted from cancelRequest - wouldn't make more sense to call userMediaAccessDenied instead? Please add a FIXME comment so we remember to look at this. > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:70 > + priv->request->allow(String::fromUTF8(videoDeviceUID), String::fromUTF8(audioDeviceUID)); You still need to redefine WebKitPermissionRequestIface.allow > Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:57 > // FIXME(147062): Need to add in the support for Safari to pass strings given from user's decision on what piece of media to open Is this comment still relevant? > Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:47 > -void UserMediaPermissionRequestProxy::allow() > +void UserMediaPermissionRequestProxy::allow(const String& videoDeviceUID, const String& audioDeviceUID) > { > - m_manager.didReceiveUserMediaPermissionDecision(m_userMediaID, true); > + m_manager.didReceiveUserMediaPermissionDecision(m_userMediaID, true, videoDeviceUID, audioDeviceUID); > } > > void UserMediaPermissionRequestProxy::deny() > { > - m_manager.didReceiveUserMediaPermissionDecision(m_userMediaID, false); > + m_manager.didReceiveUserMediaPermissionDecision(m_userMediaID, false, emptyString(), emptyString()); > } Instead of passing a two empty string when the request is denied, UserMediaPermissionRequestManager should have separate methods for allow and deny now that they take different parameters. Maybe void userMediaPermissionRequestWasGranted(uint64_t userMediaID, const String& deviceUIDVideo, const String& deviceUIDAudio); void userMediaPermissionRequestWasDenied(uint64_t userMediaID);
Created attachment 258097 [details] Patch
Comment on attachment 257983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=257983&action=review >> Source/WebCore/platform/mock/UserMediaClientMock.h:48 >> + m_request->userMediaAccessGranted(WTF::emptyString(), WTF::emptyString()); > > As I noted before, this doesn't seem right. I think you will need to modify UserMediClientMock to allow tests to specify the video and/or audio UIDs that are passed through this. It is fine to do this in a follow-up, but please file a bug and add a FIXME. See below comment on emptyString() vs choosing first "best" device. >> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:156 >> + _request->userMediaAccessGranted(WTF::emptyString(), WTF::emptyString()); > > You didn't add this, but it seems strange to call userMediaAccessGranted from cancelRequest - wouldn't make more sense to call userMediaAccessDenied instead? Please add a FIXME comment so we remember to look at this. Yep. But this is in allow >> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:70 >> + priv->request->allow(String::fromUTF8(videoDeviceUID), String::fromUTF8(audioDeviceUID)); > > You still need to redefine WebKitPermissionRequestIface.allow Not really. There's an architecture problem with that, because the class inherits the allow method from a generalized PermissionRequestIface, which affects other classes. Rather do this in the class and define the method as a way to choose the "best" device >> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:57 >> // FIXME(147062): Need to add in the support for Safari to pass strings given from user's decision on what piece of media to open > > Is this comment still relevant? Nope! Deleted >> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:47 >> } > > Instead of passing a two empty string when the request is denied, UserMediaPermissionRequestManager should have separate methods for allow and deny now that they take different parameters. Maybe > > void userMediaPermissionRequestWasGranted(uint64_t userMediaID, const String& deviceUIDVideo, const String& deviceUIDAudio); > void userMediaPermissionRequestWasDenied(uint64_t userMediaID); Later fix.
Created attachment 258109 [details] Patch
Comment on attachment 258109 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258109&action=review > Source/WebCore/ChangeLog:3 > + Linking device query ability from WebKit2 to Safari There's nothing Safari specific about this patch. "to clients"? > Source/WebCore/platform/mock/UserMediaClientMock.h:48 > + m_request->userMediaAccessGranted(m_request->videoDeviceUIDs().at(0), m_request->audioDeviceUIDs().at(0)); Are we sure there will always be at least one in both vectors? > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:156 > + _request->userMediaAccessGranted(_request->videoDeviceUIDs().at(0), _request->audioDeviceUIDs().at(0)); For what reason are we keeping the version of allow that takes no parameters? Also, who calls these? > Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:55 > + const char* deviceName = toImpl(userMediaPermissionRef)->getDeviceNameForUID(name, WebCore::RealtimeMediaSource::Type::Video).utf8().data(); > + WKArrayAppendItem(array, WKStringCreateWithUTF8CString(deviceName)); You don't need to round-trip through char* here, you can use toAPI (or something like that; look around). > Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:64 > + const char* deviceName = toImpl(userMediaPermissionRef)->getDeviceNameForUID(name, WebCore::RealtimeMediaSource::Type::Audio).utf8().data(); Ditto > Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.h:30 > -WK_EXPORT void WKUserMediaPermissionRequestAllow(WKUserMediaPermissionRequestRef); > +WK_EXPORT void WKUserMediaPermissionRequestAllowBest(WKUserMediaPermissionRequestRef); Are you 100% sure you can change the signature of this SPI? Is it new? Is anything using it?
Comment on attachment 258109 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258109&action=review >> Source/WebCore/ChangeLog:3 >> + Linking device query ability from WebKit2 to Safari > > There's nothing Safari specific about this patch. "to clients"? Fixed. >> Source/WebCore/platform/mock/UserMediaClientMock.h:48 >> + m_request->userMediaAccessGranted(m_request->videoDeviceUIDs().at(0), m_request->audioDeviceUIDs().at(0)); > > Are we sure there will always be at least one in both vectors? Yep. There must be, or else the request at the base will throw an error. >> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:156 >> + _request->userMediaAccessGranted(_request->videoDeviceUIDs().at(0), _request->audioDeviceUIDs().at(0)); > > For what reason are we keeping the version of allow that takes no parameters? Also, who calls these? The class inherits from another class, which has an "allow" and "deny" method. So, to make it easier on the developer and to make this class implement its superclass's methods correctly, we MUST keep this function. There's also talk of only allowing the "best" device to be selected for a certain task. This method makes making that request easy (vectors are organized so best device for selection is at 0 index). >> Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:55 >> + WKArrayAppendItem(array, WKStringCreateWithUTF8CString(deviceName)); > > You don't need to round-trip through char* here, you can use toAPI (or something like that; look around). Fixed. >> Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:64 >> + const char* deviceName = toImpl(userMediaPermissionRef)->getDeviceNameForUID(name, WebCore::RealtimeMediaSource::Type::Audio).utf8().data(); > > Ditto Ditto. >> Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.h:30 >> +WK_EXPORT void WKUserMediaPermissionRequestAllowBest(WKUserMediaPermissionRequestRef); > > Are you 100% sure you can change the signature of this SPI? Is it new? Is anything using it? Positive. And we also have another method that lets us use more specific selection if needed.
Created attachment 258208 [details] Patch
Comment on attachment 258208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258208&action=review > Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:54 > + WTF::String deviceName = toImpl(userMediaPermissionRef)->getDeviceNameForUID(name, WebCore::RealtimeMediaSource::Type::Video); No WTF::. Also, why does getDeviceNameForUID have a "get" in its name? No out argument that I can see! > Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:55 > + WKArrayAppendItem(array, toAPI(&API::String::create(deviceName).leakRef())); My confidence in the memory management situation here is not strong, but I haven't looked enough to say it's wrong either. I really think you want a .ptr() here, though, not a leakRef. > Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:64 > + WTF::String deviceName = toImpl(userMediaPermissionRef)->getDeviceNameForUID(name, WebCore::RealtimeMediaSource::Type::Audio); Ditto
Created attachment 258281 [details] Patch
Created attachment 258282 [details] Patch
Created attachment 258283 [details] Patch
Comment on attachment 258208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258208&action=review >> Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:54 >> + WTF::String deviceName = toImpl(userMediaPermissionRef)->getDeviceNameForUID(name, WebCore::RealtimeMediaSource::Type::Video); > > No WTF::. Also, why does getDeviceNameForUID have a "get" in its name? No out argument that I can see! Fixed. >> Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:55 >> + WKArrayAppendItem(array, toAPI(&API::String::create(deviceName).leakRef())); > > My confidence in the memory management situation here is not strong, but I haven't looked enough to say it's wrong either. > > I really think you want a .ptr() here, though, not a leakRef. You had said to do it the way WKString was doing it, and that was how WKString was doing it here. However, I can change that to a ptr. Fixed. >> Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:64 >> + WTF::String deviceName = toImpl(userMediaPermissionRef)->getDeviceNameForUID(name, WebCore::RealtimeMediaSource::Type::Audio); > > Ditto Fixed.
Comment on attachment 258283 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258283&action=review > Source/WebCore/ChangeLog:3 > + Linking device query ability from WebKit2 to clients Please use the updated bug title. > Source/WebCore/ChangeLog:13 > + * platform/graphics/mac/WebGLLayer.mm: > + (-[WebGLLayer initWithGraphicsContext3D:]): This wasn't changed. > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:156 > + _request->userMediaAccessGranted(_request->videoDeviceUIDs().at(0), _request->audioDeviceUIDs().at(0)); Will both of these vectors really have at least one element when a user chooses a device that provides only video or audio. What if the call to getUserMedia only request video or audio? > Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:37 > + toImpl(userMediaPermissionRequestRef)->allow(toImpl(userMediaPermissionRequestRef)->videoDeviceUIDs().at(0), toImpl(userMediaPermissionRequestRef)->audioDeviceUIDs().at(0)); Ditto.
Created attachment 258286 [details] Patch
Created attachment 258287 [details] Patch
Comment on attachment 258286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258286&action=review > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:176 > + if (!_request->videoDeviceUIDs().isEmpty() && !_request->audioDeviceUIDs().isEmpty()) > + _request->userMediaAccessGranted(_request->videoDeviceUIDs().at(0), _request->audioDeviceUIDs().at(0)); > + else if (!_request->videoDeviceUIDs().isEmpty()) > + _request->userMediaAccessGranted(_request->videoDeviceUIDs().at(0), emptyString()); > + else if (!_request->audioDeviceUIDs().isEmpty()) > + _request->userMediaAccessGranted(emptyString(), _request->audioDeviceUIDs().at(0)); Nit: I think this would be easier to follow with a couple of local variables: const String& videoUID = _request->videoDeviceUIDs().isEmpty() ? _request->videoDeviceUIDs().at(0) : emptyString() const String& audioUID = _request->audioDeviceUIDs().isEmpty() ? _request->audioDeviceUIDs().at(0) : emptyString() _request->userMediaAccessGranted(videoUID, audioUID); > Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp:37 > + toImpl(userMediaPermissionRequestRef)->allow(toImpl(userMediaPermissionRequestRef)->videoDeviceUIDs().at(0), toImpl(userMediaPermissionRequestRef)->audioDeviceUIDs().at(0)); Is this OK when one or both of the vectors is empty? > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:70 > + priv->request->allow(priv->request->videoDeviceUIDs().at(0), priv->request->audioDeviceUIDs().at(0)); Ditto.
Comment on attachment 258286 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258286&action=review >> Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:176 >> + _request->userMediaAccessGranted(emptyString(), _request->audioDeviceUIDs().at(0)); > > Nit: I think this would be easier to follow with a couple of local variables: > > const String& videoUID = _request->videoDeviceUIDs().isEmpty() ? _request->videoDeviceUIDs().at(0) : emptyString() > const String& audioUID = _request->audioDeviceUIDs().isEmpty() ? _request->audioDeviceUIDs().at(0) : emptyString() > > _request->userMediaAccessGranted(videoUID, audioUID); I think you mean: const String& videoUID = !_request->videoDeviceUIDs().isEmpty() ? _request->videoDeviceUIDs().at(0) : emptyString() const String& audioUID = !_request->audioDeviceUIDs().isEmpty() ? _request->audioDeviceUIDs().at(0) : emptyString() _request->userMediaAccessGranted(videoUID, audioUID); But fixed!
Created attachment 258848 [details] Patch
Comment on attachment 258848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258848&action=review > Source/WebKit2/ChangeLog:11 > + * Platform/mac/LayerHostingContext.mm: > + (WebKit::LayerHostingContext::setColorMatchUntaggedContent): > + (WebKit::LayerHostingContext::colorMatchUntaggedContent): This looks unrelated. > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:75 > + const String& videoDevice = !priv->request->videoDeviceUIDs().isEmpty() ? priv->request->videoDeviceUIDs().at(0) : emptyString(); > + const String& audioDevice = !priv->request->audioDeviceUIDs().isEmpty() ? priv->request->audioDeviceUIDs().at(0) : emptyString(); This snippet (`.isEmpty() : .at(0) : emptyString()`) is repeated now 6 times. Is there a better way to do this? Can the request vend a "firstVideoDeviceUID()" that returns "emptyString()" if its array is empty? > Tools/WebKitTestRunner/TestController.cpp:1498 > + if (WKArrayGetSize(WKUserMediaPermissionRequestDeviceNamesVideo(request.get())) && WKArrayGetSize(WKUserMediaPermissionRequestDeviceNamesAudio(request.get()))) > + WKUserMediaPermissionRequestAllow(request.get(), reinterpret_cast<WKStringRef>(WKArrayGetItemAtIndex(WKUserMediaPermissionRequestDeviceNamesVideo(request.get()), 0)), reinterpret_cast<WKStringRef>(WKArrayGetItemAtIndex(WKUserMediaPermissionRequestDeviceNamesAudio(request.get()), 0))); And this seems weird; this will only allow if BOTH the video and audio lists are non-empty. What if someone just requests a video camera with no audio capabilities, or vice versa?
Comment on attachment 258848 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258848&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:75 >> + const String& audioDevice = !priv->request->audioDeviceUIDs().isEmpty() ? priv->request->audioDeviceUIDs().at(0) : emptyString(); > > This snippet (`.isEmpty() : .at(0) : emptyString()`) is repeated now 6 times. Is there a better way to do this? Can the request vend a "firstVideoDeviceUID()" that returns "emptyString()" if its array is empty? We could, but that seems like an awfully specific function. However, that could easily be written. >> Tools/WebKitTestRunner/TestController.cpp:1498 >> + WKUserMediaPermissionRequestAllow(request.get(), reinterpret_cast<WKStringRef>(WKArrayGetItemAtIndex(WKUserMediaPermissionRequestDeviceNamesVideo(request.get()), 0)), reinterpret_cast<WKStringRef>(WKArrayGetItemAtIndex(WKUserMediaPermissionRequestDeviceNamesAudio(request.get()), 0))); > > And this seems weird; this will only allow if BOTH the video and audio lists are non-empty. What if someone just requests a video camera with no audio capabilities, or vice versa? I can change this so that the system passes back an empty string if the array size is 0.
Created attachment 258866 [details] Patch
Comment on attachment 258866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258866&action=review > Source/WebKit2/ChangeLog:11 > + * Platform/mac/LayerHostingContext.mm: > + (WebKit::LayerHostingContext::setColorMatchUntaggedContent): > + (WebKit::LayerHostingContext::colorMatchUntaggedContent): Looks like you forgot to remove these. > Source/WebCore/Modules/mediastream/UserMediaRequest.h:79 > + const String& firstVideoDeviceUID() const { return !videoDeviceUIDs().isEmpty() ? videoDeviceUIDs().at(0) : emptyString(); } > + const String& firstAudioDeviceUID() const { return !audioDeviceUIDs().isEmpty() ? audioDeviceUIDs().at(0) : emptyString(); } Nit: you can use the instance variables directly instead of calling the functions. > Source/WebKit/mac/WebCoreSupport/WebUserMediaClient.mm:157 > + const String& videoUID = _request->firstVideoDeviceUID(); > + const String& audioUID = _request->firstAudioDeviceUID(); Nit: no need for these local variables now: _request->userMediaAccessGranted(_request->firstVideoDeviceUID(), _request->firstAudioDeviceUID()); > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:77 > + const String& videoDevice = priv->request->videoDeviceUIDs().firstVideoDeviceUID(); > + const String& audioDevice = priv->request->audioDeviceUIDs().firstAudioDeviceUID(); > + > + priv->request->allow(videoDevice, audioDevice); Ditto.
Created attachment 258876 [details] Patch
Comment on attachment 258876 [details] Patch r=me.
Comment on attachment 258876 [details] Patch Clearing flags on attachment: 258876 Committed r188385: <http://trac.webkit.org/changeset/188385>
All reviewed patches have been landed. Closing bug.
Comment on attachment 258876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258876&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:91 > +static void webkitUserMediaPermissionRequestAllow(WebKitPermissionRequest* request, const char* videoDeviceUID, const char* audioDeviceUID) > +{ > + ASSERT(WEBKIT_IS_USER_MEDIA_PERMISSION_REQUEST(request)); > + > + WebKitUserMediaPermissionRequestPrivate* priv = WEBKIT_USER_MEDIA_PERMISSION_REQUEST(request)->priv; > + > + // Only one decision at a time. > + if (priv->madeDecision) > + return; > + > + priv->madeDecision = true; > + priv->request->allow(String::fromUTF8(videoDeviceUID), String::fromUTF8(audioDeviceUID)); I think this is dead code. The interface is defined as void (* allow) (WebKitPermissionRequest *request), and webkitUserMediaPermissionRequestAllow is assigned to WebKitPermissionRequestIface.allow. We have two symbols with the same name, but I guess the compiler chooses the one that matches the signature. So, this one is simply never used.