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

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


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

since mediaStreamCenter is a getter, it seems like you should be able to call
it
multiple times and get the same WebMediaStreamCenter instance back.  however,
to
call this function you have to pass a Client.  what if you pass a different
Client each time?  does the Client on the shared WebMediaStreamCenter change?

my point is that you are using an atypical pattern here.  usually, getters
like this do not take parameters (e.g., see the clipboard() method).  on the
other hand, create* functions do take parameters (e.g., see the
createLocalStorageNamespace function).

a couple options:

1)  Rename to createMediaStreamCenter and have WebKit own the
WebMediaStreamCenter.
2)  Replace WebMediaStreamCenterClient::stopLocalMediaStream() with a method on

WebMediaStreamDescriptor (e.g., a stop() method) or a static method on some
other
class.	Yeah, looking at MediaStreamCenter::endLocalMediaStream(), I see that
it
does not rely on any member variables of MediaStreamCenter.  it just operates
on
the descriptor.

> Source/WebKit/chromium/public/platform/WebMediaStreamCenter.h:42
> +    virtual void queryMediaStreamSources(const
WebMediaStreamSourcesQueryClient&) = 0;

is this supposed to be WebMediaStreamSourcesRequest instead?

> Source/WebKit/chromium/public/platform/WebMediaStreamComponent.h:56
> +    WEBKIT_EXPORT bool enabled() const;

nit: rename to "isEnabled" since it is asking a question

> Source/WebKit/chromium/src/WebMediaStreamSourcesRequest.cpp:59
> +    if (!isNull())

would the WebMediaStreamSourcesRequest passed to WebMediaStreamCenter ever
really be null?
can you instead just assert that it is never null when
audio/video/didCompleteQuery methods
are called?  or, is it possible for it to later become null?

> Source/WebKit/chromium/src/WebMediaStreamSourcesRequest.cpp:73
> +    if (!isNull()) {

nit: if this is really needed, then early return instead so that the bulk of
the function can have less indentation.