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

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


> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:41
> +    virtual ~WebMediaStreamCenter() { }

It looks like you intend the user of this interface to delete the object
through this interface.
If so, then WebKitPlatformSupport::mediaStreamCenter() is mis-named.  It should
be prefixed with
"create".  Otherwise, this interface should have its destructor hidden (made
protected).

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

nit: perhaps this function could just be named didEnableMediaStreamTrack?

> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:45
> +    virtual void didStopLocalMediaStream(const WebMediaStreamDescriptor&) =
0;

nit: this function says "Local" in its name, but the parameter type does not
have "Local" in its name.  perhaps this function should just be called
didStopMediaStream?

> Source/WebKit/chromium/public/platform/WebMediaStreamCenterClient.h:42
> +    virtual void endLocalMediaStream(const WebMediaStreamDescriptor&) = 0;

nit: here you use the term "end" but on WebMediaStreamCenter, I see the term
"stop".
Is there a relationship between ending a stream and it being reported as
stopped?
Should we try to use the same term here if they are related?

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

nit: are you sure you need both constructors?

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

nit: It seems redundant to include the class name in this method.  This method
is already scoped to a class that has in its name "media stream sources query",

so why repeat that here?  it seems like you could just call this method
didComplete.

WebUserMediaRequest is fairly similar to this interface.  In that case, we
used the term "request" in the method name, which was slightly redundant
but seemed to work nicely.  So, following that we could call this method here
queryCompleted or didCompleteQuery.

I'm also curious about the name of this class.	Is it really a Client?	Or, is
it perhaps more of a Request interface along the lines of WebUserMediaRequest?
Should we call this WebMediaStreamSourcesRequest?  Is the WebCore version
similarly mis-named?  it would be nice to be consistent with how things are
named.