RESOLVED FIXED 147263
UserMediaRequest should supply IDs of devices selected by user
https://bugs.webkit.org/show_bug.cgi?id=147263
Summary UserMediaRequest should supply IDs of devices selected by user
Matthew Daiter
Reported 2015-07-24 09:35:37 PDT
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.
Attachments
Patch (17.70 KB, patch)
2015-07-24 16:02 PDT, Matthew Daiter
no flags
Patch (17.00 KB, patch)
2015-07-30 13:38 PDT, Matthew Daiter
no flags
Patch (15.06 KB, patch)
2015-07-30 13:44 PDT, Matthew Daiter
no flags
Patch (15.88 KB, patch)
2015-07-31 14:49 PDT, Matthew Daiter
no flags
Patch (16.98 KB, patch)
2015-07-31 15:22 PDT, Matthew Daiter
no flags
Patch (17.26 KB, patch)
2015-07-31 15:36 PDT, Matthew Daiter
no flags
Patch (17.30 KB, patch)
2015-07-31 15:53 PDT, Matthew Daiter
no flags
Patch (17.32 KB, patch)
2015-07-31 15:58 PDT, Matthew Daiter
no flags
Patch (17.37 KB, patch)
2015-07-31 16:22 PDT, Matthew Daiter
no flags
Patch (18.65 KB, patch)
2015-07-31 17:40 PDT, Matthew Daiter
no flags
Patch (18.65 KB, patch)
2015-08-03 12:06 PDT, Matthew Daiter
no flags
Patch (19.76 KB, patch)
2015-08-03 13:59 PDT, Matthew Daiter
no flags
Patch (19.75 KB, patch)
2015-08-04 14:14 PDT, Matthew Daiter
no flags
Patch (19.74 KB, patch)
2015-08-05 10:17 PDT, Matthew Daiter
no flags
Patch (19.74 KB, patch)
2015-08-05 10:19 PDT, Matthew Daiter
no flags
Patch (19.73 KB, patch)
2015-08-05 10:22 PDT, Matthew Daiter
no flags
Patch (20.48 KB, patch)
2015-08-05 11:15 PDT, Matthew Daiter
no flags
Patch (20.50 KB, patch)
2015-08-05 11:16 PDT, Matthew Daiter
no flags
Patch (20.41 KB, patch)
2015-08-12 15:01 PDT, Matthew Daiter
no flags
Patch (22.25 KB, patch)
2015-08-12 18:28 PDT, Matthew Daiter
no flags
Patch (22.21 KB, patch)
2015-08-12 19:57 PDT, Matthew Daiter
no flags
Radar WebKit Bug Importer
Comment 1 2015-07-24 09:37:11 PDT
Matthew Daiter
Comment 2 2015-07-24 16:02:42 PDT
Matthew Daiter
Comment 3 2015-07-30 13:38:23 PDT
Matthew Daiter
Comment 4 2015-07-30 13:44:26 PDT
Eric Carlson
Comment 5 2015-07-31 08:51:50 PDT
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?
Matthew Daiter
Comment 6 2015-07-31 14:49:08 PDT
Matthew Daiter
Comment 7 2015-07-31 14:51:58 PDT
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?
Matthew Daiter
Comment 8 2015-07-31 15:22:40 PDT
Matthew Daiter
Comment 9 2015-07-31 15:36:58 PDT
Matthew Daiter
Comment 10 2015-07-31 15:53:31 PDT
Matthew Daiter
Comment 11 2015-07-31 15:58:49 PDT
Matthew Daiter
Comment 12 2015-07-31 16:22:51 PDT
Matthew Daiter
Comment 13 2015-07-31 17:40:57 PDT
Eric Carlson
Comment 14 2015-08-01 09:02:15 PDT
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);
Matthew Daiter
Comment 15 2015-08-03 12:06:44 PDT
Matthew Daiter
Comment 16 2015-08-03 12:24:05 PDT
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.
Matthew Daiter
Comment 17 2015-08-03 13:59:26 PDT
Tim Horton
Comment 18 2015-08-04 11:16:18 PDT
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?
Matthew Daiter
Comment 19 2015-08-04 14:13:44 PDT
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.
Matthew Daiter
Comment 20 2015-08-04 14:14:44 PDT
Tim Horton
Comment 21 2015-08-04 16:23:45 PDT
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
Matthew Daiter
Comment 22 2015-08-05 10:17:47 PDT
Matthew Daiter
Comment 23 2015-08-05 10:19:18 PDT
Matthew Daiter
Comment 24 2015-08-05 10:22:57 PDT
Matthew Daiter
Comment 25 2015-08-05 10:24:41 PDT
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.
Eric Carlson
Comment 26 2015-08-05 10:43:15 PDT
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.
Matthew Daiter
Comment 27 2015-08-05 11:15:08 PDT
Matthew Daiter
Comment 28 2015-08-05 11:16:44 PDT
Eric Carlson
Comment 29 2015-08-05 11:21:32 PDT
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.
Matthew Daiter
Comment 30 2015-08-12 14:56:30 PDT
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!
Matthew Daiter
Comment 31 2015-08-12 15:01:32 PDT
Jer Noble
Comment 32 2015-08-12 17:13:45 PDT
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?
Matthew Daiter
Comment 33 2015-08-12 17:25:15 PDT
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.
Matthew Daiter
Comment 34 2015-08-12 18:28:49 PDT
Eric Carlson
Comment 35 2015-08-12 19:29:32 PDT
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.
Matthew Daiter
Comment 36 2015-08-12 19:57:42 PDT
Jer Noble
Comment 37 2015-08-13 10:24:31 PDT
Comment on attachment 258876 [details] Patch r=me.
WebKit Commit Bot
Comment 38 2015-08-13 10:34:07 PDT
Comment on attachment 258876 [details] Patch Clearing flags on attachment: 258876 Committed r188385: <http://trac.webkit.org/changeset/188385>
WebKit Commit Bot
Comment 39 2015-08-13 10:34:12 PDT
All reviewed patches have been landed. Closing bug.
Carlos Garcia Campos
Comment 40 2015-08-14 02:02:15 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.