WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED DUPLICATE of
bug 123158
132855
[MediaStream] [EFL] getUserMedia throws DomException always
https://bugs.webkit.org/show_bug.cgi?id=132855
Summary
[MediaStream] [EFL] getUserMedia throws DomException always
Praveen Jadhav
Reported
2014-05-13 00:03:31 PDT
<!DOCTYPE html> <html> <head> </head> <body> <script> function getUserMedia(dictionary, callback) { try { navigator.webkitGetUserMedia(dictionary, callback, error); } catch (e) { alert('webkitGetUserMedia threw exception :' + e); } } function error() { alert("webkitGetUserMedia threw error"); } function sourceCallback(sources) { alert('got sources' + sources[0].sourceId); } function gotStream(stream) { // Disable the audio track => should mute output. var audioTracks = stream.getAudioTracks(); audioTracks[0].getSources(sourceCallback); } getUserMedia({audio:true}, gotStream); </script> </body> </html> ------------------------------------------------------ Above test content will always throw Domexception with "NotSupportedError" message. However, the same case works fine when it is run as a layout test case.
Attachments
Patch
(58.69 KB, patch)
2014-05-23 00:02 PDT
,
Praveen Jadhav
no flags
Details
Formatted Diff
Diff
Patch
(61.36 KB, patch)
2014-05-23 04:33 PDT
,
Praveen Jadhav
gyuyoung.kim
: review-
Details
Formatted Diff
Diff
Patch
(59.95 KB, patch)
2014-05-25 22:47 PDT
,
Praveen Jadhav
no flags
Details
Formatted Diff
Diff
Patch
(50.66 KB, patch)
2014-05-26 04:29 PDT
,
Praveen Jadhav
no flags
Details
Formatted Diff
Diff
Patch
(58.03 KB, patch)
2014-05-30 03:55 PDT
,
Praveen Jadhav
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Praveen Jadhav
Comment 1
2014-05-23 00:02:26 PDT
Created
attachment 231947
[details]
Patch Patch initializes call to provideUserMediaTo() for EFL port.
Praveen Jadhav
Comment 2
2014-05-23 04:33:53 PDT
Created
attachment 231956
[details]
Patch Patch updated to resolve GTK and Mac build errors.
Eric Carlson
Comment 3
2014-05-23 10:08:41 PDT
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.
Gyuyoung Kim
Comment 4
2014-05-23 19:54:06 PDT
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
Praveen Jadhav
Comment 5
2014-05-25 22:46:41 PDT
(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.
Praveen Jadhav
Comment 6
2014-05-25 22:47:42 PDT
Created
attachment 232058
[details]
Patch All comments are updated.
Gyuyoung Kim
Comment 7
2014-05-25 23:45:29 PDT
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 ?
Praveen Jadhav
Comment 8
2014-05-26 04:29:36 PDT
Created
attachment 232069
[details]
Patch Patch updated with previous comments. ewk_user_media.cpp is not undercompilation and removed from the patch. My mistake. :(
Eric Carlson
Comment 9
2014-05-27 07:57:30 PDT
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?
Praveen Jadhav
Comment 10
2014-05-30 03:55:03 PDT
Created
attachment 232290
[details]
Patch
Praveen Jadhav
Comment 11
2014-05-30 04:03:12 PDT
(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.
Eric Carlson
Comment 12
2014-06-13 11:26:02 PDT
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(¢er);
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.
Philippe Normand
Comment 13
2014-06-21 00:46:51 PDT
This very much looks like a duplicate of
bug 123158
.
Praveen Jadhav
Comment 14
2014-06-22 20:18:52 PDT
(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).
Praveen Jadhav
Comment 15
2014-06-22 20:19:29 PDT
*** This bug has been marked as a duplicate of
bug 123158
***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug