linux: Adding events to windowless plugins on Linux

Add events to windowless plugins on linux. This CL also refactors the event
communication between WebPlugin and WebPluginDelegate, to use a cross-platform
message based on WebInputEvent.

BUG=8202
TEST=A lot of manual testing on Linux and Windows, with Flash plugins and a
custom plugin that dumps events on Linux.

[Antoine Labour, 2009-05-13 23:13:59]

Several notes:
- I ran into the issue of including intrusive X headers because of npapi.h. I
realized that npapi.h was sometimes included with MOZ_X11 and sometimes without,
leading to different definitions of things like NPEvent or
NPSetWindowCallbackStruct in different compilation units (not good). I decided
to split the parts of npapi.h that actually need the X headers into a separate
file (npapi_x11.h), and always define MOZ_X11 (directly in npapi.h).
- In particular, plugin_message.h was using the wrong version of NPEvent. Since
it needs the actual definition (i.e. X headers) for the ParamTraits<NPEvent>
functions, I un-inlined those and moved them to a .cc file.
- We send FocusIn events, but never FocusOut. I couldn't find an easy way to get
notified about losing focus. Similarly, when we send an EnterNotify/LeaveNotify
event, the focus field isn't correct.

The GraphicsExpose events still don't behave properly. It's particularly
apparent on http://www.communitymx.com/content/source/E5141/wmodetrans.htm .
I'll look into that next.

[Evan Martin, 2009-05-14 21:03:07]

(Sorry, this fell off my radar because you didn't send it to evan@chromium.org. 
Looking now.)

[Evan Martin, 2009-05-14 21:15:04]

This looks pretty good to me.  I'm CCing the plugin master and the Mac plugin
master (mistress?).  John, I marked one place for you to examine in particular
(look for "*** John" in my review comments).

http://codereview.chromium.org/115330/diff/45/55
File chrome/common/plugin_messages.cc (right):

http://codereview.chromium.org/115330/diff/45/55#newcode6
Line 6: #include "third_party/npapi/bindings/npapi_x11.h"
Probably needs to be guarded based on the OS?

http://codereview.chromium.org/115330/diff/45/56
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/115330/diff/45/56#newcode269
Line 269: static void Write(Message* m, const param_type& p);
It'd be good to stick a comment in here about why these messages are special (in
the .cc file instead of the header).

http://codereview.chromium.org/115330/diff/45/52
File third_party/npapi/bindings/npapi_x11.h (right):

http://codereview.chromium.org/115330/diff/45/52#newcode38
Line 38: // This file was split off the original npapi.h, to avoid including
intrusive X
Maybe put this below the #ifndef guards to make it more clear it's not just part
of the license block.  (It took me a while to notice it.)

http://codereview.chromium.org/115330/diff/45/51
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/45/51#newcode45
Line 45: #ifdef OS_LINUX
#if defined(OS_LINUX)

http://codereview.chromium.org/115330/diff/45/51#newcode797
Line 797: }
We typically mark these with "  // anonymous namespace"

http://codereview.chromium.org/115330/diff/45/51#newcode803
Line 803: WebCore::FrameView* parent_view =
static_cast<WebCore::FrameView*>(parent());
Gonna assume this windows branch is identical to previous code and not review
it.

http://codereview.chromium.org/115330/diff/45/51#newcode872
Line 872: // NOTE: Firefox keeps xany.serial and xany.window as 0.
I guess if you're relying on the memset(), then the explicit assignment of False
doesn't matter either.

http://codereview.chromium.org/115330/diff/45/51#newcode924
Line 924: // notifications, but no unfocus.
*** John look here.  Perhaps windowless plugins don't know about focus on
Windows?

http://codereview.chromium.org/115330/diff/45/51#newcode1040
Line 1040: // information.
Does this NOTE here mean that we're matching Firefox and that both we and
Firefox don't have the correct info?

http://codereview.chromium.org/115330/diff/45/51#newcode1046
Line 1046: key_event.keycode = event->keyEvent()->nativeVirtualKeyCode();
I'll be curious to see if this plays out as correct...

http://codereview.chromium.org/115330/diff/45/46
File webkit/glue/webplugin_impl.h (right):

