Bug 61721 – Test plugin should support event logging on the windows port.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[noel gordon]

Tests plugins/mouse-events.html plugins/key-events.html require event logging for example.

[noel gordon]

plugins/mouse-events.html mentioned InChromiumBugs http://crbug.com/10351.

[noel gordon]

Created attachment 95419 [details]
Patch wip

[noel gordon]

I should maybe remove the X11 fixup into another bug. Anyho ...

[Alexey Proskuryakov]

> Tests plugins/mouse-events.html plugins/key-events.html require event logging for example.

Does this patch make them pass?

[noel gordon]

Maybe.  I've given up trying to build the win port with my setup.  I can build
chrome windows port though, and can reliably test this change there ...

[noel gordon]

There is an exclusion line for the mouse-events.html test in the chromium
test_expectations.txt file.  I commented that line out and ran the chromium
DRT in debug to see what the test actually outputs ...

  Test for bug 11517: Flash clicks/interactivity not working properly.

Patched in my change and repeated the above. The output is:

  Test for bug 11517: Flash clicks/interactivity not working properly.

No joy.  Add attribute windowPlugin="false" to the <embed> tag so the test
plugin is loaded in windowless mode, and retest.  The output is:

 CONSOLE MESSAGE: line 18: PLUGIN: getFocusEvent
 CONSOLE MESSAGE: line 18: PLUGIN: mouseDown at (20, 20)
 CONSOLE MESSAGE: line 19: PLUGIN: mouseUp at (20, 20)
 CONSOLE MESSAGE: line 21: PLUGIN: mouseDown at (30, 30)
 CONSOLE MESSAGE: line 23: PLUGIN: mouseUp at (40, 30)

 Test for bug 11517: Flash clicks/interactivity not working properly.

 Conclusion: mac ports load plugins in windowless mode by default, chrome
 linux/win load them windowed by default.  Attribute windowPlugin="false"
 on the <embed> forces windowless plugin mode.

[noel gordon]

OK so the keyboard-events.html might work too.  Again there's an exclusion line
in the chromium test_expectations.txt file.  I commented that line out and ran
the chromium DRT in debug _with_ my change.  The output is:

  This test checks if a plug-in can receive keyboard events sent from eventSender. This is a test for Bug 34936.

No joy.  Add attribute windowPlugin="false" to the <embed> as before and retest.
The output is:

  CONSOLE MESSAGE: line 18: PLUGIN: getFocusEvent
  CONSOLE MESSAGE: line 18: PLUGIN: mouseDown at (20, 20)
  CONSOLE MESSAGE: line 19: PLUGIN: mouseUp at (20, 20)
  CONSOLE MESSAGE: line 22: PLUGIN: keyDown 'A'
  CONSOLE MESSAGE: line 22: PLUGIN: keyUp 'A'
  CONSOLE MESSAGE: line 23: PLUGIN: keyDown 'B'
  CONSOLE MESSAGE: line 23: PLUGIN: keyUp 'B'
  CONSOLE MESSAGE: line 24: PLUGIN: keyDown 'C'
  CONSOLE MESSAGE: line 24: PLUGIN: keyUp 'C'

  This test checks if a plug-in can receive keyboard events sent from eventSender. This is a test for Bug 34936.

Windowless mode plugins are needed for these tests, and all ports should at least
test for that -- windowless Flash plugins are common on the web.

[noel gordon]

Created attachment 95600 [details]
Patch for review.

[Tony Chang]

Comment on attachment 95600 [details]
Patch for review.

This looks correct to me.

Events generated by eventSender probably won't get forwarded to windowed plugins since windowed plugins get events from the OS (I guess we could try to make OS events and forward them, but that's probably not worth the effort).  By making it a windowless plugin, WebKit can forward the events to the plugin.

[Tony Chang]

Also, I suspect this will fix the tests on webkit win as well.

[WebKit Commit Bot]

Comment on attachment 95600 [details]
Patch for review.

Clearing flags on attachment: 95600

Committed r87855: <http://trac.webkit.org/changeset/87855>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

(In reply to comment #9)
> This looks correct to me.
>
> Events generated by eventSender probably won't get forwarded to windowed plugins since windowed
> plugins get events from the OS (I guess we could try to make OS events and forward them, but
> that's probably not worth the effort).

Agree + there are windowed plugin (unit) tests in src/webkit/plugins/npapi/test for chromium
and in Tools/DumpRenderTree/TestNetscapePlugIn/win for the webkit win port.


> By making it a windowless plugin, WebKit can forward the events to the plugin.

Yes.  Note I made handleEventWin() return 1.  handleEventX11() and handleEventCarbon() return
0, handleEventCocoa() returns 1.  These become the return value of NPP_HandleEvent() and the
plugin container has to handle it -- return values all over the place, like real plugins :)

Begs the question: should plugin containers trust the NPP_HandleEvent() return value for mouse
and keyboard events?  Nope in general and chromium mentions why:

  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

They ignore the return value, and say the mouse/keyboard event was handled.  That should be
enough to close bug 58924 I believe, could you possibly/please take a look?

[noel gordon]

> Also, I suspect this will fix the tests on webkit win as well.

Concur.

[noel gordon]

> Concur.

Well, maybe for WebKit2, https://bugs.webkit.org/attachment.cgi?id=88749 where
NPP_HandleEvent() returns have started to be ignored (bug 58108), and the same
in the WebKit2 win netscape plugin code from my reading.

https://bugs.webkit.org/show_bug.cgi?id=58108#c2 asked a good question - well
what about WebKit1, are the events ignored there?

[noel gordon]

I got a clean VS2005, setup the all bits & pieces to build webkit win, and built DRT win.
My results:

 1) focus events are not sent through to the windowless plugin - there are no
    "CONSOLE MESSAGE: line 18: PLUGIN: getFocusEvent" in the DRT test results.

 2) the expected mouse events are logged to the DRT result _only_ if I return 0
    from the NPP_HandleEvent() call.

So focus is not working, but why is a return 0 needed?

The keyboard handling
  http://trac.webkit.org/browser/trunk/WebCore/plugins/win/PluginViewWin.cpp?rev=67854#L656
and the mouse event handling
  http://trac.webkit.org/browser/trunk/WebCore/plugins/win/PluginViewWin.cpp?rev=67854#L724

read the result of dispatchNPEvent(), which is the return the result of NPP_HandleEvent(),
and so I see why ...

   if (!dispatchNPEvent(npEvent))
     event->setDefaultHandled();

The event is marked handled only if the plugin returned 0 from NPP_HandleEvent().  Since
that is the expectation, and since chromium ignores the event return, this suggests a new
patch to get mouse event logging working on webkit win ...

[noel gordon]

Created attachment 96257 [details]
Patch Part #2

[noel gordon]

FYI chromium DRT plugins/{mouse-events.html,key-events.html} still pass with this change.

[noel gordon]

(In reply to comment #18)
Meant plugins/{mouse-events.html,keyboard-events.html}.

[noel gordon]

Build webkit win at tip just now, and ran the mouse-events.html test

 CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (20, 20)
 CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (20, 20)
 CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (30, 30)
 CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (40, 30)

 Test for bug 11517: Flash clicks/interactivity not working properly.

Ok, mouse event logged at last.

[noel gordon]

.. and keyboard-events.html is looking better also. 

  CONSOLE MESSAGE: line 0: PLUGIN: mouseDown at (20, 20)
  CONSOLE MESSAGE: line 0: PLUGIN: mouseUp at (20, 20)
  CONSOLE MESSAGE: line 0: PLUGIN: keyDown 'A'
  CONSOLE MESSAGE: line 0: PLUGIN: keyUp 'A'
  CONSOLE MESSAGE: line 0: PLUGIN: keyDown 'B'
  CONSOLE MESSAGE: line 0: PLUGIN: keyUp 'B'
  CONSOLE MESSAGE: line 0: PLUGIN: keyDown 'C'
  CONSOLE MESSAGE: line 0: PLUGIN: keyUp 'C'

  This test checks if a plug-in can receive keyboard events sent from eventSender. This is a test for Bug 34936.(In reply to

[noel gordon]

Created attachment 96429 [details]
Patch Part #2.1

Add focus fix.  Layout test failures noted in the win port DRT, in fast/css/* for example.
But I'm not touching CSS here, so maybe that's normal?  Not sure.

[Tony Chang]

Noel: I would make a new bug for the fixes to the Win port.

Good people to CC for a review or feedback are [e-mail] and [e-mail].

[noel gordon]

(In reply to comment #23)
> Noel: I would make a new bug for the fixes to the Win port.
> 
> Good people to CC for a review or feedback are [e-mail] and [e-mail].

Right thx, will split the Win port change out in a new bug.

[noel gordon]

Patch Part #2 will do for the current bug therefore, r? cq?

[noel gordon]

Filed bug 62375 for the fixes to the Win port.

[Tony Chang]

Comment on attachment 96257 [details]
Patch Part #2

I guess this change doesn't have tests because they are covered by the fixes in 62375?  That seems OK to me.

[WebKit Review Bot]

Comment on attachment 96257 [details]
Patch Part #2

Clearing flags on attachment: 96257

Committed r88465: <http://trac.webkit.org/changeset/88465>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[noel gordon]

> I guess this change doesn't have tests because they are covered by the fixes in 62375?  That seems OK to me.

Correct.