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

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied  review:
Bug 58550: [Chromium] Media Stream API: add the Chromium WebKit interfaces
https://bugs.webkit.org/show_bug.cgi?id=58550

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

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


Looks like we're getting close.  (Of course, we'll need to go through API
review also since you've included changes to WebKit/chromium/public in here
too.)  Also, this needs to build on GTK before we can land it.

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

This should call PeerHandler::create.  That's the point of having a separate
"base".

> Source/WebCore/mediastream/PeerConnection.h:131
> -    OwnPtr<PeerHandler> m_peerHandler;
> +    OwnPtr<PeerHandlerBase> m_peerHandler;

This should hold a PeerHandle.

If you look at the folks outside of the platform layer that interact with
ResourceHandle, you'll see that they all use ResourceHandle as the type, not
ResourceHandleBase.

> Source/WebCore/platform/mediastream/PeerHandlerBase.h:57
> +    virtual void produceInitialOffer(const MediaStreamDescriptorVector&
pendingAddStreams) = 0;
> +    virtual void handleInitialOffer(const String& sdp) = 0;
> +    virtual void processSDP(const String& sdp) = 0;
> +    virtual void processPendingStreams(const MediaStreamDescriptorVector&
pendingAddStreams, const MediaStreamDescriptorVector& pendingRemoveStreams) =
0;
> +    virtual void sendDataStreamMessage(const char* data, unsigned length) =
0;
> +
> +    virtual void stop() = 0;

I thought we agreed that these would not be virtual.

> Source/WebKit/chromium/bridge/PeerHandlerChromium.cpp:46
> +class PeerHandlerChromium : public WebCore::PeerHandlerBase {

PeerHandlerChromium should just be called PeerHandler.	The declaration should
live in WebCore/platform/mediastream/chromium/PeerHandle.h.

> Source/WebKit/chromium/bridge/PeerHandlerChromium.cpp:62
> +PeerHandlerChromium::PeerHandlerChromium(WebCore::PeerHandlerClient* client,
const String& serverConfiguration, const String& securityOrigin)

All this code should be in the WebCore namespace.