http://codereview.chromium.org/115330/diff/45/46#newcode322
Line 322: bool MouseEventToNPEvent(WebCore::MouseEvent* event, NPEvent
*np_event);
you have X* a, but Y *b.  We usually use the spacing style of the first one. 
here and below.

[jam, 2009-05-14 22:15:17]

http://codereview.chromium.org/115330/diff/45/56
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/115330/diff/45/56#newcode269
Line 269: static void Write(Message* m, const param_type& p);
On 2009/05/14 21:15:04, Evan Martin wrote:
> It'd be good to stick a comment in here about why these messages are special
(in
> the .cc file instead of the header).

Yes please.  What is the reason?  I can't figure it out by looking.

http://codereview.chromium.org/115330/diff/45/57
File chrome/plugin/webplugin_delegate_stub.cc (right):

http://codereview.chromium.org/115330/diff/45/57#newcode17
Line 17: #include "third_party/npapi/bindings/npapi_x11.h"
it seems a little odd seeing npapi_x11.h being included on all platforms.

http://codereview.chromium.org/115330/diff/45/57#newcode17
Line 17: #include "third_party/npapi/bindings/npapi_x11.h"
ditto, this should be surrounded with ifdefs for linux

http://codereview.chromium.org/115330/diff/45/53
File third_party/npapi/bindings/npapi.h (right):

http://codereview.chromium.org/115330/diff/45/53#newcode110
Line 110: #define MOZ_X11 1
is there no way of avoiding more changes to npapi.h?  There's already a
different version in third_party\webkit, this makes it even harder to eventually
only use one.

http://codereview.chromium.org/115330/diff/45/51
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/45/51#newcode892
Line 892: if ((event->type() == WebCore::eventNames().mousemoveEvent)) {
nit: extra brackets

http://codereview.chromium.org/115330/diff/45/51#newcode906
Line 906: (event->type() == WebCore::eventNames().mouseoverEvent)) {
nit: extra brackets as well.  chrome code doesn't add them in this case.

http://codereview.chromium.org/115330/diff/45/51#newcode924
Line 924: // notifications, but no unfocus.
On 2009/05/14 21:15:04, Evan Martin wrote:
> *** John look here.  Perhaps windowless plugins don't know about focus on
> Windows?

Haven't seen any bugs related to it so I don't think it's an issue.

http://codereview.chromium.org/115330/diff/45/51#newcode965
Line 965: 
this function is huge.  do you guys think it's time to add webplugin_impl_win
and webplugin_impl_gtk like what was done for webplugin_delegate_impl?

[Antoine Labour, 2009-05-14 22:18:26]

http://codereview.chromium.org/115330/diff/45/55
File chrome/common/plugin_messages.cc (right):

http://codereview.chromium.org/115330/diff/45/55#newcode6
Line 6: #include "third_party/npapi/bindings/npapi_x11.h"
On 2009/05/14 21:15:04, Evan Martin wrote:
> Probably needs to be guarded based on the OS?

The npapi_x11.h is OS-safe. I don't mind adding the #ifdef, but I find it more
readable this way. Your call.

http://codereview.chromium.org/115330/diff/45/56
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/115330/diff/45/56#newcode269
Line 269: static void Write(Message* m, const param_type& p);
On 2009/05/14 21:15:04, Evan Martin wrote:
> It'd be good to stick a comment in here about why these messages are special
(in
> the .cc file instead of the header).

Done.

http://codereview.chromium.org/115330/diff/45/52
File third_party/npapi/bindings/npapi_x11.h (right):

http://codereview.chromium.org/115330/diff/45/52#newcode38
Line 38: // This file was split off the original npapi.h, to avoid including
intrusive X
On 2009/05/14 21:15:04, Evan Martin wrote:
> Maybe put this below the #ifndef guards to make it more clear it's not just
part
> of the license block.  (It took me a while to notice it.)

Done.

http://codereview.chromium.org/115330/diff/45/51
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/45/51#newcode45
Line 45: #ifdef OS_LINUX
On 2009/05/14 21:15:04, Evan Martin wrote:
> #if defined(OS_LINUX)

Done.

http://codereview.chromium.org/115330/diff/45/51#newcode797
Line 797: }
On 2009/05/14 21:15:04, Evan Martin wrote:
> We typically mark these with "  // anonymous namespace"

