Bug 123158 - [WK2] UserMediaClient support
Summary: [WK2] UserMediaClient support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
: 132855 (view as bug list)
Depends on:
Blocks: 79203 124288 136449
  Show dependency treegraph
 
Reported: 2013-10-22 06:09 PDT by Philippe Normand
Modified: 2014-11-12 00:38 PST (History)
33 users (show)

See Also:


Attachments
[wk2] usermediaclient (67.23 KB, patch)
2013-10-25 09:27 PDT, Philippe Normand
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
[wk2] usermediaclient (69.43 KB, patch)
2013-10-26 03:03 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
[wk2] usermediaclient (69.32 KB, patch)
2013-10-26 03:21 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (81.28 KB, patch)
2013-10-26 09:44 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
wk2 usermediaclient (84.31 KB, patch)
2013-10-29 03:22 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
wk2 usermediaclient (84.31 KB, patch)
2013-10-29 03:29 PDT, Philippe Normand
buildbot: commit-queue-
Details | Formatted Diff | Diff
wk2 usermediaclient (84.37 KB, patch)
2013-10-29 05:25 PDT, Philippe Normand
benjamin: review-
Details | Formatted Diff | Diff
Patch (65.49 KB, patch)
2014-05-01 11:35 PDT, Seongjun Kim
no flags Details | Formatted Diff | Diff
Fix style (65.52 KB, patch)
2014-05-01 11:48 PDT, Seongjun Kim
no flags Details | Formatted Diff | Diff
Patch (67.40 KB, patch)
2014-05-12 04:53 PDT, Seongjun Kim
no flags Details | Formatted Diff | Diff
Patch (67.37 KB, patch)
2014-06-09 02:01 PDT, Seongjun Kim
no flags Details | Formatted Diff | Diff
Patch (67.41 KB, patch)
2014-06-23 04:26 PDT, Seongjun Kim
no flags Details | Formatted Diff | Diff
Patch (67.37 KB, patch)
2014-06-23 20:06 PDT, Seongjun Kim
no flags Details | Formatted Diff | Diff
Patch (116.00 KB, patch)
2014-09-02 00:48 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (116.00 KB, patch)
2014-09-02 01:07 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (92.52 KB, patch)
2014-09-02 03:09 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (91.63 KB, patch)
2014-09-02 09:22 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (91.64 KB, patch)
2014-09-02 09:24 PDT, Philippe Normand
benjamin: review-
Details | Formatted Diff | Diff
Patch (110.86 KB, patch)
2014-09-30 00:08 PDT, Philippe Normand
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (503.55 KB, application/zip)
2014-09-30 01:16 PDT, Build Bot
no flags Details
Patch (110.83 KB, patch)
2014-09-30 01:17 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (112.18 KB, patch)
2014-09-30 02:43 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (112.27 KB, patch)
2014-10-08 09:17 PDT, Philippe Normand
darin: review-
Details | Formatted Diff | Diff
Patch (115.98 KB, patch)
2014-10-13 02:22 PDT, Philippe Normand
benjamin: review+
benjamin: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (116.38 KB, patch)
2014-11-11 04:00 PST, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2013-10-22 06:09:26 PDT
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.
Comment 1 Philippe Normand 2013-10-25 09:27:52 PDT
Created attachment 215183 [details]
[wk2] usermediaclient

First shot for EWS.
Comment 2 WebKit Commit Bot 2013-10-25 09:29:54 PDT
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
Comment 3 WebKit Commit Bot 2013-10-25 09:30:07 PDT
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.
Comment 4 EFL EWS Bot 2013-10-25 09:35:52 PDT
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
Comment 5 Build Bot 2013-10-25 09:43:19 PDT
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
Comment 6 Build Bot 2013-10-25 10:11:15 PDT
Comment on attachment 215183 [details]
[wk2] usermediaclient

