[webkit-reviews] review denied: [Bug 102339] Provide page/window coordinates to plugin's local coordinates translation in WebPluginContainer. : [Attachment 174501] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 15 13:16:13 PST 2012
Adam Barth <abarth at webkit.org> has denied lazyboy at chromium.org's request for
review:
Bug 102339: Provide page/window coordinates to plugin's local coordinates
translation in WebPluginContainer.
https://bugs.webkit.org/show_bug.cgi?id=102339
Attachment 174501: Patch
https://bugs.webkit.org/attachment.cgi?id=174501&action=review
------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=174501&action=review
This patch is looking good. Some minor details.
> Source/WebKit/chromium/src/WebFrameImpl.cpp:307
> -static WebPluginContainerImpl* pluginContainerFromNode(const WebNode& node)
> +WebPluginContainerImpl* WebFrameImpl::pluginContainerFromNode(const WebNode&
node)
Should this be a function on WebNode?
> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:74
> + if (params.mimeType == WebKit::FakeWebPlugin::mimeType())
No need for "WebKit::". We're already in the WebKit namespace.
> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:84
> +static WebFrameClient* pluginWebFrameClient()
> +{
> + DEFINE_STATIC_LOCAL(TestPluginWebFrameClient, pluginClient, ());
> + return &pluginClient;
> +}
Why is this static? We should just create one on the stack for each test.
> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:89
> + return WebFrameImpl::pluginContainerFromNode(element);
Yeah, moving pluginContainerFromNode to WebElement might be the best thing.
> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:97
> + static_cast<WebViewImpl*>(webView)->settings()->setPluginsEnabled(true);
Why WebViewImpl? You should be able to do all this this through the API.
> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:102
> + WebPluginContainerImpl* pluginContainerOne =
getWebPluginContainer(webView, WebString::fromUTF8("translated-plugin"));
Why WebPluginContainerImpl? You should be able to do all of this though the
API.
More information about the webkit-reviews
mailing list