Bug 41292 - [V8]When a NPAPI plugin calls NPN_Evaluate to execute script in browser, if the call is initiated by user, allow the popup
Summary: [V8]When a NPAPI plugin calls NPN_Evaluate to execute script in browser, if t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords:
Depends on: 42341
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-28 11:40 PDT by Johnny(Jianning) Ding
Modified: 2011-05-26 23:23 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Johnny(Jianning) Ding 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?
Comment 1 Johnny(Jianning) Ding 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
Comment 2 Johnny(Jianning) Ding 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.
Comment 3 WebKit Review Bot 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.
Comment 4 Adam Barth 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.)
Comment 5 Johnny(Jianning) Ding 2010-06-30 08:26:43 PDT
Created attachment 60121 [details]
patch V2, add a test
Comment 6 WebKit Review Bot 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.
Comment 7 Johnny(Jianning) Ding 2010-06-30 08:56:13 PDT
Created attachment 60124 [details]
Patch V2 with fixing style warnings, add a plugin test
Comment 8 Adam Barth 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.
Comment 9 Johnny(Jianning) Ding 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?
Comment 10 Adam Barth 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.
Comment 11 Johnny(Jianning) Ding 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.
Comment 12 Adam Barth 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
Comment 13 Johnny(Jianning) Ding 2010-07-07 23:39:37 PDT
Hi Adam,

Would you or someone else please help on landing this change?
Thanks!
Comment 14 Adam Barth 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
Comment 15 WebKit Commit Bot 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
Comment 16 Eric Seidel (no email) 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.
Comment 17 Johnny(Jianning) Ding 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!
Comment 18 Johnny(Jianning) Ding 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
Comment 19 Johnny(Jianning) Ding 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.
Comment 20 Johnny(Jianning) Ding 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!
Comment 21 Adam Barth 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.
Comment 22 WebKit Commit Bot 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
Comment 23 Johnny(Jianning) Ding 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.
Comment 24 Adam Barth 2010-07-14 14:03:14 PDT
Committed r63352: <http://trac.webkit.org/changeset/63352>
Comment 26 Johnny(Jianning) Ding 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.
Comment 27 Adam Barth 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?
Comment 28 Adam Barth 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.
Comment 29 Adam Barth 2010-07-14 23:51:13 PDT
rolled out in http://trac.webkit.org/changeset/63409
Comment 30 Johnny(Jianning) Ding 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.
Comment 31 Johnny(Jianning) Ding 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.
Comment 32 Adam Barth 2010-08-10 22:32:49 PDT
Comment on attachment 61217 [details]
patch V2

Looks like we'll need a new patch here.
Comment 33 Maciej Stachowiak 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?
Comment 34 Johnny(Jianning) Ding 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.
Comment 35 Johnny(Jianning) Ding 2010-09-15 07:09:25 PDT
Created attachment 67674 [details]
debug snapshot
Comment 36 Johnny(Jianning) Ding 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.
Comment 37 Adam Barth 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 ?
Comment 38 Johnny(Jianning) Ding 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.)
Comment 39 Adam Barth 2010-09-16 00:30:26 PDT
I'm not sure who would be the best plug-in expert.  Maybe ask on #webkit.
Comment 40 Adam Barth 2010-09-28 23:28:12 PDT
Committed r68630: <http://trac.webkit.org/changeset/68630>
Comment 41 Eric Seidel (no email) 2010-09-29 19:14:04 PDT
plugins/plugin-initiate-popup-window.html is broken on Snow Leopard and has been since checkin.
Comment 42 Adam Barth 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.
Comment 43 Robert Hogan 2011-05-02 08:35:57 PDT
Created attachment 91925 [details]
Patch
Comment 44 Eric Seidel (no email) 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).
Comment 45 Robert Hogan 2011-05-10 10:29:19 PDT
Reopening for Qt fix.
Comment 46 Robert Hogan 2011-05-23 12:22:59 PDT
Created attachment 94457 [details]
Patch
Comment 47 Adam Roben (:aroben) 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?
Comment 48 Robert Hogan 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.
Comment 49 Robert Hogan 2011-05-26 14:05:07 PDT
Committed r87429: <http://trac.webkit.org/changeset/87429>
Comment 50 Adam Roben (:aroben) 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.
Comment 51 Adam Roben (:aroben) 2011-05-26 14:07:07 PDT
Why does this bug have "[V8]" in the title? Is it really V8-specific?
Comment 52 Robert Hogan 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.
Comment 53 Robert Hogan 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
Comment 54 Johnny(Jianning) Ding 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.