Attachment 215183 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/10918081
Comment 7 Darin Adler 2013-10-25 13:00:17 PDT
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*.
Comment 8 Philippe Normand 2013-10-26 02:33:58 PDT
Thanks for the review, Darin! I will upload new versions of the patch to make EWS happy and address your comments as well.
Comment 9 Philippe Normand 2013-10-26 03:03:10 PDT
Created attachment 215243 [details]
[wk2] usermediaclient
Comment 10 WebKit Commit Bot 2013-10-26 03:05:36 PDT
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.
Comment 11 Philippe Normand 2013-10-26 03:21:19 PDT
Created attachment 215244 [details]
[wk2] usermediaclient
Comment 12 WebKit Commit Bot 2013-10-26 03:24:47 PDT
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.
Comment 13 Build Bot 2013-10-26 03:55:18 PDT
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
Comment 14 Carlos Garcia Campos 2013-10-26 03:58:49 PDT
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.
Comment 15 Build Bot 2013-10-26 04:02:52 PDT
Comment on attachment 215244 [details]
[wk2] usermediaclient

Attachment 215244 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/11878102
Comment 16 Philippe Normand 2013-10-26 09:44:08 PDT
Created attachment 215252 [details]
Patch
Comment 17 Philippe Normand 2013-10-26 09:46:07 PDT
This should make EWS happy but not addressing review comments yet.
Comment 18 Philippe Normand 2013-10-28 10:49:49 PDT
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.
Comment 19 Philippe Normand 2013-10-28 10:51:37 PDT
Comment on attachment 215252 [details]
Patch

I'll upload the new version soon.
Comment 20 Philippe Normand 2013-10-29 03:22:43 PDT
Created attachment 215373 [details]
wk2 usermediaclient
Comment 21 WebKit Commit Bot 2013-10-29 03:25:14 PDT
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.
Comment 22 Philippe Normand 2013-10-29 03:29:01 PDT
Created attachment 215375 [details]
wk2 usermediaclient
Comment 23 Build Bot 2013-10-29 03:58:31 PDT
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
Comment 24 Build Bot 2013-10-29 04:13:36 PDT
Comment on attachment 215375 [details]
wk2 usermediaclient

