[webkit-reviews] review denied: [Bug 102339] Provide page/window coordinates to plugin's local coordinates translation in WebPluginContainer. : [Attachment 174527] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 15 15:10:07 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 174527: Patch
https://bugs.webkit.org/attachment.cgi?id=174527&action=review
------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=174527&action=review
> Source/WebKit/chromium/public/WebElement.h:86
> + WEBKIT_EXPORT WebPluginContainerImpl* pluginContainerFromElement()
const;
API functions should return the API type. That means this should return a
WebPluginContainer* not a WebPluginContainerImpl*.
> Source/WebKit/chromium/src/WebFrameImpl.cpp:1217
> - pluginContainer = pluginContainerFromNode(node);
> + pluginContainer =
node.toConst<WebElement>().pluginContainerFromElement();
How do you know that this cast is safe? Maybe pluginContainerFromElement would
be better on WebNode after all... Also, you'll probably want to call it just
"pluginContainer". It's obviously from a node (or an element) if its a member
function.
> Source/WebKit/chromium/tests/FakeWebPlugin.cpp:64
> +const WebString& FakeWebPlugin::mimeType()
> +{
> + DEFINE_STATIC_LOCAL(const WebString, kTestWebPluginMimeType,
(WebString::fromUTF8("application/x-webkit-test-webplugin")));
> + return kTestWebPluginMimeType;
> +}
Why are you creating a static for this? I would just create a temporary
WebString in the one place where you need it.
> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:80
> +WebPluginContainerImpl* getWebPluginContainer(WebView* webView, const
WebString& id)
WebPluginContainerImpl -> WebPluginContainer. You seem to only need the API
type. You should use the API type.
More information about the webkit-reviews
mailing list