Bug 102339 - Provide page/window coordinates to plugin's local coordinates translation in WebPluginContainer.
Summary: Provide page/window coordinates to plugin's local coordinates translation in ...
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-14 23:02 PST by lazyboy
Modified: 2012-11-20 14:42 PST (History)
7 users (show)

See Also:


Attachments
Patch (19.73 KB, patch)
2012-11-14 23:02 PST, lazyboy
no flags Details | Formatted Diff | Diff
Patch (21.97 KB, patch)
2012-11-15 12:52 PST, lazyboy
no flags Details | Formatted Diff | Diff
Patch (25.40 KB, patch)
2012-11-15 15:01 PST, lazyboy
no flags Details | Formatted Diff | Diff
Patch (25.30 KB, patch)
2012-11-15 16:15 PST, lazyboy
no flags Details | Formatted Diff | Diff
Patch (23.91 KB, patch)
2012-11-15 18:19 PST, lazyboy
no flags Details | Formatted Diff | Diff
Patch (24.06 KB, patch)
2012-11-16 13:28 PST, lazyboy
no flags Details | Formatted Diff | Diff
Patch (24.02 KB, patch)
2012-11-19 19:21 PST, lazyboy
abarth: review+
Details | Formatted Diff | Diff
Patch, sync so it applies correctly (23.96 KB, patch)
2012-11-20 11:27 PST, lazyboy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description lazyboy 2012-11-14 23:02:06 PST
Provide page/window coordinates to plugin's local coordinates translation in WebPluginContainer.
Comment 1 lazyboy 2012-11-14 23:02:56 PST
Created attachment 174357 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 WebKit Review Bot 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.
Comment 4 WebKit Review Bot 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
Comment 5 Adam Barth 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.
Comment 6 lazyboy 2012-11-15 12:52:50 PST
Created attachment 174501 [details]
Patch
Comment 7 lazyboy 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.
Comment 8 Adam Barth 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.
Comment 9 lazyboy 2012-11-15 15:01:05 PST
Created attachment 174527 [details]
Patch
Comment 10 lazyboy 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.
Comment 11 Adam Barth 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.
Comment 12 lazyboy 2012-11-15 16:15:22 PST
Created attachment 174541 [details]
Patch
Comment 13 lazyboy 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.
Comment 14 Adam Barth 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.
Comment 15 lazyboy 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.
Comment 16 Adam Barth 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
Comment 17 lazyboy 2012-11-15 18:19:13 PST
Created attachment 174583 [details]
Patch
Comment 18 lazyboy 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.
Comment 19 WebKit Review Bot 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>
Comment 20 WebKit Review Bot 2012-11-15 19:41:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Kentaro Hara 2012-11-15 23:34:43 PST
Reverted r134880 for reason:

it broke Chromium Windows build

Committed r134907: <http://trac.webkit.org/changeset/134907>
Comment 22 lazyboy 2012-11-16 13:28:54 PST
Created attachment 174751 [details]
Patch
Comment 23 lazyboy 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.
Comment 24 WebKit Review Bot 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>
Comment 25 WebKit Review Bot 2012-11-17 10:04:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Kentaro Hara 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>
Comment 28 Kentaro Hara 2012-11-18 17:53:10 PST
FYI: Rolled out in r135094.
Comment 29 lazyboy 2012-11-19 19:21:06 PST
Created attachment 175116 [details]
Patch
Comment 30 lazyboy 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
Comment 31 lazyboy 2012-11-20 11:27:42 PST
Created attachment 175252 [details]
Patch, sync so it applies correctly
Comment 32 WebKit Review Bot 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>