Done.

http://codereview.chromium.org/115330/diff/45/51#newcode803
Line 803: WebCore::FrameView* parent_view =
static_cast<WebCore::FrameView*>(parent());
On 2009/05/14 21:15:04, Evan Martin wrote:
> Gonna assume this windows branch is identical to previous code and not review
> it.

Dang, svn diff was doing a better job...
Pretty much the same code. The main difference is that np_event is a pointer,
and I added return true/return false as appropriate.

http://codereview.chromium.org/115330/diff/45/51#newcode872
Line 872: // NOTE: Firefox keeps xany.serial and xany.window as 0.
On 2009/05/14 21:15:04, Evan Martin wrote:
> I guess if you're relying on the memset(), then the explicit assignment of
False
> doesn't matter either.

Done.

http://codereview.chromium.org/115330/diff/45/51#newcode924
Line 924: // notifications, but no unfocus.
On 2009/05/14 21:15:04, Evan Martin wrote:
> *** John look here.  Perhaps windowless plugins don't know about focus on
> Windows?

On Windows, I don't think there is a specific enter/leave message, at least
we're not sending one, we send a WM_MOUSEMOVE instead, which, AFAICT, doesn't
carry the "focus state" information.

http://codereview.chromium.org/115330/diff/45/51#newcode1040
Line 1040: // information.
On 2009/05/14 21:15:04, Evan Martin wrote:
> Does this NOTE here mean that we're matching Firefox and that both we and
> Firefox don't have the correct info?

Correct. I reworded the note.

http://codereview.chromium.org/115330/diff/45/51#newcode1046
Line 1046: key_event.keycode = event->keyEvent()->nativeVirtualKeyCode();
On 2009/05/14 21:15:04, Evan Martin wrote:
> I'll be curious to see if this plays out as correct...

I tested with a custom windowless plugin that logs the events. For the few
keystrokes I tested, I got the same values as Firefox, and corresponds to the
actual X Keycode (that you can see with xev). Thankfully, because finding the
correct X keycode from the DOM event would be really painful.

http://codereview.chromium.org/115330/diff/45/46
File webkit/glue/webplugin_impl.h (right):

http://codereview.chromium.org/115330/diff/45/46#newcode322
Line 322: bool MouseEventToNPEvent(WebCore::MouseEvent* event, NPEvent
*np_event);
On 2009/05/14 21:15:04, Evan Martin wrote:
> you have X* a, but Y *b.  We usually use the spacing style of the first one. 
> here and below.

Done. I also fixed the .cc

[Amanda Walker, 2009-05-14 22:20:20]

On 2009/05/13 23:13:59, Antoine Labour wrote:
> - We send FocusIn events, but never FocusOut. I couldn't find an easy way to
get
> notified about losing focus. Similarly, when we send an
EnterNotify/LeaveNotify
> event, the focus field isn't correct.

This is also true for the Mac--we need to add a LoseFocus method to
WebpluginDelegateImpl (even if the windows delegate doesn't do anything with
it).

[Amanda Walker, 2009-05-14 22:25:01]

http://codereview.chromium.org/115330/diff/66/1047
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/66/1047#newcode964
Line 964: 
Can we push this into the delegate, instead of having big hunks of code
controlled by ifdefs?

http://codereview.chromium.org/115330/diff/66/1047#newcode1052
Line 1052: }
Same comment re: pushing into the delegate as for mouse events.

[jam, 2009-05-14 22:48:35]

http://codereview.chromium.org/115330/diff/66/1047
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/66/1047#newcode964
Line 964: 
On 2009/05/14 22:25:01, Amanda Walker wrote:
> Can we push this into the delegate, instead of having big hunks of code
> controlled by ifdefs?

this can't be pushed to the delegate, since it can't accept any WebCore types
(as the IPC implementation lives outside of webcore).

http://codereview.chromium.org/115330/diff/69/1068
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/115330/diff/69/1068#newcode271
Line 271: // plugin_messages.cc
I still don't understand.  Which structs depend on the X headers on Linux?  The
code looks like it just got moved from the header to the cc file.  Was it never
included before on Linux?  I thought it had been.

[Antoine Labour, 2009-05-14 22:59:56]

