[webkit-reviews] review denied: [Bug 97673] MediaStream API: Allow RTCPeerConnection to pass down its Document reference to the embedder : [Attachment 165786] 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 97673: MediaStream API: Allow RTCPeerConnection to pass down its Document
reference to the embedder
https://bugs.webkit.org/show_bug.cgi?id=97673

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

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


> Source/Platform/chromium/public/WebRTCPeerConnectionHandler.h:35
> +class WebDocument;

You can't reference WebDocument in Source/Platform.  Document is a DOM-level
concept, not a Platform-level concept.

> Source/WebCore/platform/mediastream/RTCPeerConnectionHandler.cpp:50
> -    virtual bool initialize(PassRefPtr<RTCConfiguration>,
PassRefPtr<MediaConstraints>) OVERRIDE;
> +    virtual bool initialize(Document*, PassRefPtr<RTCConfiguration>,
PassRefPtr<MediaConstraints>) OVERRIDE;

This is also a layering violation.  Code in WebCore/platform shouldn't know
anything about Document.

> Source/WebKit/chromium/src/RTCPeerConnectionHandlerChromium.cpp:249
> +namespace WebCore {
> +
> +PassOwnPtr<RTCPeerConnectionHandler>
RTCPeerConnectionHandler::create(RTCPeerConnectionHandlerClient* client)
> +{
> +    return adoptPtr(new WebKit::RTCPeerConnectionHandlerChromium(client));
> +}
> +
> +} // namespace WebCore

This is wrong.	We shouldn't have code in Source/WebKit/chromium that's in the
WebCore namespace.  I know other features do this, but we're trying to unwind
all these layering violations so we can split WebCore and WebKit into separate
DLLs.