[webkit-reviews] review denied: [Bug 73130] [chromium] MediaStream API: Adding the embedding code for MediaStreamCenter : [Attachment 117189] 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 73130: [chromium] MediaStream API: Adding the embedding code for
MediaStreamCenter
https://bugs.webkit.org/show_bug.cgi?id=73130

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

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


> Source/WebKit/chromium/public/platform/WebKitPlatformSupport.h:339
> +    virtual WebMediaStreamCenter*
mediaStreamCenter(WebMediaStreamCenterClient*) { return 0; }

you are passing a Client to this function, which suggests that this is a
factory method.
factory methods should have the 'create' prefix, so it is clear to the caller
that they
are getting an instance object that they will need to remember to delete.

> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:44
> +    virtual void didSetMediaStreamTrackEnabled(const
WebMediaStreamDescriptor&, size_t trackIndex) = 0;

I'm having a hard time imagining how didSetMediaStreamTrackEnabled,
didStopLocalMediaStream and
WebMediaStreamCenterClient::endLocalMediaStream relate to one another.	It is
not clear what these
functions mean either.

> Source/WebKit/chromium/public/platform/WebMediaStreamDescriptor.h:55
> +    WebMediaStreamDescriptor(WebCore::MediaStreamDescriptor*);

usually the PassRefPtr variant is enough.  do we really need this one too?

(same question applies to the conversion operators below)

> Source/WebKit/chromium/public/platform/WebMediaStreamSourcesQueryClient.h:46
> +class WebMediaStreamSourcesQueryClient {

"Client" is usually used when naming interfaces.  This class appears to be more

like WebUserMediaRequest.  It provides readonly properties that customize the
request and it has the method the embedder should call to complete the request.

I don't think *Client is the right name for this class.

> Source/WebKit/chromium/public/platform/WebMediaStreamSourcesQueryClient.h:57
> +    WEBKIT_EXPORT void mediaStreamSourcesQueryCompleted(const
WebVector<WebMediaStreamSource>&);

can the query fail?  WebUserMediaRequest has methods for success and failure. 
we don't need that here?