http://codereview.chromium.org/115330/diff/45/56
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/115330/diff/45/56#newcode269
Line 269: static void Write(Message* m, const param_type& p);
On 2009/05/14 22:15:17, John Abd-El-Malek wrote:
> On 2009/05/14 21:15:04, Evan Martin wrote:
> > It'd be good to stick a comment in here about why these messages are special
> (in
> > the .cc file instead of the header).
> 
> Yes please.  What is the reason?  I can't figure it out by looking.
 
I added a comment. The implementation of the method needs X headers, including
them breaks the rest of the code base.

http://codereview.chromium.org/115330/diff/45/57
File chrome/plugin/webplugin_delegate_stub.cc (right):

http://codereview.chromium.org/115330/diff/45/57#newcode17
Line 17: #include "third_party/npapi/bindings/npapi_x11.h"
On 2009/05/14 22:15:17, John Abd-El-Malek wrote:
> it seems a little odd seeing npapi_x11.h being included on all platforms.

Done.

http://codereview.chromium.org/115330/diff/45/57#newcode17
Line 17: #include "third_party/npapi/bindings/npapi_x11.h"
On 2009/05/14 22:15:17, John Abd-El-Malek wrote:
> ditto, this should be surrounded with ifdefs for linux

Done.

http://codereview.chromium.org/115330/diff/45/53
File third_party/npapi/bindings/npapi.h (right):

http://codereview.chromium.org/115330/diff/45/53#newcode110
Line 110: #define MOZ_X11 1
On 2009/05/14 22:15:17, John Abd-El-Malek wrote:
> is there no way of avoiding more changes to npapi.h?  There's already a
> different version in third_party\webkit, this makes it even harder to
eventually
> only use one.

