WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
41292
[V8]When a NPAPI plugin calls NPN_Evaluate to execute script in browser, if the call is initiated by user, allow the popup
https://bugs.webkit.org/show_bug.cgi?id=41292
Summary
[V8]When a NPAPI plugin calls NPN_Evaluate to execute script in browser, if t...
Johnny(Jianning) Ding
Reported
2010-06-28 11:40:59 PDT
Now YouTube's videos all have new new-window button on player (see the first attachment). When clicking that button on Chromium windows, nothing happened. The Chromium bug is here:
http://crbug.com/43547
After investigation, I found the window.open was triggered in the script calling from Plugin side by using NPN_Evaluate interface. In Chromium, the NPN_Evaluate called _NPN_EvaluateHelper(V8NPObject.cpp) to execute the script with passing a additional parameter "popupsAllowed". The parameter "popupsAllowed" was from PluginInstance::popups_allowed() which was set when the input message could be considered as user gesture event (WebPluginDelegateImpl::IsUserGesture/WebPluginDelegateImpl::IsUserGestureMessage). In chromium windows version, the input events to windowed plugin (for the youtube case) were handled by the plugin window, so the UserGestureIndicator had no chance to be set to right value:DefinitelyProcessingUserGesture. Also we didn't set right value for UserGestureIndicator when calling function _NPN_EvaluateHelper with "popupsAllowed":true, so the popup window was not considered as user initiated. But in Safari Mac version, all input events even the input events to plugin are delivered by webview and handled by event handlers in WebCore/page/EventHandler.cpp) in which UserGestureIndicator is set to DefinitelyUserGesture, so the popup window is treated as user initiated. To fix this issue on chromium, we may need to set UserGestureIndicator to DefinitelyProcessingUserGesture in function:_NPN_EvaluateHelper when popupsAllowed is true. Any suggestions?
Attachments
youtube new-window feature illustration, original uploaded by dogantuncer on http://crbug.com/43547
(2.45 KB, image/png)
2010-06-28 11:47 PDT
,
Johnny(Jianning) Ding
no flags
Details
patch v1 to set right value on UserGestureIndicator to indicate whether the NPN_Evaluate allows popup window or not.
(1.49 KB, patch)
2010-06-28 12:19 PDT
,
Johnny(Jianning) Ding
abarth
: review-
Details
Formatted Diff
Diff
patch V2, add a test
(14.56 KB, patch)
2010-06-30 08:26 PDT
,
Johnny(Jianning) Ding
no flags
Details
Formatted Diff
Diff
Patch V2 with fixing style warnings, add a plugin test
(14.95 KB, patch)
2010-06-30 08:56 PDT
,
Johnny(Jianning) Ding
no flags
Details
Formatted Diff
Diff
patch v2 with removing redundant comment and skip the test on chromium
(15.73 KB, patch)
2010-06-30 18:45 PDT
,
Johnny(Jianning) Ding
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
patch v2 to adjust to the change of TestNetscapePlugin renaming and Chromium's expectation.txt
(15.07 KB, patch)
2010-07-09 03:59 PDT
,
Johnny(Jianning) Ding
no flags
Details
Formatted Diff
Diff
patch V2
(15.39 KB, patch)
2010-07-12 06:11 PDT
,
Johnny(Jianning) Ding
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
layout test case
(3.01 KB, text/html)
2010-09-15 07:07 PDT
,
Johnny(Jianning) Ding
no flags
Details
debug snapshot
(246.67 KB, image/png)
2010-09-15 07:09 PDT
,
Johnny(Jianning) Ding
no flags
Details
patch v3
(22.01 KB, patch)
2010-09-15 10:58 PDT
,
Johnny(Jianning) Ding
no flags
Details
Formatted Diff
Diff
Patch
(13.88 KB, patch)
2011-05-02 08:35 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(9.06 KB, patch)
2011-05-23 12:22 PDT
,
Robert Hogan
aroben
: review+
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Johnny(Jianning) Ding
Comment 1
2010-06-28 11:47:15 PDT
Created
attachment 59918
[details]
youtube new-window feature illustration, original uploaded by dogantuncer on
http://crbug.com/43547
Johnny(Jianning) Ding
Comment 2
2010-06-28 12:19:38 PDT
Created
attachment 59922
[details]
patch v1 to set right value on UserGestureIndicator to indicate whether the NPN_Evaluate allows popup window or not. I didn't add layout test on this change. Will think about the test when I figure out why Chroimum Mac is OK for youtube case without this change.
WebKit Review Bot
Comment 3
2010-06-28 12:20:33 PDT
Attachment 59922
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/bindings/v8/NPV8Object.cpp:44: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Adam Barth
Comment 4
2010-06-28 12:41:28 PDT
Comment on
attachment 59922
[details]
patch v1 to set right value on UserGestureIndicator to indicate whether the NPN_Evaluate allows popup window or not. This patch looks "obviously" correct. We just need a test. You should be able to write one using the WebKit test plugin. (Also, please fix style-bots nit... We use case-sensitive alphabetical includes for whatever reason.)
Johnny(Jianning) Ding
Comment 5
2010-06-30 08:26:43 PDT
Created
attachment 60121
[details]
patch V2, add a test
WebKit Review Bot
Comment 6
2010-06-30 08:28:49 PDT
Attachment 60121
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp:983: Use 0 instead of NULL. [readability/null] [5] WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:116: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:118: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:297: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:307: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:383: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/main.cpp:406: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Total errors found: 7 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Johnny(Jianning) Ding
Comment 7
2010-06-30 08:56:13 PDT
Created
attachment 60124
[details]
Patch V2 with fixing style warnings, add a plugin test
Adam Barth
Comment 8
2010-06-30 14:09:18 PDT
Comment on
attachment 60124
[details]
Patch V2 with fixing style warnings, add a plugin test Nice test. Thanks. WebCore/bindings/v8/NPV8Object.cpp:282 + // Initialize UserGestureIndicator to right value. This comment is redundant and probably should be removed.
Johnny(Jianning) Ding
Comment 9
2010-06-30 17:11:40 PDT
Hi Adam, Thanks, should I upload a new patch which already removed the redundant comment? By the way, since Chromium has its own DumpRenderTree utilities, this test will break chromium test. I will file a chromium bug to implement same logic in Chromium's TestNetscapePlugin on crbug.com and provide a patch to change LayoutTest/platform/chromium/test_expectations.txt in this bug thread after this change is landed. Or maybe I should do this now in this patch. What do you suggest?
Adam Barth
Comment 10
2010-06-30 17:14:33 PDT
> Thanks, should I upload a new patch which already removed the redundant comment?
Sure, if you like. You don't need to get it reviewed again. Uploading a new patch will let us land you patch with the commit-queue.
> By the way, since Chromium has its own DumpRenderTree utilities, this test will break chromium test. > I will file a chromium bug to implement same logic in Chromium's TestNetscapePlugin on crbug.com and provide a patch to change LayoutTest/platform/chromium/test_expectations.txt in this bug thread after this change is landed. Or maybe I should do this now in this patch. What do you suggest?
You probably should change test_expectations.txt in this patch so that the canary bot won't turn red after landing your patch.
Johnny(Jianning) Ding
Comment 11
2010-06-30 18:45:29 PDT
Created
attachment 60182
[details]
patch v2 with removing redundant comment and skip the test on chromium Thanks Adam. The new patch v2 removed redundant comment and change the chromium's test_expectations.txt to skip the test on chromium. Since a new file was modified in this this patch, I still add review flag for this patch. Please take a look.
Adam Barth
Comment 12
2010-06-30 20:21:18 PDT
Comment on
attachment 60182
[details]
patch v2 with removing redundant comment and skip the test on chromium great. thanks
Johnny(Jianning) Ding
Comment 13
2010-07-07 23:39:37 PDT
Hi Adam, Would you or someone else please help on landing this change? Thanks!
Adam Barth
Comment 14
2010-07-08 09:31:38 PDT
Comment on
attachment 60182
[details]
patch v2 with removing redundant comment and skip the test on chromium sure
WebKit Commit Bot
Comment 15
2010-07-08 11:46:46 PDT
Comment on
attachment 60182
[details]
patch v2 with removing redundant comment and skip the test on chromium Rejecting patch 60182 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1 Last 500 characters of output: Hunk #1 FAILED at 2891. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej patching file LayoutTests/platform/gtk/Skipped Hunk #1 succeeded at 3395 (offset 1 line). patching file LayoutTests/platform/qt/Skipped Hunk #1 succeeded at 457 (offset 35 lines). patching file LayoutTests/platform/win/Skipped patching file LayoutTests/plugins/plugin-initiate-popup-window-expected.txt patching file LayoutTests/plugins/plugin-initiate-popup-window.html Full output:
http://webkit-commit-queue.appspot.com/results/3480036
Eric Seidel (no email)
Comment 16
2010-07-09 03:16:26 PDT
Comment on
attachment 60124
[details]
Patch V2 with fixing style warnings, add a plugin test Cleared Adam Barth's review+ from obsolete
attachment 60124
[details]
so that this bug does not appear in
http://webkit.org/pending-commit
.
Johnny(Jianning) Ding
Comment 17
2010-07-09 03:25:08 PDT
Adam, Eric, I just checked the log of commit failure, it's because the WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj was recently renamed to TestNetscapePlugIn and chromium's expectation.txt was changed. I am creating a new patch according to those changes and will send for review. Please add it to commit queue when the patch gets passed. Thanks!
Johnny(Jianning) Ding
Comment 18
2010-07-09 03:59:36 PDT
Created
attachment 61031
[details]
patch v2 to adjust to the change of TestNetscapePlugin renaming and Chromium's expectation.txt
Johnny(Jianning) Ding
Comment 19
2010-07-12 06:11:52 PDT
Created
attachment 61217
[details]
patch V2 just found conflict on chromium/text_expectation.txt, resolved it and created the latest patch V2.
Johnny(Jianning) Ding
Comment 20
2010-07-12 07:59:34 PDT
Hi Adam, Since platform/chromiun/test_expectation.txt was changed very frequently, juts found conflict again. Do you know anyway I can use to let my patch have no conflict with that file, otherwise I can not land this patch via the commit queue. Thanks!
Adam Barth
Comment 21
2010-07-12 18:06:00 PDT
Comment on
attachment 61217
[details]
patch V2 Let's try one more time. If it doesn't work, we can land it manually.
WebKit Commit Bot
Comment 22
2010-07-12 18:59:02 PDT
Comment on
attachment 61217
[details]
patch V2 Rejecting patch 61217 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1 Last 500 characters of output: nk #1 succeeded at 2993 (offset 3 lines). Hunk #2 FAILED at 3011. 1 out of 2 hunks FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej patching file LayoutTests/platform/gtk/Skipped patching file LayoutTests/platform/qt/Skipped patching file LayoutTests/platform/win/Skipped Hunk #1 succeeded at 804 (offset -1 lines). patching file LayoutTests/plugins/plugin-initiate-popup-window-expected.txt patching file LayoutTests/plugins/plugin-initiate-popup-window.html Full output:
http://webkit-commit-queue.appspot.com/results/3481249
Johnny(Jianning) Ding
Comment 23
2010-07-12 20:56:55 PDT
The commit is failed again because of the conflict in test_expectation.txt. Seems like we have to quickly resolve the conflict based on the latest chromium/test_expectation.txt and manually land it. Thanks Adam! By the way, I already had 10+ non-trivial CL(plus my previous account: johnnyding.webkit@gmail). I will apply the committer privilege when I have 20.
Adam Barth
Comment 24
2010-07-14 14:03:14 PDT
Committed
r63352
: <
http://trac.webkit.org/changeset/63352
>
Adam Barth
Comment 25
2010-07-14 23:34:06 PDT
I'm rolling out this patch because it broke two tests on snow leopard: plugins/plugin-initiate-popup-window.html plugins/plugin-javascript-access.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r63408%20(13677)/plugins/plugin-initiate-popup-window-pretty-diff.html
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r63408%20(13677)/plugins/plugin-javascript-access-pretty-diff.html
Johnny(Jianning) Ding
Comment 26
2010-07-14 23:38:17 PDT
(In reply to
comment #25
)
> I'm rolling out this patch because it broke two tests on snow leopard: > > plugins/plugin-initiate-popup-window.html > plugins/plugin-javascript-access.html > >
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r63408%20(13677)/plugins/plugin-initiate-popup-window-pretty-diff.html
>
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r63408%20(13677)/plugins/plugin-javascript-access-pretty-diff.html
I see, my mac is leopard not snow leopard. Looks the diff files, seems like we need to generate the individual expected file for leopard and snow leopard.
Adam Barth
Comment 27
2010-07-14 23:40:32 PDT
> I see, my mac is leopard not snow leopard. > Looks the diff files, seems like we need to generate the individual expected file for leopard and snow leopard.
Why does leopard and snow leopard have different results?
Adam Barth
Comment 28
2010-07-14 23:41:28 PDT
It looks like state is leaking from one test to another. Notice that the console messages disappear from one and appear in the other. It's more complex than just rebaselining.
Adam Barth
Comment 29
2010-07-14 23:51:13 PDT
rolled out in
http://trac.webkit.org/changeset/63409
Johnny(Jianning) Ding
Comment 30
2010-07-15 00:02:01 PDT
(In reply to
comment #28
)
> It looks like state is leaking from one test to another. Notice that the console messages disappear from one and appear in the other. It's more complex than just rebaselining.
you are right, it's stranger. I will find a snow leopard machine to debug this.
Johnny(Jianning) Ding
Comment 31
2010-07-15 01:16:58 PDT
My preliminary guess is when running plugin-initiate-popup-window.html on snow leopard, when calling notifyDone, the test plugin still had focus. But after calling notifyDone, the document was unloading to cause plugin to lost focus, so the function plugLog sent 'LostFocus' message to dumpDOMTree. However, notifyDone already called Dump() to generate actual result file, so the console messages disappeared from plugin-initiate-popup-window.html and appeared in the other test:plugins/plugin-javascript-access.html.
Adam Barth
Comment 32
2010-08-10 22:32:49 PDT
Comment on
attachment 61217
[details]
patch V2 Looks like we'll need a new patch here.
Maciej Stachowiak
Comment 33
2010-09-14 12:37:35 PDT
A bug was filed in Radar that mentioned this bugzilla bug, but I don't understand the relationship: <
rdar://problem/8215635
> WebKitPluginHost didn't pass right value to nestcape plugin for allowPopup flag Can someone explain the relationship? Is there an actual symptom in SnowLeopard Safari?
Johnny(Jianning) Ding
Comment 34
2010-09-15 07:07:32 PDT
Created
attachment 67673
[details]
layout test case (In reply to
comment #33
)
> A bug was filed in Radar that mentioned this bugzilla bug, but I don't understand the relationship: > > <
rdar://problem/8215635
> WebKitPluginHost didn't pass right value to nestcape plugin for allowPopup flag > > Can someone explain the relationship? Is there an actual symptom in SnowLeopard Safari?
Hi Maciej, I filed the Radar bug because of failure of the attached layout test. I am going to add this layout test in the patch for this bug. Starting from snow leopard, the npapi plugin is run under "WebKitPluginHost.app" process, and WebKit relies on the allowPopups flag passed from "WebKitPluginHost.app" RPC to control to whether block the popups initiated by plugin or not. When debugging with XCode, I found the allowPopups flags from "WebKitPluginHost.app" was not right. (Please take a look the attached debug snapshot). I hope my explanation answers your question.
Johnny(Jianning) Ding
Comment 35
2010-09-15 07:09:25 PDT
Created
attachment 67674
[details]
debug snapshot
Johnny(Jianning) Ding
Comment 36
2010-09-15 10:58:54 PDT
Created
attachment 67689
[details]
patch v3 Hi Adam, I have create a new patch. It sets the popupAllowed flag passed from plugin to current execution frame, so WebKit can get right gesture state for the popups initiated by users from plugin side.
Adam Barth
Comment 37
2010-09-15 12:46:51 PDT
Comment on
attachment 67689
[details]
patch v3 View in context:
https://bugs.webkit.org/attachment.cgi?id=67689&action=prettypatch
This looks good to me, but I'm not an expert on the plugin code. Might be worth having a plug-in expert take a look too.
> WebCore/bindings/v8/NPV8Object.cpp:285 > + ASSERT(frame);
This assert seems unneeded. We'll crash on the next line anyway.
> WebKit/mac/ChangeLog:9 > + execution frame, so WeKit can always get right gesture state on Mac.
WeKit -> WebKit
> WebKit/mac/ChangeLog:10 > + From Mac Snow Leopard, the plugin is run under "WebKitPluginHost,app"
.app ?
Johnny(Jianning) Ding
Comment 38
2010-09-15 17:48:01 PDT
(In reply to
comment #37
)
> (From update of
attachment 67689
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=67689&action=prettypatch
> > This looks good to me, but I'm not an expert on the plugin code. Might be worth having a plug-in expert take a look too.
Thanks, Adam! Would you please add or recommend a plug-in expert t review this patch? I will upload a landing patch (remove the typo and unnecessary code according to comments of you and the plugin-expert) after the plug-in expert reviews this patch. If he/she is OK with this patch, would you please help me to land this patch? (may need to manually land since the chromium/test-expectations.txt often changes to cause conflict.)
Adam Barth
Comment 39
2010-09-16 00:30:26 PDT
I'm not sure who would be the best plug-in expert. Maybe ask on #webkit.
Adam Barth
Comment 40
2010-09-28 23:28:12 PDT
Committed
r68630
: <
http://trac.webkit.org/changeset/68630
>
Eric Seidel (no email)
Comment 41
2010-09-29 19:14:04 PDT
plugins/plugin-initiate-popup-window.html is broken on Snow Leopard and has been since checkin.
Adam Barth
Comment 42
2010-09-29 20:03:14 PDT
(In reply to
comment #41
)
> plugins/plugin-initiate-popup-window.html is broken on Snow Leopard and has been since checkin.
Not sure why sheriffbot didn't complain. In any case, I've update the expected results.
Robert Hogan
Comment 43
2011-05-02 08:35:57 PDT
Created
attachment 91925
[details]
Patch
Eric Seidel (no email)
Comment 44
2011-05-09 20:58:08 PDT
Comment on
attachment 91925
[details]
Patch Cleared review? from
attachment 91925
[details]
so that this bug does not appear in
http://webkit.org/pending-review
. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Robert Hogan
Comment 45
2011-05-10 10:29:19 PDT
Reopening for Qt fix.
Robert Hogan
Comment 46
2011-05-23 12:22:59 PDT
Created
attachment 94457
[details]
Patch
Adam Roben (:aroben)
Comment 47
2011-05-26 12:59:53 PDT
Comment on
attachment 94457
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=94457&action=review
> LayoutTests/ChangeLog:10 > + * platform/qt/plugins/plugin-initiate-popup-window-expected.txt: Copied from LayoutTests/platform/mac/plugins/plugin-initiate-popup-window-expected.txt.
Can we put the results next to the test instead? In what way are they platform-specific?
> Source/WebCore/ChangeLog:9 > + If the event is from a user gesture, allow popups.
...and the plugin doesn't support the popup-related NPAPI calls, right? You should mention that here, too.
> Source/WebCore/plugins/qt/PluginViewQt.cpp:390 > + if (m_plugin->pluginFuncs()->version < NPVERS_HAS_POPUPS_ENABLED_STATE > + && (event.type == ButtonRelease || event.type == 3 /*KeyRelease*/)) {
Is there really no constant for KeyRelease?
Robert Hogan
Comment 48
2011-05-26 14:03:23 PDT
(In reply to
comment #47
)
> (From update of
attachment 94457
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=94457&action=review
> > > LayoutTests/ChangeLog:10 > > + * platform/qt/plugins/plugin-initiate-popup-window-expected.txt: Copied from LayoutTests/platform/mac/plugins/plugin-initiate-popup-window-expected.txt. > > Can we put the results next to the test instead? In what way are they platform-specific?
The plugin-log lines make this and most tests that use them platform-specific it seems.
> > > Source/WebCore/ChangeLog:9 > > + If the event is from a user gesture, allow popups. > > ...and the plugin doesn't support the popup-related NPAPI calls, right? You should mention that here, too.
Updated.
> > > Source/WebCore/plugins/qt/PluginViewQt.cpp:390 > > + if (m_plugin->pluginFuncs()->version < NPVERS_HAS_POPUPS_ENABLED_STATE > > + && (event.type == ButtonRelease || event.type == 3 /*KeyRelease*/)) { > > Is there really no constant for KeyRelease?
There is, but Qt overwrites it.
Robert Hogan
Comment 49
2011-05-26 14:05:07 PDT
Committed
r87429
: <
http://trac.webkit.org/changeset/87429
>
Adam Roben (:aroben)
Comment 50
2011-05-26 14:06:29 PDT
Comment on
attachment 94457
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=94457&action=review
>>> LayoutTests/ChangeLog:10 >>> + * platform/qt/plugins/plugin-initiate-popup-window-expected.txt: Copied from LayoutTests/platform/mac/plugins/plugin-initiate-popup-window-expected.txt. >> >> Can we put the results next to the test instead? In what way are they platform-specific? > > The plugin-log lines make this and most tests that use them platform-specific it seems.
Can't we just require that all platforms make the test plugin log the same messages? We already require that for DRT.
Adam Roben (:aroben)
Comment 51
2011-05-26 14:07:07 PDT
Why does this bug have "[V8]" in the title? Is it really V8-specific?
Robert Hogan
Comment 52
2011-05-26 14:43:45 PDT
(In reply to
comment #51
)
> Why does this bug have "[V8]" in the title? Is it really V8-specific?
The original patch on this bug introduced the test that my patch fixes for Qt. At the time I thought it made sense to keep them under the same bug, but I now think it's just confusing for someone looking at the bug later.
Robert Hogan
Comment 53
2011-05-26 14:48:32 PDT
(In reply to
comment #50
)
> (From update of
attachment 94457
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=94457&action=review
> > >>> LayoutTests/ChangeLog:10 > >>> + * platform/qt/plugins/plugin-initiate-popup-window-expected.txt: Copied from LayoutTests/platform/mac/plugins/plugin-initiate-popup-window-expected.txt. > >> > >> Can we put the results next to the test instead? In what way are they platform-specific? > > > > The plugin-log lines make this and most tests that use them platform-specific it seems. > > Can't we just require that all platforms make the test plugin log the same messages? We already require that for DRT.
The order in which the plugin receives events and the line numbers associated with the event seems to differ across platforms. For example Qt vs Chromium-Win: < CONSOLE MESSAGE: line 17: window count: 1 < CONSOLE MESSAGE: line 0: PLUGIN: getFocusEvent < CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (12, 12) < CONSOLE MESSAGE: line 30: window count: 2 < CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (12, 12) < CONSOLE MESSAGE: line 0: PLUGIN: loseFocusEvent < CONSOLE MESSAGE: line 0: PLUGIN: getFocusEvent < CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (52, 12) < CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (52, 12) < CONSOLE MESSAGE: line 0: PLUGIN: keyDown 'p' < CONSOLE MESSAGE: line 52: window count: 3 < CONSOLE MESSAGE: line 0: PLUGIN: keyUp 'p' < < < Specify a script and a mouse/keyboard event to the plugin. The specified script will be evaluated in the browser when the specified event is received by the plugin. The test is for bug
https://bugs.webkit.org/show_bug.cgi?id=41292
. < Opening window by mouse down is PASSED < Opening window by key down is PASSED ---
> CONSOLE MESSAGE: line 0: PLUGIN: event 71 > CONSOLE MESSAGE: line 0: PLUGIN: event 71 > CONSOLE MESSAGE: line 17: window count: 1 > CONSOLE MESSAGE: line 21: PLUGIN: getFocusEvent > CONSOLE MESSAGE: line 21: PLUGIN: mouseDown at (20, 20) > CONSOLE MESSAGE: line 30: window count: 2 > CONSOLE MESSAGE: line 22: PLUGIN: mouseUp at (20, 20) > CONSOLE MESSAGE: line 42: PLUGIN: loseFocusEvent > CONSOLE MESSAGE: line 42: PLUGIN: getFocusEvent > CONSOLE MESSAGE: line 42: PLUGIN: mouseDown at (60, 60) > CONSOLE MESSAGE: line 43: PLUGIN: mouseUp at (60, 60) > CONSOLE MESSAGE: line 44: PLUGIN: keyDown 'P' > CONSOLE MESSAGE: line 52: window count: 3 > CONSOLE MESSAGE: line 44: PLUGIN: keyUp 'P' >
Qt vs Mac: 12a13
> CONSOLE MESSAGE: line 0: PLUGIN: loseFocusEvent
I've opened a bug for this :
https://bugs.webkit.org/show_bug.cgi?id=61566
Johnny(Jianning) Ding
Comment 54
2011-05-26 23:23:05 PDT
(In reply to
comment #51
)
> Why does this bug have "[V8]" in the title? Is it really V8-specific?
In the original bug, V8 version ScriptController:::processingUserGesture doesn't consider the allowPopups flag passed from plugin side, so the original patch forced on solving that issue, what's why it has [V8] in the title.
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