Bug 132855

Created attachment 231947 [details] Patch Patch initializes call to provideUserMediaTo() for EFL port.

Created attachment 231956 [details] Patch Patch updated to resolve GTK and Mac build errors.

Comment on attachment 231956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231956&action=review > Source/WebKit2/UIProcess/API/C/WKPage.cpp:1487 > + return true; This indentation isn't needed. > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:195 > + WKPageDecidePolicyForUserMediaPermissionRequestCallback decidePolicyForUserMediaPermissionRequest; I don't know why the other field names in this struct are aligned, but the new one should probably be aligned too. > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:247 > + WKPageDecidePolicyForUserMediaPermissionRequestCallback decidePolicyForUserMediaPermissionRequest; Ditto. > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:366 > + WKPageDecidePolicyForUserMediaPermissionRequestCallback decidePolicyForUserMediaPermissionRequest; Ditto. > Source/WebKit2/UIProcess/MediaStream/UserMediaPermissionRequestManagerProxy.h:53 > + typedef HashMap<uint64_t, RefPtr<UserMediaPermissionRequest> > PendingRequestMap; Nit: the space in "> >" is unnecessary. > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:69 > + // TODO: Create audio and video stream sources here. Why would you automatically create audio and video sources here when you don't know what the page wants yet? > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:78 > + startUserMedia(); Is this necessary? > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2810 > + return m_userMediaPermissionRequestManager.get(); Why the indentation? > Tools/ChangeLog:3 > + [MediaStream] [EFL] getUserMedia throws DomException always.. Nit: you have two periods here.

Comment on attachment 231956 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231956&action=review r- because of a lot of comments. > Source/WebKit2/PlatformEfl.cmake:124 > + UIProcess/MediaStream/UserMediaPermissionRequest.cpp It would be nicer if you place this common files(which are used by GTK together) to CMakeLists.txt

