Bug 102339 – Provide page/window coordinates to plugin's local coordinates translation in WebPluginContainer.

• Component: New Bugs
• Status: UNCONFIRMED

Provide page/window coordinates to plugin's local coordinates translation in WebPluginContainer.

Created attachment 174357 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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 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 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.

Created attachment 174501 [details]
Patch

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 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.

Created attachment 174527 [details]
Patch

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 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.

Created attachment 174541 [details]
Patch

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 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 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]

> 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

Created attachment 174583 [details]
Patch

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 on attachment 174583 [details]
Patch

Clearing flags on attachment: 174583

Committed r134880: <http://trac.webkit.org/changeset/134880>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Reverted r134880 for reason:

it broke Chromium Windows build

Committed r134907: <http://trac.webkit.org/changeset/134907>

Created attachment 174751 [details]
Patch

(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 on attachment 174751 [details]
Patch

Clearing flags on attachment: 174751

Committed r135047: <http://trac.webkit.org/changeset/135047>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Reverted r135047 for reason:

it broke Chromium Windows/Mac build

Committed r135094: <http://trac.webkit.org/changeset/135094>

[Kentaro Hara]

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.

[Kentaro Hara]

FYI: Rolled out in r135094.

Created attachment 175116 [details]
Patch

(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

Created attachment 175252 [details]
Patch, sync so it applies correctly

[WebKit Review Bot]

Comment on attachment 175252 [details]
Patch, sync so it applies correctly

Clearing flags on attachment: 175252

Committed r135315: <http://trac.webkit.org/changeset/135315>