Attachment 215375 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/17148046
Comment 25 Philippe Normand 2013-10-29 05:25:27 PDT
Created attachment 215378 [details]
wk2 usermediaclient
Comment 26 Benjamin Poulain 2013-11-05 15:42:46 PST
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.
Comment 27 Carlos Garcia Campos 2013-11-05 23:43:37 PST
(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.
Comment 28 Philippe Normand 2013-11-06 09:01:09 PST
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.
Comment 29 Eric Carlson 2013-11-06 10:33:06 PST
(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
Comment 30 Seongjun Kim 2014-05-01 11:35:10 PDT
Created attachment 230597 [details]
Patch

Patch based on work previously done by Philippe Normand

Commit log is not yet..
Comment 31 WebKit Commit Bot 2014-05-01 11:37:29 PDT
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.
Comment 32 Seongjun Kim 2014-05-01 11:48:17 PDT
Created attachment 230599 [details]
Fix style
Comment 33 Philippe Normand 2014-05-09 06:42:34 PDT
I wanted to try the patch but it seems like it already got bitrotten, would you mind rebasing it if possible?
Comment 34 Seongjun Kim 2014-05-11 19:13:05 PDT
(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.
Comment 35 Seongjun Kim 2014-05-12 04:53:07 PDT
Created attachment 231286 [details]
Patch
Comment 36 Philippe Normand 2014-06-06 00:06:53 PDT
It'd be nice to have green EWS so we can ping Benjamin about a review. Can you please rebase the patch against ToT?
Comment 37 Seongjun Kim 2014-06-06 05:33:15 PDT
(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.
Comment 38 Seongjun Kim 2014-06-09 02:01:47 PDT
Created attachment 232699 [details]
Patch
Comment 39 Philippe Normand 2014-06-10 03:45:38 PDT
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
Comment 40 Praveen Jadhav 2014-06-22 20:19:29 PDT
*** Bug 132855 has been marked as a duplicate of this bug. ***
Comment 41 Seongjun Kim 2014-06-23 04:26:40 PDT
Created attachment 233596 [details]
Patch
Comment 42 Seongjun Kim 2014-06-23 04:35:19 PDT
I rebased the patch and applied Philippe's comment.
Comment 43 Philippe Normand 2014-06-23 08:42:08 PDT
/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.
Comment 44 Seongjun Kim 2014-06-23 20:06:45 PDT
Created attachment 233672 [details]
Patch
Comment 45 Seongjun Kim 2014-06-23 20:08:26 PDT
(In reply to comment #44)
> Created an attachment (id=233672) [details]
> Patch

Patch updated to resolve --no-media-stream build errors.
Comment 46 Eric Carlson 2014-06-24 07:01:55 PDT
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.
Comment 47 Philippe Normand 2014-08-25 03:58:27 PDT
(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.
Comment 48 Eric Carlson 2014-08-26 10:42:08 PDT
(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.
Comment 49 Philippe Normand 2014-09-02 00:48:12 PDT
Created attachment 237479 [details]
Patch
Comment 50 Philippe Normand 2014-09-02 01:07:19 PDT
Created attachment 237480 [details]
Patch

Rebased againtst ToT
Comment 51 Philippe Normand 2014-09-02 02:43:04 PDT
Comment on attachment 237480 [details]
Patch

I'll split out the GTK bits in a separate patch...
Comment 52 Philippe Normand 2014-09-02 03:09:41 PDT
Created attachment 237482 [details]
Patch
Comment 53 Carlos Garcia Campos 2014-09-02 03:21:17 PDT
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()
Comment 54 Eric Carlson 2014-09-02 08:33:01 PDT
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.
Comment 55 Philippe Normand 2014-09-02 09:00:30 PDT
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."
Comment 56 Philippe Normand 2014-09-02 09:22:24 PDT
Created attachment 237494 [details]
Patch
Comment 57 Philippe Normand 2014-09-02 09:24:53 PDT
Created attachment 237495 [details]
Patch
Comment 58 Philippe Normand 2014-09-04 06:55:49 PDT
Benjamin, would you have time to review this patch again?
Comment 59 Benjamin Poulain 2014-09-16 15:52:27 PDT
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.
Comment 60 Philippe Normand 2014-09-17 04:41:19 PDT
(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.
Comment 61 Philippe Normand 2014-09-29 04:09:46 PDT
(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 :)
Comment 62 Philippe Normand 2014-09-30 00:08:48 PDT
Created attachment 238912 [details]
Patch
Comment 63 Build Bot 2014-09-30 01:16:31 PDT
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
Comment 64 Build Bot 2014-09-30 01:16:41 PDT
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
Comment 65 Philippe Normand 2014-09-30 01:17:21 PDT
Created attachment 238917 [details]
Patch

Rebased on ToT
Comment 66 Philippe Normand 2014-09-30 02:43:27 PDT
Created attachment 238918 [details]
Patch

Fix all the EWS
Comment 67 Philippe Normand 2014-10-08 09:17:32 PDT
Created attachment 239478 [details]
Patch

Rebased on top of ToT
Comment 68 Darin Adler 2014-10-08 12:01:29 PDT
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) {
Comment 69 Philippe Normand 2014-10-13 02:22:40 PDT
Created attachment 239719 [details]
Patch
Comment 70 Philippe Normand 2014-10-13 02:24:31 PDT
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.
Comment 71 Benjamin Poulain 2014-10-31 21:40:31 PDT
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.
Comment 72 Philippe Normand 2014-11-01 02:03:50 PDT
I used the LGPL headers but they're from last year, I'd be totally fine with using the newer version :)
Comment 73 Benjamin Poulain 2014-11-03 18:30:00 PST
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.
Comment 74 Philippe Normand 2014-11-10 05:29:03 PST
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 :(
Comment 75 Philippe Normand 2014-11-11 02:55:41 PST
(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.
Comment 76 Philippe Normand 2014-11-11 04:00:31 PST
Created attachment 241348 [details]
Patch for landing
Comment 77 Philippe Normand 2014-11-12 00:26:10 PST
Committed r176011: <http://trac.webkit.org/changeset/176011>
Comment 78 Philippe Normand 2014-11-12 00:38:15 PST
(In reply to comment #77)
> Committed r176011: <http://trac.webkit.org/changeset/176011>

Badly screwed up commit message :( Sorry about that.