[webkit-reviews] review denied: [Bug 123158] [WK2] UserMediaClient support : [Attachment 239478] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 123158: [WK2] UserMediaClient support
https://bugs.webkit.org/show_bug.cgi?id=123158

Attachment 239478: Patch
https://bugs.webkit.org/attachment.cgi?id=239478&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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) {