WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
lazyboy
Comment 1
2012-11-14 23:02:56 PST
Created
attachment 174357
[details]
Patch
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
Created
attachment 174501
[details]
Patch
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
Created
attachment 174527
[details]
Patch
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
Created
attachment 174541
[details]
Patch
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
Created
attachment 174583
[details]
Patch
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
Created
attachment 174751
[details]
Patch
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 27
2012-11-18 17:52:41 PST
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
Comment 28
2012-11-18 17:53:10 PST
FYI: Rolled out in
r135094
.
lazyboy
Comment 29
2012-11-19 19:21:06 PST
Created
attachment 175116
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug