[webkit-reviews] review denied: [Bug 58550] [Chromium] Media Stream API: add the Chromium WebKit interfaces : [Attachment 112181] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 58550: [Chromium] Media Stream API: add the Chromium WebKit interfaces
https://bugs.webkit.org/show_bug.cgi?id=58550

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=112181&action=review


> Source/WebCore/ChangeLog:10
> +	   No new tests. (OOPS!)

We'll need to remove this line from the ChangeLog in order to land this patch. 
Generally it's a good idea to fill out the ChangeLog with an explanation of why
you're making this change.

> Source/WebCore/mediastream/PeerConnection.cpp:58
> -    , m_peerHandler(PeerHandler::create(this, serverConfiguration,
context->securityOrigin()->toString()))
> +    , m_peerHandler(adoptPtr(PeerHandler::create(this, serverConfiguration,
context->securityOrigin()->toString())))

This is wrong.	PeerHandler::create should call adoptPtr.

> Source/WebCore/mediastream/PeerConnection.cpp:224
> -void PeerConnection::remoteStreamRemoved(MediaStreamDescriptor*
streamDescriptor)
> +void PeerConnection::remoteStreamRemoved(PassRefPtr<MediaStreamDescriptor>
prpStreamDescriptor)
>  {
> +    RefPtr<MediaStreamDescriptor> streamDescriptor = prpStreamDescriptor;

Why did you make this change?  This function does not appear to take ownership
of streamDescriptor.

> Source/WebCore/platform/mediastream/PeerHandler.h:45
>  class PeerHandler {

This shouldn't be a virtual interface.	Any given compilation of WebKit should
only have one implementation of PeerHandler, which means this doesn't need to
be virtual.  We just need to link in different CPP files for different
configurations.

> Source/WebKit/chromium/public/WebMediaStreamDescriptor.h:43
> +    WEBKIT_EXPORT void initialize(); // FIXME Add params.

You need a : after "FIXME".

> Source/WebKit/chromium/src/PeerHandlerChromium.cpp:46
> +class PeerHandlerChromium : public WebCore::PeerHandler {

This class should be in WebCore/platform and should use PlatformSupport to
reach WebKit/chromium/src.

> Source/WebKit/chromium/src/PeerHandlerChromium.cpp:122
> +namespace WebCore {
> +
> +PeerHandler* PeerHandler::create(PeerHandlerClient* client, const String&
serverConfiguration, const String& securityOrigin)
> +{
> +    return new WebKit::PeerHandlerChromium(client, serverConfiguration,
securityOrigin);
> +}
> +
> +} // namespace WebCore

WebKit/chromium/src cannot contain code in the WebCore namespace.