Bug 58924

Summary: plugins/mouse-click-iframe-to-plugin.html is timing out on mac, win, chromium-mac
Product: WebKit Reporter: Tony Chang <tony>
Component: WebKit Misc.Assignee: noel gordon <noel.gordon>
Status: RESOLVED WONTFIX    
Severity: Normal CC: dpranke, noel.gordon, rniwa, stuartmorgan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
screen-shot-missing-plugin-click-mac-safari-5.0.4
none
Patch
none
Patch eric: review+, webkit.review.bot: commit-queue-

Description Tony Chang 2011-04-19 13:42:47 PDT
plugins/mouse-click-iframe-to-plugin.html, which was added in bug 58419 is failing on mac and win ports.

I'm going to add the test to Skipped for now.
Comment 1 Stuart Morgan 2011-04-19 13:46:06 PDT
The Windows failure is because Flash is missing, based on the output text. Other tests used Flash, so I assumed that would be fine (I can't use the test plugin because it doesn't consume clicks). Do we just need to skip the test there?
Comment 2 Tony Chang 2011-04-19 13:52:14 PDT
Oh, I see.  On Win it failed, rather than timed out.

Jessie, should we check in the failing result or put the test on the Skipped list?
Comment 3 Jessie Berlin 2011-04-19 16:12:27 PDT
If it is skipped on Mac (in the mac/Skipped file), then it will automatically be skipped on Win because win/ inherits from mac/.
Comment 4 Tony Chang 2011-04-19 16:16:09 PDT
(In reply to comment #3)
> If it is skipped on Mac (in the mac/Skipped file), then it will automatically be skipped on Win because win/ inherits from mac/.

Oh, I didn't know that.  Is it possible to run the test on win but not mac?  Should I remove the entry from win/Skipped?

http://trac.webkit.org/changeset/84287
Comment 6 Stuart Morgan 2011-05-04 07:52:14 PDT
This was my first experience writing this kind of test; I thought I understood what I was doing based on the other tests I looked at, and the fact that it worked in local testing, but clearly I'm missing something important. It would be really helpful if someone with a better understanding of these tests could look at it and let me know if I made some n00b mistake.
Comment 7 noel gordon 2011-05-05 01:58:31 PDT
I'll rework the test and attempt to first get it working first on chromium mac.
Comment 8 noel gordon 2011-05-05 02:03:31 PDT
Created attachment 92390 [details]
Patch

Sent this patch to the chromium layout try bots, awaiting results.
Comment 9 noel gordon 2011-05-06 02:42:53 PDT
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/573

2011-05-05 01:56:54,649 13273 worker.py:218 DEBUG worker/1 plugins/mouse-click-iframe-to-plugin.html passed
2011-05-05 01:56:54,649 13273 worker.py:218 DEBUG worker/1 plugins/mouse-click-iframe-to-plugin.html passed
2011-05-05 01:56:54,649 13260 printing.py:538 INFO  plugins/mouse-click-iframe-to-plugin.html -> unexpected pass
Comment 10 Tony Chang 2011-05-06 10:46:05 PDT
Comment on attachment 92390 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92390&action=review

Do you want this patched to be reviewed?

Stuart, does this test rewrite seem fine to you?

> LayoutTests/plugins/mouse-click-iframe-to-plugin.html:9
> +  if (window.layoutTestController)
> +      layoutTestController.dumpAsText();

We normally just left align the <script> contents.

> LayoutTests/plugins/mouse-click-iframe-to-plugin.html:17
> +      var s = getComputedStyle(textarea, null).getPropertyValue("outline-style");
> +      var w = getComputedStyle(textarea, null).getPropertyValue("outline-width");

Using getComputedStyle seems fine, but why didn't frameDocument.activeElement work?
Comment 11 Stuart Morgan 2011-05-06 10:51:09 PDT
(In reply to comment #10)
> Stuart, does this test rewrite seem fine to you?

Without understanding what was wrong with the test as I wrote it, it's hard for me to evaluate a rewrite.
Comment 12 noel gordon 2011-05-10 09:25:11 PDT
(In reply to comment #10)
> Do you want this patched to be reviewed?

Nope, just wanted to first see if I could make chrome-mac co-operate.  This needs a
better test to address the issues reported on bug 58419 - focus the textarea, then
click on the plugin, then type in the textarea.
 
> > LayoutTests/plugins/mouse-click-iframe-to-plugin.html:9
> > +  if (window.layoutTestController)
> > +      layoutTestController.dumpAsText();
> 
> We normally just left align the <script> contents.

I'll fix that in the proceedings.
 
> > LayoutTests/plugins/mouse-click-iframe-to-plugin.html:17
> > +      var s = getComputedStyle(textarea, null).getPropertyValue("outline-style");
> > +      var w = getComputedStyle(textarea, null).getPropertyValue("outline-width");
> 
> Using getComputedStyle seems fine, but why didn't frameDocument.activeElement work?

http://build.webkit.org/old-results/Windows%207%20Release%20(Tests)/r84283%20(11851)/plugins/mouse-click-iframe-to-plugin-actual.txt
MISSING PLUGIN BUTTON PRESSED ...

There's no Flash installed on the test system, so the test was clicked on the missing
plugin and testing the missing plugin code paths, not the code added on r84270.

In Safari win/mac you can load a plugin of type="application/foo-bar", and then click
the missing plugin.  Safari does not move the focus to a missing plugin in my testing
of it.  Screen shot of me clicking the plugin OBJECT tag in Safari 5.0.4 attached.
Comment 13 noel gordon 2011-05-10 09:27:45 PDT
Created attachment 92967 [details]
screen-shot-missing-plugin-click-mac-safari-5.0.4
Comment 14 noel gordon 2011-05-10 23:42:18 PDT
Noting that r84270 requires a windowless plugin, I  wondered if the webkit NPAPI test plugin could help.  I read
over its main.cpp, and it does support windowless operation.  Some DRT debugging on chrome-mac/win confirmed
that webkit sends click events to the test plugin in windowless mode ...
Comment 15 noel gordon 2011-05-10 23:43:55 PDT
Created attachment 93079 [details]
Patch
Comment 17 Stuart Morgan 2011-05-11 08:29:06 PDT
(In reply to comment #14)
> Noting that r84270 requires a windowless plugin, I  wondered if the webkit NPAPI test plugin could help.

See comment 1. It's unsuitable for actually testing what this test is trying to test.
Comment 18 Stuart Morgan 2011-05-11 08:30:35 PDT
(In reply to comment #12)
> There's no Flash installed on the test system, so the test was clicked on the missing
> plugin and testing the missing plugin code paths

That should only be true on Windows; again, see comment 1. Are you only trying to fix this on Windows, or on all platforms?
Comment 19 noel gordon 2011-05-16 00:40:09 PDT
(In reply to comment #17) 
> See comment 1. It's unsuitable for actually testing what this test is trying to test.

Unsuitable because the test plugin doesn't consume click events?  Test plugin events can be logged.  That
was added to resolve bug 11517 "REGRESSION: Flash clicks/interactivity not working properly."

The test for bug 11517 is plugins/mouse-events.html -- logged plugin events seem to be output here:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=plugins%2Fmouse-events.html
Comment 20 Stuart Morgan 2011-05-16 07:40:57 PDT
"Consume" meaning "return true from NPP_HandleEvent"; i.e., tell the browser that the click should not be propagated any further. If the click is propagating in the test then the test doesn't tell us anything about whether or not focus works with a plugin that does consume (stop) click events, because the underlying web view handling could move focus.

Although now that I think about it more, on the Mac the return value is generally ignored. If that's true of all ports on all platforms, I guess we could use the test plugin after all.
Comment 21 noel gordon 2011-05-23 01:00:34 PDT
> Although now that I think about it more, on the Mac the return value is generally ignored. If that's true of all ports on all platforms, I guess we could use the test plugin after all.

And there are good reasons for that ignorance: refer to WebPluginDelegateImpl::PlatformHandleInputEvent

  webplugin_delegate_impl_mac.mm http://goo.gl/WuCtv
  webplugin_delegate_impl_win.cc http://goo.gl/BUQ67
  webplugin_delegate_impl_gtk.cc http://goo.gl/yBmQ8
Comment 22 noel gordon 2011-05-30 00:45:31 PDT
> The test for bug 11517 is plugins/mouse-events.html

Noticed that on the windows ports (chrome/safari), the test plugin doesn't support event logging.  Filed bug 61721 about that.
Comment 23 noel gordon 2011-05-30 00:57:50 PDT
Test fails on WebKit2, but there's and exclusion in place.  See http://trac.webkit.org/changeset/84279
Comment 24 noel gordon 2011-05-30 01:01:16 PDT
For  reference, bug 57439 is similar in that the test there involve clicking on the test plugin.
Comment 25 noel gordon 2011-05-30 01:06:04 PDT
Created attachment 95329 [details]
Patch
Comment 26 Tony Chang 2011-06-02 11:01:52 PDT
Noel: Do we ignore the return value of NPP_HandleEvent on Safari Mac and Safari Win too?

Stuart: If so, is switching to the test plugin OK with you?
Comment 27 Stuart Morgan 2011-06-02 11:05:23 PDT
Yes, the only reason I avoided the test plugin is that I wasn't thinking about the return value being ignored. If it's universally true that it's ignored, I don't have any problem with using the test plugin.
Comment 28 noel gordon 2011-06-03 01:10:07 PDT
> Noel: Do we ignore the return value of NPP_HandleEvent on Safari Mac and Safari Win too?

I don't know, but If we don't, it's a bug.  Flash Win32 always returns 0 for mouse + keyboard events according to the chromium windows code.  I'll go find the code and see what I can make of it ...
Comment 29 Ryosuke Niwa 2011-06-13 18:03:29 PDT
This test intermittently passes on Chromium Mac.
Comment 30 Eric Seidel (no email) 2011-12-02 14:12:24 PST
Comment on attachment 95329 [details]
Patch

OK.
Comment 31 WebKit Review Bot 2011-12-02 14:21:38 PST
Comment on attachment 95329 [details]
Patch

Rejecting attachment 95329 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/test_expectations.txt.rej
patching file LayoutTests/plugins/mouse-click-iframe-to-plugin-expected.txt
patching file LayoutTests/plugins/mouse-click-iframe-to-plugin.html
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/plugins/mouse-click-iframe-to-plugin.html.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10733129
Comment 32 Stephen Chenney 2013-04-09 16:10:43 PDT
LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.