UNCONFIRMED 102339
Provide page/window coordinates to plugin's local coordinates translation in WebPluginContainer.
https://bugs.webkit.org/show_bug.cgi?id=102339
Summary Provide page/window coordinates to plugin's local coordinates translation in ...
lazyboy
Reported 2012-11-14 23:02:06 PST
Provide page/window coordinates to plugin's local coordinates translation in WebPluginContainer.
Attachments
Patch (19.73 KB, patch)
2012-11-14 23:02 PST, lazyboy
no flags
Patch (21.97 KB, patch)
2012-11-15 12:52 PST, lazyboy
no flags
Patch (25.40 KB, patch)
2012-11-15 15:01 PST, lazyboy
no flags
Patch (25.30 KB, patch)
2012-11-15 16:15 PST, lazyboy
no flags
Patch (23.91 KB, patch)
2012-11-15 18:19 PST, lazyboy
no flags
Patch (24.06 KB, patch)
2012-11-16 13:28 PST, lazyboy
no flags
Patch (24.02 KB, patch)
2012-11-19 19:21 PST, lazyboy
abarth: review+
Patch, sync so it applies correctly (23.96 KB, patch)
2012-11-20 11:27 PST, lazyboy
no flags
lazyboy
Comment 1 2012-11-14 23:02:56 PST
WebKit Review Bot
Comment 2 2012-11-14 23:06:44 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
WebKit Review Bot
Comment 3 2012-11-14 23:07:05 PST
Attachment 174357 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:33: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 4 2012-11-15 00:07:23 PST
Comment on attachment 174357 [details] Patch Attachment 174357 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14833626 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Adam Barth
Comment 5 2012-11-15 10:28:11 PST
Comment on attachment 174357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174357&action=review > Source/WebKit/chromium/ChangeLog:8 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). Please fill out this part of the ChangeLog. > Source/WebKit/chromium/tests/FrameTestHelpers.cpp:60 > + virtual WebPlugin* createPlugin(WebFrame* frame, const WebPluginParams& params) OVERRIDE We shouldn't need to add this to every TestWebFrameClient. The tests in WebPluginContainerTest.cpp should create a new subclass of TestWebFrameClient that adds this override. > Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:66 > +WebPluginContainerImpl* getWebPluginContainer(WebView* webView, const WebString& id) This function dives too much into WebCore. We should be able to write these tests in terms of the WebKit API, not in terms of WebCore implementation details.
lazyboy
Comment 6 2012-11-15 12:52:50 PST
lazyboy
Comment 7 2012-11-15 12:58:56 PST
Comment on attachment 174357 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174357&action=review >> Source/WebKit/chromium/ChangeLog:8 >> + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > > Please fill out this part of the ChangeLog. Done. >> Source/WebKit/chromium/tests/FrameTestHelpers.cpp:60 >> + virtual WebPlugin* createPlugin(WebFrame* frame, const WebPluginParams& params) OVERRIDE > > We shouldn't need to add this to every TestWebFrameClient. The tests in WebPluginContainerTest.cpp should create a new subclass of TestWebFrameClient that adds this override. Done. >> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:66 >> +WebPluginContainerImpl* getWebPluginContainer(WebView* webView, const WebString& id) > > This function dives too much into WebCore. We should be able to write these tests in terms of the WebKit API, not in terms of WebCore implementation details. I'm using WebFrameImpl::pluginContainerFromNode(WebNode) now, so no more WebCore here, but also means I had to expose that static function in WebFrameImpl.
Adam Barth
Comment 8 2012-11-15 13:18:03 PST
Comment on attachment 174501 [details] Patch 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.
lazyboy
Comment 9 2012-11-15 15:01:05 PST
lazyboy
Comment 10 2012-11-15 15:02:29 PST
Comment on attachment 174501 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174501&action=review >> Source/WebKit/chromium/src/WebFrameImpl.cpp:307 >> +WebPluginContainerImpl* WebFrameImpl::pluginContainerFromNode(const WebNode& node) > > Should this be a function on WebNode? Moved the function to WebElement (non-static). Calling it pluginContainerFromElement(), "FromElement" part seems extraneous, but taking it away also creates confusion, do you have any suggestion? >> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:74 >> + if (params.mimeType == WebKit::FakeWebPlugin::mimeType()) > > No need for "WebKit::". We're already in the WebKit namespace. Done. >> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:84 >> +} > > Why is this static? We should just create one on the stack for each test. Copy paste effect from WebViewTest. Fixed. >> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:89 >> + return WebFrameImpl::pluginContainerFromNode(element); > > Yeah, moving pluginContainerFromNode to WebElement might be the best thing. Moved. >> 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. Done. >> 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. Done.
Adam Barth
Comment 11 2012-11-15 15:11:57 PST
Comment on attachment 174527 [details] Patch 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.
lazyboy
Comment 12 2012-11-15 16:15:22 PST
lazyboy
Comment 13 2012-11-15 16:20:49 PST
Comment on attachment 174527 [details] Patch 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*. Yes, I was hesitant to use Impl here, changed to WebPluginContainer. This also means even if we're returning WebPluginContainer* from the function, the actual function contents need to get it as WebPluginContainerImpl* and then return it (WebPluginContainerImpl is Widget, but WebPluginContainer is not). Also all usages in WebFrameImpl requires casting the return value to WebPluginContainerImpl*, which I think is OK? Done. >> Source/WebKit/chromium/src/WebFrameImpl.cpp:1217 >> + 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. Move to WebNode. >> Source/WebKit/chromium/tests/FakeWebPlugin.cpp:64 >> +} > > Why are you creating a static for this? I would just create a temporary WebString in the one place where you need it. Done. I thought using static would be nicer so we don't create WebString each time we hit createPlugin(), well this is test. >> 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. Done.
Adam Barth
Comment 14 2012-11-15 17:34:38 PST
Comment on attachment 174541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174541&action=review Thanks. One (minor) question below. > Source/WebKit/chromium/tests/FrameTestHelpers.h:46 > +class TestWebFrameClient : public WebFrameClient { > +}; What's the point of exposing the class? > Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:71 > +class TestPluginWebFrameClient : public FrameTestHelpers::TestWebFrameClient { It seesms like this class could just as easily inherit from WebFrameClient directly.
lazyboy
Comment 15 2012-11-15 17:50:03 PST
Comment on attachment 174541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174541&action=review Sending replies. Thanks for the review. >> Source/WebKit/chromium/tests/FrameTestHelpers.h:46 >> +}; > > What's the point of exposing the class? Comment below. >> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:71 >> +class TestPluginWebFrameClient : public FrameTestHelpers::TestWebFrameClient { > > It seesms like this class could just as easily inherit from WebFrameClient directly. Yes it easily could. All tests using same base Test* class would have the benefit that we share mocking/testing properties as much, so down the road if anyone adds useful stub to TestWebFrameClient, we get that for free here. Unfortunately this is not a win-win situation as I think of it more now, it also makes TestWebFrameClient more constrained, it cannot break certain plugin functionality this test expects (which is empty at this moment). I can remove this and make it direct subclass of WebFrameClient if you like.
Adam Barth
Comment 16 2012-11-15 17:54:38 PST
> I can remove this and make it direct subclass of WebFrameClient if you like. Yeah, that's probably better. We can always expose TestWebFrameClient in the header later if we find that useful
lazyboy
Comment 17 2012-11-15 18:19:13 PST
lazyboy
Comment 18 2012-11-15 18:20:38 PST
Comment on attachment 174541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174541&action=review >>> Source/WebKit/chromium/tests/WebPluginContainerTest.cpp:71 >>> +class TestPluginWebFrameClient : public FrameTestHelpers::TestWebFrameClient { >> >> It seesms like this class could just as easily inherit from WebFrameClient directly. > > Yes it easily could. > All tests using same base Test* class would have the benefit that we share mocking/testing properties as much, so down the road if anyone adds useful stub to TestWebFrameClient, we get that for free here. > Unfortunately this is not a win-win situation as I think of it more now, it also makes TestWebFrameClient more constrained, it cannot break certain plugin functionality this test expects (which is empty at this moment). > I can remove this and make it direct subclass of WebFrameClient if you like. Removed and subclassing WebFrameClient. Done.
WebKit Review Bot
Comment 19 2012-11-15 19:41:09 PST
Comment on attachment 174583 [details] Patch Clearing flags on attachment: 174583 Committed r134880: <http://trac.webkit.org/changeset/134880>
WebKit Review Bot
Comment 20 2012-11-15 19:41:14 PST
All reviewed patches have been landed. Closing bug.
Kentaro Hara
Comment 21 2012-11-15 23:34:43 PST
Reverted r134880 for reason: it broke Chromium Windows build Committed r134907: <http://trac.webkit.org/changeset/134907>
lazyboy
Comment 22 2012-11-16 13:28:54 PST
lazyboy
Comment 23 2012-11-16 13:30:43 PST
(In reply to comment #21) > Reverted r134880 for reason: > > it broke Chromium Windows build > > Committed r134907: <http://trac.webkit.org/changeset/134907> Uploaded patch with windows fix. Change in WebPluginContainer.h: #include "platform/WebCommon.h" -> for uint32_t in windows.
WebKit Review Bot
Comment 24 2012-11-17 10:04:28 PST
Comment on attachment 174751 [details] Patch Clearing flags on attachment: 174751 Committed r135047: <http://trac.webkit.org/changeset/135047>
WebKit Review Bot
Comment 25 2012-11-17 10:04:35 PST
All reviewed patches have been landed. Closing bug.
Kentaro Hara
Comment 26 2012-11-18 17:50:33 PST
Reverted r135047 for reason: it broke Chromium Windows/Mac build Committed r135094: <http://trac.webkit.org/changeset/135094>
Kentaro Hara
Comment 28 2012-11-18 17:53:10 PST
FYI: Rolled out in r135094.
lazyboy
Comment 29 2012-11-19 19:21:06 PST
lazyboy
Comment 30 2012-11-19 19:25:18 PST
(In reply to comment #27) > Rolled out this patch because it broke Chromium Win/Mac build: > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20Builder%20%28dbg%29/builds/31315/steps/compile/logs/stdio > > http://build.webkit.org/builders/Chromium%20Win%20Release%20%28Tests%29/builds/31480/steps/webkitpy-test/logs/stdio > > It looks like you need to address symbol issues. Another fixed attempt for windows, looks like I've been missing a comma separating file list in WebKit.gyp ! Checked in win builder, compile passes: before (to replicate): build.chromium.org/p/tryserver.chromium/builders/win/builds/37536 after: build.chromium.org/p/tryserver.chromium/builders/win/builds/37577
lazyboy
Comment 31 2012-11-20 11:27:42 PST
Created attachment 175252 [details] Patch, sync so it applies correctly
WebKit Review Bot
Comment 32 2012-11-20 14:42:11 PST
Comment on attachment 175252 [details] Patch, sync so it applies correctly Clearing flags on attachment: 175252 Committed r135315: <http://trac.webkit.org/changeset/135315>
Note You need to log in before you can comment on or make changes to this bug.