[Webkit-unassigned] [Bug 123158] [WK2] UserMediaClient support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=123158





--- Comment #7 from Darin Adler <darin at apple.com>  2013-10-25 13:00:17 PST ---
(From update of attachment 215183)
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*.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.