[webkit-reviews] review denied: [Bug 76593] Merge WebGraphicsContext3D creation and initialization : [Attachment 123167] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Antoine Labour
<piman at chromium.org>'s request for review:
Bug 76593: Merge WebGraphicsContext3D creation and initialization
https://bugs.webkit.org/show_bug.cgi?id=76593

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

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


> Source/WebKit/chromium/public/WebViewClient.h:114
> +    virtual WebGraphicsContext3D*
createGraphicsContext3D(WebGraphicsContext3D::Attributes, WebView*, bool
renderDirectlyToWebView) { return 0; }

nit: no need for the WebView parameter since the WebViewClient knows what
WebView it is associated with.	(I presume you don't need to ask a RenderWidget
to create a WGC3D for some other RenderWidget.)

What does renderDirectlyToWebView mean?  Maybe you could document that a bit?

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:159
> +    OwnPtr<WebKit::WebGraphicsContext3D> webContext =
adoptPtr(webViewImpl->client()->createGraphicsContext3D(webAttributes,
webViewImpl, renderDirectlyToHostWindow));

In the future, we'll probably want to bridge up through an interface like
HostWindow, but that doesn't have to happen now.  You see,
GraphicsContext3DChromium.cpp is technically part of WebCore/platform/, and as
such it should not have direct access to the WebViewImpl either.  But, please
don't try to fix this in this patch :-)

Otherwise, LGTM