Bug 58924 – plugins/mouse-click-iframe-to-plugin.html is timing out on mac, win, chromium-mac

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Tony Chang]

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.

[Stuart Morgan]

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?

[Tony Chang]

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?

[Jessie Berlin]

If it is skipped on Mac (in the mac/Skipped file), then it will automatically be skipped on Win because win/ inherits from mac/.

[Tony Chang]

(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

[Dirk Pranke]

This is failing on the chromium bots as well ...

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=%20plugins%2Fmouse-click-iframe-to-plugin.html%20&group=%40ToT%20-%20chromium.org

[Stuart Morgan]

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.

[noel gordon]

I'll rework the test and attempt to first get it working first on chromium mac.

[noel gordon]

Created attachment 92390 [details]
Patch

Sent this patch to the chromium layout try bots, awaiting results.

[noel gordon]

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

[Tony Chang]

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?

[Stuart Morgan]

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

[noel gordon]

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

[noel gordon]

Created attachment 92967 [details]
screen-shot-missing-plugin-click-mac-safari-5.0.4

[noel gordon]

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

[noel gordon]

Created attachment 93079 [details]
Patch

[noel gordon]

chromium layout trys
  http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/653 
  http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/585 
  http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/798

[Stuart Morgan]

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

[Stuart Morgan]

(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?

[noel gordon]

(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

[Stuart Morgan]

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

[noel gordon]

> 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

[noel gordon]

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

[noel gordon]

Test fails on WebKit2, but there's and exclusion in place.  See http://trac.webkit.org/changeset/84279

[noel gordon]

For  reference, bug 57439 is similar in that the test there involve clicking on the test plugin.

[noel gordon]

Created attachment 95329 [details]
Patch

[Tony Chang]

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?

[Stuart Morgan]

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.

[noel gordon]

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

[Ryosuke Niwa]

This test intermittently passes on Chromium Mac.

[Eric Seidel (no email)]

Comment on attachment 95329 [details]
Patch

OK.

[WebKit Review Bot]

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

[Stephen Chenney]

LayoutTest failures for Chromium are being marked WontFix. The Bug is still accessible and referenced from TestExpectations.