Bug 123158 – [WK2] UserMediaClient support

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Philippe Normand]

We need a UserMediaClient in WK2, to ask the user access to audio/video devices. It would be similar to the GeolocationPermission client, I think.

In the GTK API I propose to integrate it via the permission-request signal.

I started a patch, not nearly ready yet though.

[Philippe Normand]

Created attachment 215183 [details]
[wk2] usermediaclient

First shot for EWS.

[WebKit Commit Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Commit Bot]

Attachment 215183 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediastream/UserMediaRequest.cpp', u'Source/WebCore/Modules/mediastream/UserMediaRequest.h', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/UIProcess/API/C/WKAPICast.h', u'Source/WebKit2/UIProcess/API/C/WKPage.h', u'Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp', u'Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequestPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.messages.in', u'Source/WebKit2/UIProcess/WebUIClient.cpp', u'Source/WebKit2/UIProcess/WebUIClient.h', u'Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp', u'Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h', u'Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.messages.in', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/WebKitTestRunner/TestController.cpp']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h"
Tools/MiniBrowser/gtk/BrowserWindow.c:417:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:418:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:419:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 4 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 215183 [details]
[wk2] usermediaclient

Attachment 215183 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/10658006

[Build Bot]

Comment on attachment 215183 [details]
[wk2] usermediaclient

Attachment 215183 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/12508008

[Build Bot]

Comment on attachment 215183 [details]
[wk2] usermediaclient

Attachment 215183 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/10918081

[Darin Adler]

Comment on attachment 215183 [details]
[wk2] usermediaclient

View in context: https://bugs.webkit.org/attachment.cgi?id=215183&action=review

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:111
> +    return document() ? document()->frame() : 0;

nullptr please, not 0

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:63
> +    Document* document() const;

Can this be null? If not, then please use Document& instead of Document*.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:39
> +    m_manager = 0;

Please add nullptr.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:48
> +    m_manager = 0;

Please add nullptr.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:53
> +    m_manager = 0;

Please add nullptr.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:32
> +    static PassRefPtr<UserMediaPermissionRequestProxy> create(UserMediaPermissionRequestManagerProxy* manager, uint64_t userMediaPermissionRequestID)

Argument type should be UserMediaPermissionRequestManagerProxy&.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:43
> +    UserMediaPermissionRequestProxy(UserMediaPermissionRequestManagerProxy*, uint64_t userMediaPermissionRequestID);

Argument type should be UserMediaPermissionRequestManagerProxy&.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:57
> +    Frame* frame = request->frame();

What guarantees this won’t be null?

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:61
> +    WebFrameLoaderClient* webFrameLoaderClient = toWebFrameLoaderClient(frame->loader().client());
> +    WebFrame* webFrame = webFrameLoaderClient ? webFrameLoaderClient->webFrame() : 0;
> +    ASSERT(webFrame);

Doesn’t make sense to check for null and then assert that it’s not null. Either remove the null checks, or make them something more than an assertion.

Also, please use nullptr, not 0.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:63
> +    SecurityOrigin* origin = frame->document()->securityOrigin();

Since this can’t be null, if you want to use a local variable, please use a reference, not a pointer.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:65
> +

Stray blank line.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:36
> +    explicit UserMediaRequestManager(WebPage*);

This should take WebPage& instead of WebPage*.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:38
> +    void requestPermission(PassRefPtr<WebCore::UserMediaRequest>);

This should take WebCore::UserMediaRequest& instead of PassRefPtr.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:39
> +    void cancelRequest(WebCore::UserMediaRequest*);

This should take WebCore::UserMediaRequest& instead of WebCore::UserMediaRequest*.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:45
> +    typedef HashMap<uint64_t, WebCore::UserMediaRequest*> IDToUserMediaRequestMap;
> +    typedef HashMap<WebCore::UserMediaRequest*, uint64_t> UserMediaRequestToIDMap;

These should not be typedef'd. You should just use the types explicitly below.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:49
> +    WebPage* m_page;

This should be WebPage& instead of WebPage*.

[Philippe Normand]

Thanks for the review, Darin! I will upload new versions of the patch to make EWS happy and address your comments as well.

[Philippe Normand]

Created attachment 215243 [details]
[wk2] usermediaclient

[WebKit Commit Bot]

Attachment 215243 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediastream/UserMediaRequest.cpp', u'Source/WebCore/Modules/mediastream/UserMediaRequest.h', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/UIProcess/API/C/WKAPICast.h', u'Source/WebKit2/UIProcess/API/C/WKPage.h', u'Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp', u'Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequestPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.messages.in', u'Source/WebKit2/UIProcess/WebUIClient.cpp', u'Source/WebKit2/UIProcess/WebUIClient.h', u'Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp', u'Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h', u'Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.messages.in', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/WebKitTestRunner/TestController.cpp']" exit_code: 1
Source/WebKit2/CMakeLists.txt:437:  Alphabetical sorting problem. "UIProcess/UserMediaPermissionRequestManagerProxy.cpp" should be before "UIProcess/Storage/LocalStorageDatabaseTracker.cpp".  [list/order] [5]
Source/WebKit2/CMakeLists.txt:533:  Alphabetical sorting problem. "WebProcess/MediaStream/UserMediaRequestManager.cpp" should be before "WebProcess/Storage/StorageNamespaceImpl.cpp".  [list/order] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h"
Tools/MiniBrowser/gtk/BrowserWindow.c:417:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:418:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:419:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/MiniBrowser/gtk/BrowserWindow.c:420:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 6 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 215244 [details]
[wk2] usermediaclient

[WebKit Commit Bot]

Attachment 215244 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediastream/UserMediaRequest.cpp', u'Source/WebCore/Modules/mediastream/UserMediaRequest.h', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/UIProcess/API/C/WKAPICast.h', u'Source/WebKit2/UIProcess/API/C/WKPage.h', u'Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp', u'Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequestPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.messages.in', u'Source/WebKit2/UIProcess/WebUIClient.cpp', u'Source/WebKit2/UIProcess/WebUIClient.h', u'Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp', u'Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h', u'Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.messages.in', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/WebKitTestRunner/TestController.cpp']" exit_code: 1
Source/WebKit2/CMakeLists.txt:428:  Alphabetical sorting problem. "UIProcess/UserMediaPermissionRequestManagerProxy.cpp" should be before "UIProcess/Notifications/WebNotificationProvider.cpp".  [list/order] [5]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 215244 [details]
[wk2] usermediaclient

Attachment 215244 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/11958008

[Carlos Garcia Campos]

Comment on attachment 215244 [details]
[wk2] usermediaclient

View in context: https://bugs.webkit.org/attachment.cgi?id=215244&action=review

> Source/WebKit2/UIProcess/API/C/WKPage.h:288
> +    WKPageDecidePolicyForUserMediaPermissionRequestCallback             decidePolicyForUserMediaPermissionRequest;
> +

I think you need to add a new interface version here.

[Build Bot]

Comment on attachment 215244 [details]
[wk2] usermediaclient

Attachment 215244 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/11878102

[Philippe Normand]

Created attachment 215252 [details]
Patch

[Philippe Normand]

This should make EWS happy but not addressing review comments yet.

[Philippe Normand]

Replying to Darin's comments, I made most of the requested changes :)

(In reply to comment #7)
> (From update of attachment 215183 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215183&action=review
> 
> > Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:111
> > +    return document() ? document()->frame() : 0;
> 
> nullptr please, not 0
> 

Ok.

> > Source/WebCore/Modules/mediastream/UserMediaRequest.h:63
> > +    Document* document() const;
> 
> Can this be null? If not, then please use Document& instead of Document*.
> 

Well, I don't see how to make it return a reference anyway, toDocument() won't cast its result to a reference because m_scriptExecutionContext is a raw pointer (non-const).

> > Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:39
> > +    m_manager = 0;
> 
> Please add nullptr.
> 
> > Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:48
> > +    m_manager = 0;
> 
> Please add nullptr.
> 
> > Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:53
> > +    m_manager = 0;
> 
> Please add nullptr.
> 

Well, if the variable becomes a reference I think there's no need anymore to reset it, right?

> > Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:32
> > +    static PassRefPtr<UserMediaPermissionRequestProxy> create(UserMediaPermissionRequestManagerProxy* manager, uint64_t userMediaPermissionRequestID)
> 
> Argument type should be UserMediaPermissionRequestManagerProxy&.
> 
> > Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h:43
> > +    UserMediaPermissionRequestProxy(UserMediaPermissionRequestManagerProxy*, uint64_t userMediaPermissionRequestID);
> 
> Argument type should be UserMediaPermissionRequestManagerProxy&.
> 

Ok!

> > Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:57
> > +    Frame* frame = request->frame();
> 
> What guarantees this won’t be null?
> 

Good point, an ASSERT here wouldn't hurt I think.

> > Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:61
> > +    WebFrameLoaderClient* webFrameLoaderClient = toWebFrameLoaderClient(frame->loader().client());
> > +    WebFrame* webFrame = webFrameLoaderClient ? webFrameLoaderClient->webFrame() : 0;
> > +    ASSERT(webFrame);
> 
> Doesn’t make sense to check for null and then assert that it’s not null. Either remove the null checks, or make them something more than an assertion.
> 
> Also, please use nullptr, not 0.
> 

Ok.

> > Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:63
> > +    SecurityOrigin* origin = frame->document()->securityOrigin();
> 
> Since this can’t be null, if you want to use a local variable, please use a reference, not a pointer.
> 

Ok.

> > Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:65
> > +
> 
> Stray blank line.
> 

Removed.

> > Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:36
> > +    explicit UserMediaRequestManager(WebPage*);
> 
> This should take WebPage& instead of WebPage*.
> 

Right!

> > Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:38
> > +    void requestPermission(PassRefPtr<WebCore::UserMediaRequest>);
> 
> This should take WebCore::UserMediaRequest& instead of PassRefPtr.
> 
> > Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:39
> > +    void cancelRequest(WebCore::UserMediaRequest*);
> 
> This should take WebCore::UserMediaRequest& instead of WebCore::UserMediaRequest*.
> 

Hum I'm not sure because otherwise the ownership of the request is not guaranteed. The manager should keep track of the requests in its hash tables.

> > Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:45
> > +    typedef HashMap<uint64_t, WebCore::UserMediaRequest*> IDToUserMediaRequestMap;
> > +    typedef HashMap<WebCore::UserMediaRequest*, uint64_t> UserMediaRequestToIDMap;
> 
> These should not be typedef'd. You should just use the types explicitly below.
> 

Oh, ok. I've seen this kind of typedefs in other places in WebKit2, may I ask why this is a bad practice?

> > Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:49
> > +    WebPage* m_page;
> 
> This should be WebPage& instead of WebPage*.

Ok.

[Philippe Normand]

Comment on attachment 215252 [details]
Patch

I'll upload the new version soon.

[Philippe Normand]

Created attachment 215373 [details]
wk2 usermediaclient

[WebKit Commit Bot]

Attachment 215373 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/mediastream/UserMediaRequest.cpp', u'Source/WebCore/Modules/mediastream/UserMediaRequest.h', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/Shared/API/c/WKBase.h', u'Source/WebKit2/Shared/APIClientTraits.cpp', u'Source/WebKit2/Shared/APIClientTraits.h', u'Source/WebKit2/Shared/APIObject.h', u'Source/WebKit2/UIProcess/API/C/WKAPICast.h', u'Source/WebKit2/UIProcess/API/C/WKPage.h', u'Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.cpp', u'Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequestPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp', u'Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.messages.in', u'Source/WebKit2/UIProcess/WebUIClient.cpp', u'Source/WebKit2/UIProcess/WebUIClient.h', u'Source/WebKit2/UIProcess/mac/WebInspectorProxyMac.mm', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp', u'Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h', u'Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.cpp', u'Source/WebKit2/WebProcess/WebPage/WebPage.h', u'Source/WebKit2/WebProcess/WebPage/WebPage.messages.in', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/MiniBrowser/mac/WK2BrowserWindowController.m', u'Tools/WebKitTestRunner/TestController.cpp']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.h:24:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h"
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 215375 [details]
wk2 usermediaclient

[Build Bot]

Comment on attachment 215375 [details]
wk2 usermediaclient

Attachment 215375 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/17048056

[Build Bot]

Comment on attachment 215375 [details]
wk2 usermediaclient

Attachment 215375 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/17148046

[Philippe Normand]

Created attachment 215378 [details]
wk2 usermediaclient

[Benjamin Poulain]

Comment on attachment 215378 [details]
wk2 usermediaclient

View in context: https://bugs.webkit.org/attachment.cgi?id=215378&action=review

I think your code is in great shape. For me the biggest road block with this iterations are:
-Generic API tests.
-You should have helpers for WebKitTestRunner to use this from LayoutTest (+ some layout tests). See TestController::setGeolocationPermission() for an example.

I was originally against the public API, but I start to think it will be a useful addition to everyone. What you need to do though is cross-platform tests.

> Source/WebKit2/UIProcess/API/C/WKPage.h:224
> +typedef void (*WKPageDecidePolicyForUserMediaPermissionRequestCallback)(WKPageRef page, WKFrameRef frame, WKSecurityOriginRef origin, WKUserMediaPermissionRequestRef permissionRequest, const void* clientInfo);

Maybe WKPageDecidePolicyForUserMediaCapturePermissionRequestCallback?

Ok, tell me if I am misunderstanding...but:
From the spec of getUserMedia (http://www.w3.org/TR/mediacapture-streams/#methods-5), it looks to me that the JS API can request either audio, video, or both. The API can also request specific hardware like the front facing camera ("facingMode").
It seem to me that the UIClient interface should, at a minimum, expose the device type. The message presented to the user should be different if the page request access to the microphone, the camera or both.

> Source/WebKit2/UIProcess/API/C/WKPage.h:290
> +

Extra line here.

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:160
> +    GRefPtr<WebKitUserMediaPermissionRequest> userMediaPermissionRequest = adoptGRef(webkitUserMediaPermissionRequestCreate(toImpl(request)));
> +    webkitWebViewMakePermissionRequest(WEBKIT_WEB_VIEW(clientInfo), WEBKIT_PERMISSION_REQUEST(userMediaPermissionRequest.get()));

I am a little concerned you do not use the security origin here.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:59
> +    // Only one decision at a time.
> +    if (priv->madeDecision)
> +        return;

This is smart, I got into a but like this once. :)

But do you initialize "madeDecision" to false anywhere?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:62
> +    priv->madeDecision = true;

I think you should do this before priv->request->allow() since it is impossible to ensure no side effect from allow() will re-enter this path.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:76
> +    priv->request->deny();
> +    priv->madeDecision = true;

Ditto.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:66
> +#if ENABLE(MEDIA_STREAM)
> +    m_page->process()->send(Messages::WebPage::DidReceiveUserMediaPermissionDecision(userMediaRequestID, allowed), m_page->pageID());
> +#else
> +    UNUSED_PARAM(allowed);
> +#endif
> +
> +    m_pendingRequests.remove(it);

You should take a new ref of UserMediaPermissionRequestProxy, and remove it immediately from the HashMap, then call send on the process.

That way you avoid the risk of holding an iterator after the ADT was modified.

> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:4106
> +				4A2020A0181C15DE00E17834 /* MediaStream */,
>  				755422C518064FFC0046F6A8 /* OriginData */,
>  				512E352A130B559900ABD19A /* ApplicationCache */,

Build sections should be sorted alphabetically. Same for the the ones bellow.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:48
> +

Extra blank line.

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:49
> +void UserMediaRequestManager::requestPermission(PassRefPtr<WebCore::UserMediaRequest> prpRequest)

prp?

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:62
> +    WebFrameLoaderClient* webFrameLoaderClient = toWebFrameLoaderClient(frame->loader().client());
> +    WebFrame* webFrame = webFrameLoaderClient ? webFrameLoaderClient->webFrame() : nullptr;
> +    ASSERT(webFrame);

Seriously? That is the only way to get the WebFrame? :(

> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:77
> +    for (UserMediaRequestToIDMap::const_iterator it = m_userMediaRequestToIDMap.begin(), end = m_userMediaRequestToIDMap.end(); it != end; ++it) {
> +        RefPtr<WebCore::UserMediaRequest> storedRequest = it->key;
> +        if (storedRequest.get() == request) {
> +            m_idToUserMediaRequestMap.remove(it->value);
> +            m_userMediaRequestToIDMap.remove(it->key);
> +            return;
> +        }
> +    }

I don't think that is what this is intended to do.

The idea behind the cancelRequests APIs is you can dismiss a dialog if it was never shown to the user. For example, if you make your dialog tab-modal, and the tab that shows the dialog is not visible, you can dismiss the dialog without the user ever knowing.

The reason this is not implemented in managers is simply that OS X does not do Tab Modal.

I don't mind this code. You can keep it as it is if it is enough for GTK. But if you expose a frame-based API for request, you may want to expose this so that your browser/apps can do a better job at presenting the information to the user.

> Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.cpp:36
> +WebUserMediaClient::~WebUserMediaClient()
> +{
> +}

This is unnecessary.

> Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h:34
> +    WebUserMediaClient(WebPage* page)
> +        : m_page(page)

You should use references instead of pointers for page and m_page.

[Carlos Garcia Campos]

(In reply to comment #26)
> > Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:59
> > +    // Only one decision at a time.
> > +    if (priv->madeDecision)
> > +        return;
> 
> This is smart, I got into a but like this once. :)
> 
> But do you initialize "madeDecision" to false anywhere?

GObjects are filled with 0 when allocated.

[Philippe Normand]

Comment on attachment 215378 [details]
wk2 usermediaclient

View in context: https://bugs.webkit.org/attachment.cgi?id=215378&action=review

Thanks for the review! I started updating the patch but haven't had time yet to complete it.

>> Source/WebKit2/UIProcess/API/C/WKPage.h:224
>> +typedef void (*WKPageDecidePolicyForUserMediaPermissionRequestCallback)(WKPageRef page, WKFrameRef frame, WKSecurityOriginRef origin, WKUserMediaPermissionRequestRef permissionRequest, const void* clientInfo);
> 
> Maybe WKPageDecidePolicyForUserMediaCapturePermissionRequestCallback?
> 
> Ok, tell me if I am misunderstanding...but:
> From the spec of getUserMedia (http://www.w3.org/TR/mediacapture-streams/#methods-5), it looks to me that the JS API can request either audio, video, or both. The API can also request specific hardware like the front facing camera ("facingMode").
> It seem to me that the UIClient interface should, at a minimum, expose the device type. The message presented to the user should be different if the page request access to the microphone, the camera or both.

Right, good point! I started adding MediaConstraints support in the WK2 messages (the webprocess would need to pass those to the UI process) but I'm having some issues integrating this in the build :(

>> Source/WebKit2/UIProcess/API/C/WKPage.h:290
>> +
> 
> Extra line here.

Removed

>> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:160
>> +    webkitWebViewMakePermissionRequest(WEBKIT_WEB_VIEW(clientInfo), WEBKIT_PERMISSION_REQUEST(userMediaPermissionRequest.get()));
> 
> I am a little concerned you do not use the security origin here.

Hum yes, I should do something with it indeed :) Thanks for pointing that out

>> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:62
>> +    priv->madeDecision = true;
> 
> I think you should do this before priv->request->allow() since it is impossible to ensure no side effect from allow() will re-enter this path.

Right.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:66
>> +    m_pendingRequests.remove(it);
> 
> You should take a new ref of UserMediaPermissionRequestProxy, and remove it immediately from the HashMap, then call send on the process.
> 
> That way you avoid the risk of holding an iterator after the ADT was modified.

Ok I'll look into this!

>> Source/WebKit2/WebKit2.xcodeproj/project.pbxproj:4106
>>  				512E352A130B559900ABD19A /* ApplicationCache */,
> 
> Build sections should be sorted alphabetically. Same for the the ones bellow.

I only added the files using Xcode... Perhaps this ordering issue belongs to a future patch as I see other ordering issues in the project file.

>> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:49
>> +void UserMediaRequestManager::requestPermission(PassRefPtr<WebCore::UserMediaRequest> prpRequest)
> 
> prp?

PassRefPtr, that's used else where (and in the smart pointer docs of webkit.org), but I can rename it.

>> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:62
>> +    ASSERT(webFrame);
> 
> Seriously? That is the only way to get the WebFrame? :(

I'll try to improve this.

>> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:77
>> +    }
> 
> I don't think that is what this is intended to do.
> 
> The idea behind the cancelRequests APIs is you can dismiss a dialog if it was never shown to the user. For example, if you make your dialog tab-modal, and the tab that shows the dialog is not visible, you can dismiss the dialog without the user ever knowing.
> 
> The reason this is not implemented in managers is simply that OS X does not do Tab Modal.
> 
> I don't mind this code. You can keep it as it is if it is enough for GTK. But if you expose a frame-based API for request, you may want to expose this so that your browser/apps can do a better job at presenting the information to the user.

Oh ok, thanks for the explanation. I'm not sure yet this is really needed for GTK, I'll talk with the other port maintainers :)

>> Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h:34
>> +        : m_page(page)
> 
> You should use references instead of pointers for page and m_page.

Yes I think I did it in other places of the patch but forgot to do it here indeed.

[Eric Carlson]

(In reply to comment #28)
> (From update of attachment 215378 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215378&action=review
> >> Source/WebKit2/WebProcess/MediaStream/UserMediaRequestManager.cpp:49
> >> +void UserMediaRequestManager::requestPermission(PassRefPtr<WebCore::UserMediaRequest> prpRequest)
> > 
> > prp?
> 
> PassRefPtr, that's used else where (and in the smart pointer docs of webkit.org), but I can rename it.
> 

  Indeed, our "RefPtr and PassRefPtr Basics" docs say [1]:

    If a function does take ownership of an object, the argument should be a PassRefPtr. This
    includes most setter functions. Unless the use of the argument is very simple, the argument
    should be transferred to a RefPtr at the start of the function; the argument can be named
    with a “prp” prefix in such cases.

[1] http://www.webkit.org/coding/RefPtr.html

[Seongjun Kim]

Created attachment 230597 [details]
Patch

Patch based on work previously done by Philippe Normand

Commit log is not yet..

[WebKit Commit Bot]

Attachment 230597 [details] did not pass style-queue:


WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h"
ERROR: Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:42:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 35 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Seongjun Kim]

Created attachment 230599 [details]
Fix style

[Philippe Normand]

I wanted to try the patch but it seems like it already got bitrotten, would you mind rebasing it if possible?

[Seongjun Kim]

(In reply to comment #33)
> I wanted to try the patch but it seems like it already got bitrotten, would you mind rebasing it if possible?

No, not at all.
I will update in today.

[Seongjun Kim]

Created attachment 231286 [details]
Patch

[Philippe Normand]

It'd be nice to have green EWS so we can ping Benjamin about a review. Can you please rebase the patch against ToT?

[Seongjun Kim]

(In reply to comment #36)
> It'd be nice to have green EWS so we can ping Benjamin about a review. Can you please rebase the patch against ToT?

Yes, I can do that in Monday.

[Seongjun Kim]

Created attachment 232699 [details]
Patch

[Philippe Normand]

Comment on attachment 232699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=232699&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.h:48
> +#include "UserMediaPermissionRequestManager.h"

Missing ENABLE(MEDIA_STREAM) guards

[Praveen Jadhav]

*** Bug 132855 has been marked as a duplicate of this bug. ***

[Seongjun Kim]

Created attachment 233596 [details]
Patch

[Seongjun Kim]

I rebased the patch and applied Philippe's comment.

[Philippe Normand]

/Volumes/Data/EWS/WebKit/Source/WebKit2/UIProcess/WebPageProxy.cpp:263:15: error: constructor for 'WebKit::WebPageProxy' must explicitly initialize the member 'm_userMediaPermissionRequestManager' which does not have a default constructor
WebPageProxy::WebPageProxy(PageClient& pageClient, WebProcessProxy& process, uint64_t pageID, const WebPageConfiguration& configuration)
              ^
In file included from /Volumes/Data/EWS/WebKit/Source/WebKit2/UIProcess/WebPageProxy.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebKit2/UIProcess/WebPageProxy.h:1325:44: note: member is declared here
    UserMediaPermissionRequestManagerProxy m_userMediaPermissionRequestManager;
                                           ^
In file included from /Volumes/Data/EWS/WebKit/Source/WebKit2/UIProcess/WebPageProxy.cpp:28:
In file included from /Volumes/Data/EWS/WebKit/Source/WebKit2/UIProcess/WebPageProxy.h:48:
/Volumes/Data/EWS/WebKit/Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:30:7: note: 'WebKit::UserMediaPermissionRequestManagerProxy' declared here
class UserMediaPermissionRequestManagerProxy {
      ^
1 error generated.

[Seongjun Kim]

Created attachment 233672 [details]
Patch

[Seongjun Kim]

(In reply to comment #44)
> Created an attachment (id=233672) [details]
> Patch

Patch updated to resolve --no-media-stream build errors.

[Eric Carlson]

Comment on attachment 233672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233672&action=review

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:41
> +    static uint64_t uniqueRequestID = 1;
> +    return uniqueRequestID++;

First, 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?

Second, 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 (when it does wrap around).

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:41
> +    void startRequest(PassRefPtr<WebCore::UserMediaRequest>);

Nit: "requestPermission" might be a better name.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:44
> +    void didReceiveUserMediaPermissionDecision(uint64_t userMediaID, bool allowed);

Nit: these parameter names aren't necessary.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1192
> +    UserMediaPermissionRequestManager m_userMediaPermissionRequestManager;

This will be unused in most clients, why not have a std::unique_ptr<> that is allocated the first time it is used.

[Philippe Normand]

(In reply to comment #46)
> (From update of attachment 233672 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233672&action=review
> 
> > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:41
> > +    static uint64_t uniqueRequestID = 1;
> > +    return uniqueRequestID++;
> 
> First, 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?
> 

I think that so far we haven't paid attention to this issue, this pattern is used elsewhere in WebKit2 (WebFrame, Geolocation, Notifications and others).

> Second, 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 (when it does wrap around).
> 

Well, would it be acceptable to add an ASSERT?

> > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:41
> > +    void startRequest(PassRefPtr<WebCore::UserMediaRequest>);
> 
> Nit: "requestPermission" might be a better name.
> 
> > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:44
> > +    void didReceiveUserMediaPermissionDecision(uint64_t userMediaID, bool allowed);
> 
> Nit: these parameter names aren't necessary.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.h:1192
> > +    UserMediaPermissionRequestManager m_userMediaPermissionRequestManager;
> 
> This will be unused in most clients, why not have a std::unique_ptr<> that is allocated the first time it is used.

Fair points, thanks for the review, I'll update the patch :) And also have a look at writing tests as Benjamin suggested in a previous review.

[Eric Carlson]

(In reply to comment #47)
> (In reply to comment #46)
> > (From update of attachment 233672 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=233672&action=review
> > 
> > > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:41
> > > +    static uint64_t uniqueRequestID = 1;
> > > +    return uniqueRequestID++;
> > 
> > First, 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?
> > 
> 
> I think that so far we haven't paid attention to this issue, this pattern is used elsewhere in WebKit2 (WebFrame, Geolocation, Notifications and others).
> 
> > Second, 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 (when it does wrap around).
> > 
> 
> Well, would it be acceptable to add an ASSERT?
> 

Or just don't return 0.

[Philippe Normand]

Created attachment 237479 [details]
Patch

[Philippe Normand]

Created attachment 237480 [details]
Patch

Rebased againtst ToT

[Philippe Normand]

Comment on attachment 237480 [details]
Patch

I'll split out the GTK bits in a separate patch...

[Philippe Normand]

Created attachment 237482 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 237480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237480&action=review

We need also unit tests for the new GTK+ API.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:42
> + */

Remember to add Since: 2.8 everywhere in the API docs.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:56
> +    gchar* origin;

This should be a CString or GUniquePtr<char>

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:101
> +    g_free(request->priv->origin);

This shouldn't be needed if you use CString or GUniquePtr<char>, also note that dispose can be called multiple times, so you should reset the pointer if you use g_free directly

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:108
> +guint32 webkit_user_media_permission_get_parameters(WebKitUserMediaPermissionRequest* request)

This should be documented.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:120
> + * requesting the permission to accec media device(s).

accec?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:122
> + * Returns: the security origin URI or an empty string.

We usually return NULL instead of empty strings in public API methods.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:124
> +const gchar* webkit_user_media_permission_get_origin(WebKitUserMediaPermissionRequest* request)

Is a string enough to represent a security origin?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:161
> +            0, WEBKIT_PARAM_READABLE));

Use nullptr instead of 0 here.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:189
> +    if (audio->value())
> +        mediaParameters |= WEBKIT_USER_MEDIA_PARAMETERS_AUDIO;
> +    if (video->value())
> +        mediaParameters |= WEBKIT_USER_MEDIA_PARAMETERS_VIDEO;

So parameters is the media type?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:195
> +    if (!origin)

This is not a pointer, I guess you should use origin.isEmpty()

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.cpp:197
> +    String origin = securityOrigin->securityOrigin().toString();
> +    if (!origin)
> +        origin = "";
> +    usermediaPermissionRequest->priv->origin = g_strdup(origin.utf8().data());

Using a CString all this would be just:

usermediaPermissionRequest->priv->origin = securityOrigin->securityOrigin().toString().utf8().

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h:44
> + * @WEBKIT_USER_MEDIA_PARAMETERS_NONE: no media. Shouldn't happen.

If this shouldn't happen we should not expose it in the API.

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h:54
> +  WEBKIT_USER_MEDIA_PARAMETERS_AUDIO = 1 << 0,
> +  WEBKIT_USER_MEDIA_PARAMETERS_VIDEO = 1 << 1

So the media can be audio and video at the same time?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h:55
> +} WebKitUserMediaParameters;

I find it a bit confusing, using Parameters for the media type, or will this be expanded eventually with more "parameters"?

> Source/WebKit2/UIProcess/API/gtk/WebKitUserMediaPermissionRequest.h:77
> +guint32 webkit_user_media_permission_get_parameters(WebKitUserMediaPermissionRequest* request);
> +const gchar* webkit_user_media_permission_get_origin(WebKitUserMediaPermissionRequest* request);

You should use WEBKIT_API and format it correctly, moving the * to the right please, etc.

> Tools/MiniBrowser/gtk/BrowserWindow.c:411
> +    gchar *dialog_message = NULL;
> +    gchar *dialog_message_format = NULL;

These should be const.

> Tools/MiniBrowser/gtk/BrowserWindow.c:422
> +        dialog_title = g_strdup_printf("UserMedia request from %s", origin);

This is leaked, you can use this as const char* for geolocation request and char* for user media. You should use g_strdup() for geolocation and free the string after the gtk_message_dialog_new()

[Eric Carlson]

Comment on attachment 237482 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237482&action=review

This looks good to me modulo the minor nits noted, but I am not a WK2 reviewer.

> LayoutTests/fast/mediastream/error.html:7
> +<script src="script-tests/error.js"></script>

Do you plan to use "error.js" in more than one test? If not, I think it would be much clearer to include the code rather than loading it.

> LayoutTests/fast/mediastream/success.html:7
> +<script src="script-tests/success.js"></script>

Do you plan to use "success.js" in more than one test? If not, I think it would be much clearer to include the code rather than loading it.

> Source/WebKit2/UIProcess/API/C/WKUserMediaPermissionRequest.h:31
> +WK_EXPORT void WKUserMediaPermissionRequestAllow(WKUserMediaPermissionRequestRef userMediaPermissionRequest);
> +WK_EXPORT void WKUserMediaPermissionRequestDeny(WKUserMediaPermissionRequestRef userMediaPermissionRequest);

Nit: the parameter names aren't necessary.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:36
> +    PassRefPtr<UserMediaPermissionRequestProxy> createRequest(uint64_t userMediaID, bool audio, bool video);

Nit: "userMediaID" is unnecessary.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.h:39
> +    void didReceiveUserMediaPermissionDecision(uint64_t, bool allow);

Nit: "allow" is unnecessary.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:43
> +    void didReceiveUserMediaPermissionDecision(uint64_t userMediaID, bool allowed);

Nit: "userMediaID" and "allowed" are unnecessary.

[Philippe Normand]

Thanks Eric for the review! I'll move the js files content to their html pendant :)

About un-needed names, our coding style guide has this recommendation:

"Leave meaningless variable names out of function declarations. A good rule of thumb is if the parameter type name contains the parameter name (without trailing numbers or pluralization), then the parameter name isn't needed. Usually, there should be a parameter name for bools, strings, and numerical types."

[Philippe Normand]

Created attachment 237494 [details]
Patch

[Philippe Normand]

Created attachment 237495 [details]
Patch

[Philippe Normand]

Benjamin, would you have time to review this patch again?

[Benjamin Poulain]

Comment on attachment 237495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237495&action=review

> LayoutTests/ChangeLog:16
> +        * fast/mediastream/error-expected.txt: Added.
> +        * fast/mediastream/error.html: Added.
> +        * fast/mediastream/script-tests/error.js: Added.
> +        * fast/mediastream/script-tests/success.js: Added.
> +        * fast/mediastream/success-expected.txt: Added.
> +        * fast/mediastream/success.html: Added.

It looks to me like you need one a few more tests:

First, two for "pending permission". For example:
-query for UserMedia.
-wait a small amount of time (say 25ms).
-check that it is not granted nor denied.
-grant/deny the permission.

The other kind you need is permission reset. You grant/deny the permission, then reset the permission. For example:
-grand permission for user media
-query userMedia
-reset the permission
--> I am not familiar with mediaStream, but I guess there is a way to handle this and it should be tested.

For Geolocation, only iOS has/tests reset.


It would also be useful to have tests with iframes. E.g.:
-request from an iFrame->pending->grand->request from a second iFrame->no pending, immediately granted if same security origin.
-request from an iFrame->pending->kill the iframe->grant or deny permission->no crash.
-request from different security origins if possible.

> LayoutTests/fast/mediastream/error.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Let's use the HTML5 doctype.

> LayoutTests/fast/mediastream/error.html:26
> +window.jsTestIsAsync = true;

Move this after description(), easier to follow IMHO.

> LayoutTests/fast/mediastream/success.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Same for doctype.

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:74
> +    bool audio() const { return m_audioConstraints; }
> +    bool video() const { return m_videoConstraints; }

Weird names, maybe requiresAudio()/wantsAudio() or something like that?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:34
> +    parametersMap.set("audio", API::Boolean::create(audio));
> +    parametersMap.set("video", API::Boolean::create(video));

ASCIILiteral() for the strings.

[Philippe Normand]

(In reply to comment #59)
> (From update of attachment 237495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=237495&action=review
> 
> > LayoutTests/ChangeLog:16
> > +        * fast/mediastream/error-expected.txt: Added.
> > +        * fast/mediastream/error.html: Added.
> > +        * fast/mediastream/script-tests/error.js: Added.
> > +        * fast/mediastream/script-tests/success.js: Added.
> > +        * fast/mediastream/success-expected.txt: Added.
> > +        * fast/mediastream/success.html: Added.
> 
> It looks to me like you need one a few more tests:
> 
> First, two for "pending permission". For example:
> -query for UserMedia.
> -wait a small amount of time (say 25ms).
> -check that it is not granted nor denied.
> -grant/deny the permission.
> 

Ok.

> The other kind you need is permission reset. You grant/deny the permission, then reset the permission. For example:
> -grand permission for user media
> -query userMedia
> -reset the permission

Does the testRunner needs an API for permission cancellation too? I'm not sure how to proceed about this kind of test.

> --> I am not familiar with mediaStream, but I guess there is a way to handle this and it should be tested.
> 

I need to check the spec about this kind of use-case...

> For Geolocation, only iOS has/tests reset.
> 
> 
> It would also be useful to have tests with iframes. E.g.:
> -request from an iFrame->pending->grand->request from a second iFrame->no pending, immediately granted if same security origin.
> -request from an iFrame->pending->kill the iframe->grant or deny permission->no crash.
> -request from different security origins if possible.
> 

I started working on that but having some issues with CORS :(

Thanks for the quick review, I aknowledged your other comments in the next iteration of the patch.

[Philippe Normand]

(In reply to comment #59)
> 
> The other kind you need is permission reset. You grant/deny the permission, then reset the permission. For example:
> -grand permission for user media
> -query userMedia
> -reset the permission
> --> I am not familiar with mediaStream, but I guess there is a way to handle this and it should be tested.
> 
> For Geolocation, only iOS has/tests reset.
> 

Well after checking the spec again and the UserMediaRequest code in WebCore, cancelRequest() is only invoked when the ScriptExecutionContext is destroyed. Moreover the spec doesn't mention that the permission request can be reset by the user. If the user denies the permission the errback is invoked and if the user grants the permission the callback is invoked.

> 
> It would also be useful to have tests with iframes. E.g.:
> -request from an iFrame->pending->grand->request from a second iFrame->no pending, immediately granted if same security origin.
> -request from an iFrame->pending->kill the iframe->grant or deny permission->no crash.
> -request from different security origins if possible.
> 

I need to get back to these tests :)

[Philippe Normand]

Created attachment 238912 [details]
Patch

[Build Bot]

Comment on attachment 238912 [details]
Patch

Attachment 238912 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5212932023517184

New failing tests:
http/tests/media/media-stream/disconnected-frame-already.html
http/tests/media/media-stream/disconnected-frame.html
http/tests/media/media-stream/disconnected-frame-permission-denied.html

[Build Bot]

Created attachment 238916 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Philippe Normand]

Created attachment 238917 [details]
Patch

Rebased on ToT

[Philippe Normand]

Created attachment 238918 [details]
Patch

Fix all the EWS

[Philippe Normand]

Created attachment 239478 [details]
Patch

Rebased on top of ToT

[Darin Adler]

Comment on attachment 239478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239478&action=review

Enough suggested changes that I think I’m going to say review- for now, but it looks like some steps in the right direction.

> Source/WebCore/Modules/mediastream/UserMediaRequest.cpp:107
> +    return downcast<Document>(m_scriptExecutionContext);

What makes this cast safe? Is there some guarantee this script execution context is a document? If we have that guarantee, why do we store it as a ScriptExecutionContext* instead of a Document*?

> Source/WebCore/Modules/mediastream/UserMediaRequest.h:67
> +    Document* document() const;
> +    Frame* frame() const;

Do we really need to add these helper functions? Mapping from script execution context to Document and Frame seems like something a caller can do on its own.

I think UserMediaPermissionRequestManager::startRequest could do it.

> Source/WebKit2/UIProcess/API/APIUIClient.h:126
> +    virtual bool decidePolicyForUserMediaPermissionRequest(WebKit::WebPageProxy*, WebKit::WebFrameProxy*, WebKit::WebSecurityOrigin*, WebKit::UserMediaPermissionRequestProxy*) { return false; }

I know the old functions use pointers, but for this new function I suggest we use references for any non-optional arguments.

> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:44
> +    m_pendingRequests.add(userMediaID, request.get());

This line of code will be a no-op if there is already a request with this ID in the hash map. What guarantees there is not one? What behavior do we want if there is a number conflict?

> Source/WebKit2/UIProcess/UserMediaPermissionRequestProxy.cpp:34
> +    parametersMap.set(ASCIILiteral("audio"), API::Boolean::create(audio));
> +    parametersMap.set(ASCIILiteral("video"), API::Boolean::create(video));

Should use add here instead of set.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:60
> +    m_idToRequestMap.set(requestID, request);
> +    m_requestToIDMap.set(request, requestID);

These should be add rather than set. Might want to assert there is no conflict with what’s already in the map, or if there is a conflict, consider what behavior we want when there is.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:40
> +    void startRequest(PassRefPtr<WebCore::UserMediaRequest>);

This should take a PassRef rather than a PassRefPtr.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:41
> +    void cancelRequest(WebCore::UserMediaRequest*);

This should take a reference, not a pointer.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:47
> +    typedef HashMap<uint64_t, RefPtr<WebCore::UserMediaRequest>> IDToRequestMap;
> +    typedef HashMap<RefPtr<WebCore::UserMediaRequest>, uint64_t> RequestToIDMap;

I don’t think we need typedefs here.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:49
> +    IDToRequestMap m_idToRequestMap;
> +    RequestToIDMap m_requestToIDMap;

I suggest putting these after the m_page data member.

> Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h:21
> +#define WebUserMediaClient_h
> +#if ENABLE(MEDIA_STREAM)

Should have a blank line here between these two lines.

> Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h:25
> +#include <wtf/PassRefPtr.h>
> +namespace WebKit {

Should have a blank line here between these two lines.

> Source/WebKit2/WebProcess/WebCoreSupport/WebUserMediaClient.h:36
> +    virtual void pageDestroyed();
> +    virtual void requestPermission(PassRefPtr<WebCore::UserMediaRequest>);
> +    virtual void cancelRequest(WebCore::UserMediaRequest*);

Need to use the override keyword here.

> Tools/TestWebKitAPI/Tests/WebKit2/UserMedia.cpp:29
> +using namespace std;

We normally don’t use this.

> Tools/TestWebKitAPI/Tests/WebKit2/UserMedia.cpp:35
> +void decidePolicyForUserMediaPermissionRequestCallBack(WKPageRef page, WKFrameRef frame, WKSecurityOriginRef origin, WKUserMediaPermissionRequestRef permissionRequest, const void* clientInfo)

Should leave out the names of the unused arguments for platforms that compile with that warning on.

> Tools/TestWebKitAPI/Tests/WebKit2/UserMedia.cpp:43
> +    WKRetainPtr<WKContextRef> context(AdoptWK, WKContextCreate());

Please use the adoptWK function rather than the AdoptWK constructor argument:

    auto context = adoptWK(WKContextCreate());

> Tools/TestWebKitAPI/Tests/WebKit2/UserMedia.cpp:53
> +    WKRetainPtr<WKURLRef> url(AdoptWK, Util::createURLForResource("getUserMedia", "html"));

Please use the adoptWK function rather than the AdoptWK constructor argument:

    auto url = adoptWK(Util::createURLForResource("getUserMedia", "html"));

Later, maybe we should change that Util function to return a WKRetainPtr.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:465
> +    WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("SetUserMediaPermission"));
> +    WKRetainPtr<WKBooleanRef> messageBody(AdoptWK, WKBooleanCreate(enabled));
> +    WKBundlePostMessage(m_bundle, messageName.get(), messageBody.get());

Please use the adoptWK function rather than the AdoptWK constructor argument. Could even consider doing this as a one-liner:

    WKBundlePostMessage(m_bundle, adoptWK(WKStringCreateWithUTF8CString("SetUserMediaPermission")).get(), adoptWK(WKBooleanCreate(enabled)));

But it’s OK to use multiple lines. I also suggest using auto for the type rather than WKRetainPtr<WKStringRef> once you are using the adoptWK function.

> Tools/WebKitTestRunner/TestController.cpp:1402
> +    for (size_t i = 0; i < m_userMediaPermissionRequests.size(); ++i) {
> +        WKUserMediaPermissionRequestRef permissionRequest = m_userMediaPermissionRequests[i].get();

Should use a modern for loop:

    for (auto& request : m_userMediaPermissionRequests) {

[Philippe Normand]

Created attachment 239719 [details]
Patch

[Philippe Normand]

Comment on attachment 239478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239478&action=review

Thanks for the review Darin. I updated the patch and here are answers to your questions.

>> Source/WebKit2/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:44
>> +    m_pendingRequests.add(userMediaID, request.get());
> 
> This line of code will be a no-op if there is already a request with this ID in the hash map. What guarantees there is not one? What behavior do we want if there is a number conflict?

The userMediaID is unique, generated from the WebProcess, by the UserMediaPermissionRequestManager (generateRequestID()).

>> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:60
>> +    m_requestToIDMap.set(request, requestID);
> 
> These should be add rather than set. Might want to assert there is no conflict with what’s already in the map, or if there is a conflict, consider what behavior we want when there is.

Well I don't see how there would be a conflict there since the request ID is unique, see the generateRequestID() function.

[Benjamin Poulain]

Comment on attachment 239719 [details]
Patch

I did not get a chance to review this today. I'll try to have a look this weekend.

Did you use those license header intentionally? Those are the old headers without the redistribution parts.

[Philippe Normand]

I used the LGPL headers but they're from last year, I'd be totally fine with using the newer version :)

[Benjamin Poulain]

Comment on attachment 239719 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=239719&action=review

Great work.

> LayoutTests/ChangeLog:9
> +        Simple UserMedia permission request tests inspired from the
> +        Geolocation test suite.

I am a fan of your tests.

> LayoutTests/http/tests/media/media-stream/disconnected-frame-already-expected.txt:1
> +CONSOLE MESSAGE: line 24: NotSupportedError: DOM Exception 9: The implementation did not support the requested type of object or operation.

Is this expected?

> LayoutTests/http/tests/media/media-stream/disconnected-frame-already.html:22
> +    setTimeout(finishTest, 500);

500 seems a bit long, can it be made shorter?

> Source/WebCore/ChangeLog:14
> +        Tests: fast/mediastream/error.html
> +               fast/mediastream/success.html

Need update.

> Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:54
> +    ASSERT(frame);

When you have an assertion followed by a if() branch checking the same thing, it is good to document why the assertion is there.

You should use ASSERT_WITH_MESSAGE(frame, "Explanation about why frame should always be valid here.")

That way, if someone runs into the assertion, she/he can evaluate if the caller is incorrect, or the assertion is invalid.

[Philippe Normand]

Thanks for the review Benjamin! I need to investigate an ASSERT that triggers in the UserMediaClient before landing this. Sorry I've just spotted it today :(

[Philippe Normand]

(In reply to comment #73)
> Comment on attachment 239719 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=239719&action=review
> 
> 
> > LayoutTests/http/tests/media/media-stream/disconnected-frame-already-expected.txt:1
> > +CONSOLE MESSAGE: line 24: NotSupportedError: DOM Exception 9: The implementation did not support the requested type of object or operation.
> 
> Is this expected?
> 

Well this message appears because getUserMedia is invoked for a null frame, after it's been unloaded.

> > LayoutTests/http/tests/media/media-stream/disconnected-frame-already.html:22
> > +    setTimeout(finishTest, 500);
> 
> 500 seems a bit long, can it be made shorter?
> 

100 still works :)

> > Source/WebKit2/WebProcess/MediaStream/UserMediaPermissionRequestManager.cpp:54
> > +    ASSERT(frame);
> 
> When you have an assertion followed by a if() branch checking the same
> thing, it is good to document why the assertion is there.
> 
> You should use ASSERT_WITH_MESSAGE(frame, "Explanation about why frame
> should always be valid here.")
> 

This ASSERT hits on debug builds for some of the mediastream tests though. This is because getUserMedia is called on a disconnected frame, I'm not sure it's right to ASSERT in this case. A console message is emitted and the permission request is denied in this case.

[Philippe Normand]

Created attachment 241348 [details]
Patch for landing

[Philippe Normand]

Committed r176011: <http://trac.webkit.org/changeset/176011>

[Philippe Normand]

(In reply to comment #77)
> Committed r176011: <http://trac.webkit.org/changeset/176011>

Badly screwed up commit message :( Sorry about that.