[webkit-reviews] review denied: [Bug 71678] [chromium] MediaStream API: Adding embedding code for GetUserMedia : [Attachment 115579] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 71678: [chromium] MediaStream API: Adding embedding code for GetUserMedia
https://bugs.webkit.org/show_bug.cgi?id=71678

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=115579&action=review


> Source/WebKit/chromium/public/WebUserMediaClient.h:28
> +#include "WebVector.h"

nit: please just forward declare WebVector (see for example
WebPageSerializer.h)

> Source/WebKit/chromium/public/WebUserMediaClient.h:39
> +    virtual void pageDestroyed() = 0;

"page" (referring to WebCore::Page) is also a name that doesn't exist in the
WebKit API.

I think a name like "cancelAllUserMediaRequests" might be better here.	It is
much more specific.

> Source/WebKit/chromium/public/WebUserMediaRequest.h:49
> +    WEBKIT_EXPORT WebUserMediaRequest& operator=(const
WebUserMediaRequest&);

as with other interface, please define an assign method, and implement
operator=
in terms of that.

> Source/WebKit/chromium/public/WebUserMediaRequest.h:60
> +    WEBKIT_EXPORT void requestSucceeded(const
WebVector<WebMediaStreamSource>&);

There was some discussion (over email) about making requestSucceeded and
requestFailed
virtual to support mocking in unit tests.  I commented that I thought perhaps
that
suggested that these functions really don't belong here on the 'request'
object.

Compare this to WebURLRequest, which just describes the parameters of a
request.  It
is just data.

> Source/WebKit/chromium/public/WebUserMediaRequest.h:63
> +    WEBKIT_EXPORT bool operator==(const WebUserMediaRequest&) const;

nit: we don't export operators in the WebKit API.  Please add an equals method,

and then define an inline operator== that calls the equals method.  See
WebNode.h
for example.

> Source/WebKit/chromium/public/WebUserMediaRequest.h:66
> +    WebUserMediaRequest(WebCore::UserMediaRequest*);

nit: do you really need both forms of constructor and both forms of operator
here?
we don't elsewhere in the API.

> Source/WebKit/chromium/src/WebUserMediaRequest.cpp:102
> +    m_private->succeed(s);

I'm trying to study what happens when succeed and fail are called, but I don't
see
UserMediaRequest checked into the tree.  Is there a separate bug with that
patch?

I'm a little concerned about the idea that UserMediaRequest is both data
representing
the request and controller code for what happens when the request succeeds or
fails.