For having been through the exercise about 4 to 5 times now... In practice, the
only way that can happen is by either:
1- renaming every occurrence of True, False, Status, Font, etc. (anything
#defined or typedef-ed by X headers) into something else everywhere in the
codebase (including WebKit)
2- only including npapi.h in a very few isolated .cc files, no .h file, which
means changing several interfaces (including some in v8, WebKit ?), and
messages.

2 will probably work best, but it's a long undertaking. I wouldn't like to see
plugins on Linux being held up by that.

http://codereview.chromium.org/115330/diff/45/51
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/45/51#newcode892
Line 892: if ((event->type() == WebCore::eventNames().mousemoveEvent)) {
On 2009/05/14 22:15:17, John Abd-El-Malek wrote:
> nit: extra brackets

Done.

http://codereview.chromium.org/115330/diff/45/51#newcode906
Line 906: (event->type() == WebCore::eventNames().mouseoverEvent)) {
On 2009/05/14 22:15:17, John Abd-El-Malek wrote:
> nit: extra brackets as well.  chrome code doesn't add them in this case.

Done. I fixed the windows version as well.

http://codereview.chromium.org/115330/diff/45/51#newcode965
Line 965: 
On 2009/05/14 22:15:17, John Abd-El-Malek wrote:
> this function is huge.  do you guys think it's time to add webplugin_impl_win
> and webplugin_impl_gtk like what was done for webplugin_delegate_impl?

I can do that.

http://codereview.chromium.org/115330/diff/69/1068
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/115330/diff/69/1068#newcode271
Line 271: // plugin_messages.cc
On 2009/05/14 22:48:35, John Abd-El-Malek wrote:
> I still don't understand.  Which structs depend on the X headers on Linux? 
The
> code looks like it just got moved from the header to the cc file.  Was it
never
> included before on Linux?  I thought it had been.

It was included wrong. See the CL description.
NPEvent is defined differently (void * vs XEvent) whether MOZ_X11 is defined or
not.
In some files, MOZ_X11 was defined. In some other, it was not. See the bunch of
"HACK" comments that I removed in this CL.
The side effect of defining MOZ_X11 is that it includes X headers, that #defines
and typedefs a bunch of symbols that are used in many places (Status, True,
False, Font, etc.), breaking them badly.
Including X headers in plugin_messages.h breaks everything, but X headers are
needed for sizeof(NPEvent) to work.

[Amanda Walker, 2009-05-14 23:02:36]

On 2009/05/14 22:48:35, John Abd-El-Malek wrote:
> http://codereview.chromium.org/115330/diff/66/1047
> File webkit/glue/webplugin_impl.cc (right):
> 
> http://codereview.chromium.org/115330/diff/66/1047#newcode964
> Line 964: 
> On 2009/05/14 22:25:01, Amanda Walker wrote:
> > Can we push this into the delegate, instead of having big hunks of code
> > controlled by ifdefs?
> 
> this can't be pushed to the delegate, since it can't accept any WebCore types
> (as the IPC implementation lives outside of webcore).

Not sure I see how is WebPluginDelegateImpl different from WebPluginImpl in this
regard, but in any case, it seems bad to have large pieces of code here #ifdef'd
per platform.

[Antoine Labour, 2009-05-14 23:10:18]

http://codereview.chromium.org/115330/diff/66/1047
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/66/1047#newcode964
Line 964: 
On 2009/05/14 22:48:35, John Abd-El-Malek wrote:
> On 2009/05/14 22:25:01, Amanda Walker wrote:
> > Can we push this into the delegate, instead of having big hunks of code
> > controlled by ifdefs?
> 
> this can't be pushed to the delegate, since it can't accept any WebCore types
> (as the IPC implementation lives outside of webcore).

For the multi-process case, we may have to change the definition of the plugin
message. Passing an XEvent cross process isn't quite right - there is a pointer
in it (Display*). There are probably similar issues on the mac.
I think it makes sense to keep the platform-specific event hackery in one place
(SetFocus and Paint are in the delegate already).
I intended to look into that change too, but as a separate one, since it affects
all platforms more deeply.

How are events passed between the browser and the renderer ? Maybe we can use
the same types if we can't use the WebCore ones. I need to look into it.

[Amanda Walker, 2009-05-14 23:22:37]

http://codereview.chromium.org/115330/diff/66/1047
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/66/1047#newcode964
Line 964: 
On 2009/05/14 23:10:18, Antoine Labour wrote:
> I intended to look into that change too, but as a separate one, since it
affects
> all platforms more deeply.

Sounds fine.  I don't mean to block on this, just expressing my hatred of #ifdef
:-)

[jam, 2009-05-14 23:24:58]

http://codereview.chromium.org/115330/diff/66/1047
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/66/1047#newcode964
Line 964: 
On 2009/05/14 23:10:18, Antoine Labour wrote:
> On 2009/05/14 22:48:35, John Abd-El-Malek wrote:
> > On 2009/05/14 22:25:01, Amanda Walker wrote:
> > > Can we push this into the delegate, instead of having big hunks of code
> > > controlled by ifdefs?
> > 
> > this can't be pushed to the delegate, since it can't accept any WebCore
types
> > (as the IPC implementation lives outside of webcore).
> 
> For the multi-process case, we may have to change the definition of the plugin
> message. Passing an XEvent cross process isn't quite right - there is a
pointer
> in it (Display*). There are probably similar issues on the mac.

Sure, I'm not saying that XEvent is right.  However sending WebCore types won't
be possible.  Not only is the WebKit API that Darin is working on going to
disallow all src\chrome code from including any WebCore code, but even if you
were to try that now, you'd get tons of compile errors.

> I think it makes sense to keep the platform-specific event hackery in one
place
> (SetFocus and Paint are in the delegate already).
> I intended to look into that change too, but as a separate one, since it
affects
> all platforms more deeply.
> 
> How are events passed between the browser and the renderer ? Maybe we can use
> the same types if we can't use the WebCore ones. I need to look into it.

WebInputEvent is used.  Sure it can be sent to the plugin.  By the time
WebPlugin is called though, it has already been converted to a WebCore type.

http://codereview.chromium.org/115330/diff/69/1068
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/115330/diff/69/1068#newcode271
Line 271: // plugin_messages.cc
On 2009/05/14 22:59:56, Antoine Labour wrote:
> On 2009/05/14 22:48:35, John Abd-El-Malek wrote:
> > I still don't understand.  Which structs depend on the X headers on Linux? 
> The
> > code looks like it just got moved from the header to the cc file.  Was it
> never
> > included before on Linux?  I thought it had been.
> 
> It was included wrong. See the CL description.
> NPEvent is defined differently (void * vs XEvent) whether MOZ_X11 is defined
or
> not.
> In some files, MOZ_X11 was defined. In some other, it was not. 

I see, thanks for the explanation.

> See the bunch of
> "HACK" comments that I removed in this CL.
> The side effect of defining MOZ_X11 is that it includes X headers, that
#defines
> and typedefs a bunch of symbols that are used in many places (Status, True,
> False, Font, etc.), breaking them badly.
> Including X headers in plugin_messages.h breaks everything, but X headers are
> needed for sizeof(NPEvent) to work.

[jam, 2009-05-19 05:50:25]

http://codereview.chromium.org/115330/diff/116/138
File webkit/glue/webplugin_impl.h (right):

http://codereview.chromium.org/115330/diff/116/138#newcode322
Line 322: bool MouseEventToWebMouseEvent(WebCore::MouseEvent* event,
I think these two functions belong in event_conversion.cc

http://codereview.chromium.org/115330/diff/116/139
File webkit/glue/webview_impl.cc (right):

http://codereview.chromium.org/115330/diff/116/139#newcode1050
Line 1050: NOTREACHED();
nit: this kinds of assertions (callee asserting what the caller is doing) aren't
common in the chrome codebase.  i.e. see WebWidget which doesn't do anything
with WebInputEvent::Char.  The places receiving a WebInputEvent already have
code to deal with whether they're receiving an event they care about or not
(i.e. see the "handled=false" code below which propagates this event to the
WebWidget).

[darin (slow to review), 2009-05-19 16:36:22]

http://codereview.chromium.org/115330/diff/116/127
File webkit/api/public/WebInputEvent.h (right):

http://codereview.chromium.org/115330/diff/116/127#newcode50
Line 50: class WebInputEvent {
it seems like there should be some corresponding changes to
WebInputEventFactory.{cpp,mm}

the new fields should be assigned properly even if they aren't required today.

http://codereview.chromium.org/115330/diff/116/127#newcode68
Line 68: NoEvent = -1,
how about Undefined or Unspecified instead?

http://codereview.chromium.org/115330/diff/116/136
File webkit/glue/webplugin_delegate.h (right):

http://codereview.chromium.org/115330/diff/116/136#newcode27
Line 27: }  // namespace WebKit
nit: please drop the closing namespace comment.  it doesn't help readability and
is inconsistent with other similar code in this file and other files in this
directory.

http://codereview.chromium.org/115330/diff/116/136#newcode95
Line 95: virtual bool HandleKeyboardEvent(const WebKit::WebKeyboardEvent& event,
why break this into two functions?  we have the WebInputEvent base class to
allow for a single HandleInputEvent function.  see webwidget.h for instance.

http://codereview.chromium.org/115330/diff/116/137
File webkit/glue/webplugin_impl.cc (right):

http://codereview.chromium.org/115330/diff/116/137#newcode777
Line 777: namespace {
nit: please just use the 'static' keyword on the function in cases like this.

http://codereview.chromium.org/115330/diff/116/137#newcode887
Line 887: bool WebPluginImpl::KeyEventToWebKeyboardEvent(WebCore::KeyboardEvent*
event,
i think it would be good to break these conversion functions out into a separate
file (so they can be more easily unit tested), or at least break them out into
separate static functions (since they don't depend on any members of this)... 
maybe grouped at the top of this file.

maybe you could put these conversion functions in
webkit/glue/event_conversion.{h,cc}

[jam, 2009-05-19 16:45:21]

http://codereview.chromium.org/115330/diff/116/136
File webkit/glue/webplugin_delegate.h (right):

http://codereview.chromium.org/115330/diff/116/136#newcode95
Line 95: virtual bool HandleKeyboardEvent(const WebKit::WebKeyboardEvent& event,
On 2009/05/19 16:36:22, darin wrote:
> why break this into two functions?  we have the WebInputEvent base class to
> allow for a single HandleInputEvent function.  see webwidget.h for instance.

Antonie, correct me if I'm wrong, but I believe the reason is that the derived
classes have different sizes.  I also thought the same thing about the two
functions (it makes it harder to add WebMouseWheelEvent in the future for
example).

I think if we make WebInputEvent have a size() functions that derived classes
implement, at the serialization end we can copy the right number of bits.  At
the deserialization end, we can allocate that much memory and copy the data to
it, and then cast it to a WebInputEvent.  This would make the RenderWidget code
cleaner as well.

[darin (slow to review), 2009-05-20 16:22:05]

In WebInputEventFactory, shouldn't you be setting the new mouse button
modifiers?

http://codereview.chromium.org/115330/diff/217/1184
File webkit/api/public/WebInputEvent.h (right):

http://codereview.chromium.org/115330/diff/217/1184#newcode52
Line 52: WebInputEvent(unsigned int size_param)
nit: WebKit style is to just write "unsigned" instead of "unsigned int".  this
is something they always nit pick.  also, size_param should just be size.

http://codereview.chromium.org/115330/diff/217/1184#newcode108
Line 108: int size;   // The size of this structure, for serialization.
nit: int -> unsigned or size_t

http://codereview.chromium.org/115330/diff/217/1184#newcode234
Line 234: size = sizeof(WebMouseWheelEvent);
nit: how about just using this approach to initialize "size" in all of the
classes.  using a consistent method to assign size would be nice, and this seems
like a reasonable way.  another would be to give all of the constructors an
optional size argument that has a default value of sizeof(WebFooEvent)

http://codereview.chromium.org/115330/diff/217/1173
File webkit/glue/event_conversion.h (right):

http://codereview.chromium.org/115330/diff/217/1173#newcode18
Line 18: class FrameView;
nit: the contents of namespaces are not supposed to be indented.  this was done
wrong originally, and it would be good to fix.

http://codereview.chromium.org/115330/diff/217/1173#newcode57
Line 57: bool MouseEventToWebMouseEvent(WebCore::FrameView* parent_view,
can the input events be passed as const references?

the name parent_view is a little confusing.  maybe containing_view or just view
would be better.  you could say that this is the FrameView corresponding to the
given event.

http://codereview.chromium.org/115330/diff/217/1173#newcode64
Line 64: bool KeyEventToWebKeyboardEvent(WebCore::KeyboardEvent* event,
nit: perhaps the name should be KeyboardEventToWebKeyboardEvent to match the
WebCore::KeyboardEvent name.  however, these names are kind of a mouthful...
ToWebKeyboardEvent and ToWebMouseEvent would be fine too in my opinion.

http://codereview.chromium.org/115330/diff/217/1192
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/115330/diff/217/1192#newcode1094
Line 1094: static_cast<const WebMouseEvent &>(event), np_event);
nit: best to avoid casting reference types.  it has bitten me in the past since
copy-constructors can get invoked.  not that that is a problem here.

nit: there should be no space after WebMouseEvent

[jam, 2009-05-20 21:34:52]

lgtm with darin's nits

http://codereview.chromium.org/115330/diff/217/1175
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/115330/diff/217/1175#newcode295
Line 295: // before validated.
nit: i think this comment is not necessary since lots of other IPC code uses
this same pattern.  I doubt we'll switch to shared memory anytime, but if we
ever do, there'll have to be a lot of changes to the existing IPC traits..

[Antoine Labour, 2009-05-20 23:32:59]

http://codereview.chromium.org/115330/diff/217/1175
File chrome/common/plugin_messages.h (right):

http://codereview.chromium.org/115330/diff/217/1175#newcode295
Line 295: // before validated.
On 2009/05/20 21:34:52, John Abd-El-Malek wrote:
> nit: i think this comment is not necessary since lots of other IPC code uses
> this same pattern.  I doubt we'll switch to shared memory anytime, but if we
> ever do, there'll have to be a lot of changes to the existing IPC traits..

Done.

http://codereview.chromium.org/115330/diff/217/1184
File webkit/api/public/WebInputEvent.h (right):

http://codereview.chromium.org/115330/diff/217/1184#newcode52
Line 52: WebInputEvent(unsigned int size_param)
On 2009/05/20 16:22:05, darin wrote:
> nit: WebKit style is to just write "unsigned" instead of "unsigned int".  this
> is something they always nit pick.  also, size_param should just be size.

Done. I need the parameter to be different from 'size' because otherwise it
would shadow the field. I renamed it sizeParam to be more consistent with the
webkit style.

http://codereview.chromium.org/115330/diff/217/1184#newcode108
Line 108: int size;   // The size of this structure, for serialization.
On 2009/05/20 16:22:05, darin wrote:
> nit: int -> unsigned or size_t

Done.

http://codereview.chromium.org/115330/diff/217/1184#newcode234
Line 234: size = sizeof(WebMouseWheelEvent);
On 2009/05/20 16:22:05, darin wrote:
> nit: how about just using this approach to initialize "size" in all of the
> classes.  using a consistent method to assign size would be nice, and this
seems
> like a reasonable way.  another would be to give all of the constructors an
> optional size argument that has a default value of sizeof(WebFooEvent)

Done. I chose the default value, that way you can initialize it in the
initializer, and makes it somewhat more obvious when you extend with new
classes.

http://codereview.chromium.org/115330/diff/217/1173
File webkit/glue/event_conversion.h (right):

http://codereview.chromium.org/115330/diff/217/1173#newcode18
Line 18: class FrameView;
On 2009/05/20 16:22:05, darin wrote:
> nit: the contents of namespaces are not supposed to be indented.  this was
done
> wrong originally, and it would be good to fix.

Done.

http://codereview.chromium.org/115330/diff/217/1173#newcode57
Line 57: bool MouseEventToWebMouseEvent(WebCore::FrameView* parent_view,
On 2009/05/20 16:22:05, darin wrote:
> can the input events be passed as const references?
> 
> the name parent_view is a little confusing.  maybe containing_view or just
view
> would be better.  you could say that this is the FrameView corresponding to
the
> given event.

Done.

http://codereview.chromium.org/115330/diff/217/1173#newcode64
Line 64: bool KeyEventToWebKeyboardEvent(WebCore::KeyboardEvent* event,
On 2009/05/20 16:22:05, darin wrote:
> nit: perhaps the name should be KeyboardEventToWebKeyboardEvent to match the
> WebCore::KeyboardEvent name.  however, these names are kind of a mouthful...
> ToWebKeyboardEvent and ToWebMouseEvent would be fine too in my opinion.

Done.

http://codereview.chromium.org/115330/diff/217/1192
File webkit/glue/plugins/webplugin_delegate_impl.cc (right):

http://codereview.chromium.org/115330/diff/217/1192#newcode1094
Line 1094: static_cast<const WebMouseEvent &>(event), np_event);
On 2009/05/20 16:22:05, darin wrote:
> nit: best to avoid casting reference types.  it has bitten me in the past
since
> copy-constructors can get invoked.  not that that is a problem here.
> 
> nit: there should be no space after WebMouseEvent

Done.

[darin (slow to review), 2009-05-21 04:25:45]

http://codereview.chromium.org/115330/diff/217/1184
File webkit/api/public/WebInputEvent.h (right):

http://codereview.chromium.org/115330/diff/217/1184#newcode52
Line 52: WebInputEvent(unsigned int size_param)
there isn't a shadowing problem so long as you reference the size member in the
initializer list.  ": size(size)" works and is a common coding pattern.  that
said, it doesn't matter... either way is fine.

[Evan Martin, 2009-05-21 20:59:47]

If y'all will LG, I will commit.

[Evan Martin, 2009-05-21 21:38:07]

error: patch failed: webkit/api/src/win/WebInputEventFactory.cpp:130
error: webkit/api/src/win/WebInputEventFactory.cpp: patch does not apply

Maybe you need to sync?

[Evan Martin, 2009-05-21 22:33:08]

And now I think maybe you need to upload your new patch?

[Evan Martin, 2009-05-21 22:36:56]

BTW, it'd probably be useful to check in your test plugin as well.

[Evan Martin, 2009-05-22 01:29:54]

aargh i got so excited about the tree opening i forgot the credit again.  :\

[darin (slow to review), 2009-05-22 15:08:47]

> aargh i got so excited about the tree opening i forgot the credit again.  :\

I guess that means you committed this?  Can you record the revision number of
the commit here and close the issue?  Thanks!

[Evan Martin, 2009-05-22 15:17:11]

r16708

(The commit message came from my follow-up review, which was this plus fixes to
make it work on Windows.)

[Evan Martin, 2009-05-22 15:17:48]

Whoops, wires crossed.  Commit message was correct, and this was r16692.