(In reply to comment #3) > (From update of attachment 231956 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=231956&action=review > > > Source/WebKit2/UIProcess/API/C/WKPage.cpp:1487 > > + return true; > > This indentation isn't needed. > > > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:195 > > + WKPageDecidePolicyForUserMediaPermissionRequestCallback decidePolicyForUserMediaPermissionRequest; > > I don't know why the other field names in this struct are aligned, but the new one should probably be aligned too. > > > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:247 > > + WKPageDecidePolicyForUserMediaPermissionRequestCallback decidePolicyForUserMediaPermissionRequest; > > Ditto. > > > Source/WebKit2/UIProcess/API/C/WKPageUIClient.h:366 > > + WKPageDecidePolicyForUserMediaPermissionRequestCallback decidePolicyForUserMediaPermissionRequest; > > Ditto. > > > Source/WebKit2/UIProcess/MediaStream/UserMediaPermissionRequestManagerProxy.h:53 > > + typedef HashMap<uint64_t, RefPtr<UserMediaPermissionRequest> > PendingRequestMap; > > Nit: the space in "> >" is unnecessary. > > > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:69 > > + // TODO: Create audio and video stream sources here. > > Why would you automatically create audio and video sources here when you don't know what the page wants yet? > > > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:78 > > + startUserMedia(); > > Is this necessary? startUserMedia() will be called whenever webkitGetUserMedia/getUserMedia API call is triggered from web content. so, I assume this is the proper location to create MediaStreamSource instances. In any case, I will clean up now and leave to next implementation review about initialization location. > > > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2810 > > + return m_userMediaPermissionRequestManager.get(); > > Why the indentation? > > > Tools/ChangeLog:3 > > + [MediaStream] [EFL] getUserMedia throws DomException always.. > > Nit: you have two periods here. Thanks! I will update all the comments in the next patch.

Created attachment 232058 [details] Patch All comments are updated.

Comment on attachment 232058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232058&action=review > Source/WebKit2/UIProcess/API/efl/ewk_user_media.cpp:100 > + delete permissionRequest; I'm not sure if we can remove *permissionRequest* instance in this function. Could you explain why we should remove it inside _set() ? > Source/WebKit2/UIProcess/API/efl/ewk_user_media.h:38 > + * Sets the permission to access local media nit: Missing period at the end of line. > Source/WebKit2/UIProcess/API/efl/ewk_user_media.h:46 > + * Suspend the operation for user media permission ditto > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:78 > + } ASSERT_NOT_REACHED() ? > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h:51 > + Unnecessary line. > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h:52 > + WebUserMediaClient(WebPage*); nit: Missing explicit keyword. > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h:53 > + ~WebUserMediaClient(); Would be nicer to add virtual keyword ?

Created attachment 232069 [details] Patch Patch updated with previous comments. ewk_user_media.cpp is not undercompilation and removed from the patch. My mistake. :(

Comment on attachment 232069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232069&action=review > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:42 > + static PassRefPtr<UserMediaPermissionRequestManager> create(WebPage*); It doesn't look like "WebPage" can ever be NULL so this and the constructor should take a reference. > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:43 > + void requestPermission(UserMediaPermission*); Can "UserMediaPermission" be NULL? If not, this should take a reference. > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:60 > +void WebUserMediaClient::requestPermission(WTF::PassRefPtr<UserMediaRequest> request) > +{ > + m_userMediaRequest = request; > + > + m_page->userMediaPermissionRequestManager()->requestPermission(this); > +} How can the request be stored in m_userMediaRequest? If this is called again before decidePermission() is called, won't decidePermission() make a callback on the wrong request? > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:65 > +void WebUserMediaClient::cancelRequest(UserMediaRequest*) > +{ > + return; > +} How does this work? What happens when decidePermission is called subsequently?

Created attachment 232290 [details] Patch

(In reply to comment #9) > (From update of attachment 232069 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232069&action=review > > > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:42 > > + static PassRefPtr<UserMediaPermissionRequestManager> create(WebPage*); > > It doesn't look like "WebPage" can ever be NULL so this and the constructor should take a reference. done. > > > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:43 > > + void requestPermission(UserMediaPermission*); > > Can "UserMediaPermission" be NULL? If not, this should take a reference. Have removed UserMediaPermission completely to update your next comment. :) > > > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:60 > > +void WebUserMediaClient::requestPermission(WTF::PassRefPtr<UserMediaRequest> request) > > +{ > > + m_userMediaRequest = request; > > + > > + m_page->userMediaPermissionRequestManager()->requestPermission(this); > > +} > > How can the request be stored in m_userMediaRequest? If this is called again before decidePermission() is called, won't decidePermission() make a callback on the wrong request? True, have removed m_userMediaRequest and the parameter is registered in HashMap to that it callback is received properly. > > > Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:65 > > +void WebUserMediaClient::cancelRequest(UserMediaRequest*) > > +{ > > + return; > > +} > > How does this work? What happens when decidePermission is called subsequently? I have included notImplemented() for now. Will continue to work on this.

Comment on attachment 232290 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232290&action=review This looks fine to me modulo the fairly minor comments, but I am not a WK2 reviewer so someone else will have to do the r+. > Source/WebCore/platform/mediastream/gstreamer/AVCaptureDeviceManagerGStreamer.cpp:45 > + AVCaptureDeviceManagerGStreamer::setSharedAVCaptureDeviceManagerGStreamer(&center); Nit: this calls setSharedAVCaptureDeviceManagerGStreamer every time, which presumably isn't harmful but does waste cycles. > Source/WebKit2/UIProcess/MediaStream/UserMediaPermissionRequest.cpp:42 > + : m_manager(manager) > + , m_userMediaID(userMediaID) Nit: the destructor should set m_manager to NULL. > Source/WebKit2/UIProcess/MediaStream/UserMediaPermissionRequest.h:40 > + static PassRefPtr<UserMediaPermissionRequest> create(UserMediaPermissionRequestManagerProxy*, uint64_t userMediaID); Nit: "userMediaID" isn't necessary. > Source/WebKit2/UIProcess/MediaStream/UserMediaPermissionRequest.h:48 > + UserMediaPermissionRequest(UserMediaPermissionRequestManagerProxy*, uint64_t userMediaID); Ditto. > Source/WebKit2/UIProcess/MediaStream/UserMediaPermissionRequestManagerProxy.h:47 > + PassRefPtr<UserMediaPermissionRequest> createRequest(uint64_t userMediaID); Nit: "userMediaID" isn't necessary. > Source/WebKit2/UIProcess/MediaStream/UserMediaPermissionRequestManagerProxy.h:50 > + void didReceiveUserMediaPermissionDecision(uint64_t userMediaID, bool allow); Ditto, plus "allow". > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:43 > + static uint64_t uniqueRequestID = 1; > + return uniqueRequestID++; First, both requestPermission and UserMediaPermissionRequestManagerProxy::createRequest will use this ID as the key in a HashMap. I believe a standard HashMap won't allow 0 to be use for a key value, so you should never return 0. Second, it is possible for this to wrap around. While that will take a long time, and it is unlikely that an existing request will still be pending when it does wrap around, it is can happen. Do we care? > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:44 > + void didReceiveUserMediaPermissionDecision(uint64_t userMediaID, bool allowed); Nit: "userMediaID" and "allowed" are not necessary. > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2824 > +UserMediaPermissionRequestManager* WebPage::userMediaPermissionRequestManager() Nit: this never returns NULL, and the call sites assume it can not, so it should return a UserMediaPermissionRequestManager reference.

This very much looks like a duplicate of bug 123158.

(In reply to comment #13) > This very much looks like a duplicate of bug 123158. My mistake. I din't search for the previously raised bugs with proper keystring. bug 108461 and bug 123158 are raised for this issue. Marking this as duplicate of bug 123158 (currently active).

*** This bug has been marked as a duplicate of bug 123158 ***