Fixing WebKeyboardEvent.

Fixing WebKeyboardEvent.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=10360

[Avi (use Gerrit), 2009-02-23 19:20:35]

First pass at WebKeyboardEvent. This is an early preview; this isn't going to be
committed like this, but this is the basic idea. I'm working out the kinks and
verifying things, but I wanted some early feedback, if this is what you guys had
in mind.

http://codereview.chromium.org/27056/diff/1/17
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/27056/diff/1/17#newcode1185
Line 1185: (event.type == WebInputEvent::XXNONRAWXX_KEY_DOWN) ||
I'll be taking off the "XXNONRAWXX" prefix before any final commit, but I wanted
to find everywhere that enum value was used.

[Evan Martin, 2009-02-23 19:37:33]

thanks again for doing this!

http://codereview.chromium.org/27056/diff/1/11
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/1/11#newcode129
Line 129: // Caps on string lengths so we can make them static arrays and keep
them PODs.
Can we not just write custom serialization code for this like we have elsewhere?
 The only reason I was opposed to custom serialization before is if someone was
attempting to serialize platform-specific types...

http://codereview.chromium.org/27056/diff/1/11#newcode141
Line 141: int windows_key_code;
I can imagine why you renamed here but for what it's worth, it's also a "web"
key code in that it's the code expected by websites.  So it's kind of implicit
from WebKeyboardEvent::key_code .

But putting a 'windows' in there will probably make our other code clearer
anyway.

http://codereview.chromium.org/27056/diff/1/11#newcode167
Line 167: // References to the original event.
I think we don't want these.

[Avi (use Gerrit), 2009-02-23 19:51:43]

http://codereview.chromium.org/27056/diff/1/11
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/1/11#newcode129
Line 129: // Caps on string lengths so we can make them static arrays and keep
them PODs.
On 2009/02/23 19:37:33, Evan Martin wrote:
> Can we not just write custom serialization code for this like we have
elsewhere?

No. We have a parent class and three subtypes. If we want to send them over the
wire we either need to:

- Have one message for each type, and teach IPC to serialize each type
separately.
- Write a factory so that the base type can deserialize to one of the three
subtypes at the other end.
- Keep the type a POD and slam bits over the wire.

We're already doing the third, and Darin said that it would be the best to just
keep doing that way. It doesn't thrill me but it's not so simple...

http://codereview.chromium.org/27056/diff/1/11#newcode141
Line 141: int windows_key_code;
On 2009/02/23 19:37:33, Evan Martin wrote:
> I can imagine why you renamed here but for what it's worth, it's also a "web"
> key code in that it's the code expected by websites.

I'm trying to make WebKeyboardEvent parallel PlatformKeyboardEvent, and PKE has
"windowsVirtualKeyCode".

http://codereview.chromium.org/27056/diff/1/11#newcode167
Line 167: // References to the original event.
On 2009/02/23 19:37:33, Evan Martin wrote:
> I think we don't want these.

I think we need them whether or not we want them. See
WebContentsViewWin::HandleKeyboardEvent.

[darin (slow to review), 2009-02-23 20:56:40]

http://codereview.chromium.org/27056/diff/1/11
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/1/11#newcode167
Line 167: // References to the original event.
the windows actual_message is echoed back to the browser, but it is just data. 
the mac and gdk objects are heap allocated, so i don't see how these are the
same as actual_message.  how is the memory managed for actual_event?

[Evan Martin, 2009-02-23 21:02:50]

http://codereview.chromium.org/27056/diff/1/11
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/1/11#newcode167
Line 167: // References to the original event.
On 2009/02/23 19:51:43, Avi wrote:
> I think we need them whether or not we want them. See
> WebContentsViewWin::HandleKeyboardEvent.

Ah, this is the badness I was talking about.  We can talk about it outside of
this review.

[Avi (use Gerrit), 2009-02-23 21:27:14]

http://codereview.chromium.org/27056/diff/1/11
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/1/11#newcode167
Line 167: // References to the original event.
On 2009/02/23 20:56:40, darin wrote:
> the mac and gdk objects are heap allocated, so i don't see how these are the
> same as actual_message.

They're pointers to heap data. I see three scenarios:

1. WKE passed from browser to renderer. The renderer doesn't care/use the field.
2. WKE passed from renderer to browser. The fields are null.
3. WKE passed from browser to renderer and back to browser. That happens in one
message cycle, right? HandleKeyboardEvent is called from UnhandledInputEvent,
called from OnMsgInputEventAck. Hmmm... maybe not.

I can drop the Mac/Linux events from here, but I'm not sure how to get the
Windows MSG out. It really does need to go, though.

[darin (slow to review), 2009-02-23 21:29:34]

http://codereview.chromium.org/27056/diff/1/11
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/1/11#newcode167
Line 167: // References to the original event.
A while back, Dean pointed out how the usage of MSG here is problematic for
other reasons.  It allows a corrupt renderer to send arbitrary unhandled
keystrokes up to the browser.  The better solution is for the browser to keep
track of the keystroke, and just have the renderer refer to the event in a more
indirect fashion.

[Evan Martin, 2009-02-23 21:33:02]

On 2009/02/23 21:29:34, darin wrote:
> http://codereview.chromium.org/27056/diff/1/11
> File webkit/glue/webinputevent.h (right):
> 
> http://codereview.chromium.org/27056/diff/1/11#newcode167
> Line 167: // References to the original event.
> A while back, Dean pointed out how the usage of MSG here is problematic for
> other reasons.  It allows a corrupt renderer to send arbitrary unhandled
> keystrokes up to the browser.  The better solution is for the browser to keep
> track of the keystroke, and just have the renderer refer to the event in a
more
> indirect fashion.

We should start a separate thread about this.  I don't redesigning this should
block Avi's change.  I would be ok with leaving in the Windows message and leave
out the Mac/Linux ones.

This means accelerators won't work, but it'd be useful for the mac port to have
keyboard work at all first.  :)

[Avi (use Gerrit), 2009-02-23 21:33:52]

http://codereview.chromium.org/27056/diff/1/11
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/1/11#newcode167
Line 167: // References to the original event.
On 2009/02/23 21:29:34, darin wrote:
> A while back, Dean pointed out how the usage of MSG here is problematic for
> other reasons.

I can see that; I was hoping that was out of scope for this particular change.
Can I leave the MSG in for now and try to tackle it later?

[darin (slow to review), 2009-02-23 21:36:55]

http://codereview.chromium.org/27056/diff/1/11
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/1/11#newcode167
Line 167: // References to the original event.
Absolutely!

[Avi (use Gerrit), 2009-02-24 22:10:30]

OK, so this is tested and works on all platforms. We're a lot more streamlined.
I'm not sure this is doing the absolute correct thing, but it should be a lot
closer.

[darin (slow to review), 2009-02-24 22:24:36]

Looks good overall...

http://codereview.chromium.org/27056/diff/95/1084
File webkit/glue/event_conversion.cc (right):

http://codereview.chromium.org/27056/diff/95/1084#newcode159
Line 159: webkit_glue::StdStringToString(std::string(e.key_identifier));
looks like you assume null termination.  better use base::strlcpy when creating
key_identifier, etc.

http://codereview.chromium.org/27056/diff/95/1083
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/95/1083#newcode9
Line 9: #include "base/string16.h"
nit: extraneous include

http://codereview.chromium.org/27056/diff/95/1083#newcode69
Line 69: MERGED_KEY_DOWN,
it would be nice to keep these names in sync with the values of the Type enum
under PlatformKeyboardEvent.  so, i think that means that MERGED_KEY_DOWN should
just be KEY_DOWN.

http://codereview.chromium.org/27056/diff/95/1083#newcode141
Line 141: #define kTextLengthCap        4
nit: please follow google style in this file.

http://codereview.chromium.org/27056/diff/95/1076
File webkit/glue/webinputevent_mac.mm (right):

http://codereview.chromium.org/27056/diff/95/1076#newcode9
Line 9: #include "base/sys_string_conversions.h"
is this header needed?

http://codereview.chromium.org/27056/diff/95/1078
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/27056/diff/95/1078#newcode349
Line 349: strncpy(key_identifier, key_identifier_str.c_str(),
kIdentifierLengthCap);
strncpy -> base::strlcpy?  or is it okay for this to not be null terminated?

http://codereview.chromium.org/27056/diff/95/1080
File webkit/glue/webinputeventutils.cc (right):

http://codereview.chromium.org/27056/diff/95/1080#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
nit: fix copyright

http://codereview.chromium.org/27056/diff/95/1074
File webkit/glue/webinputeventutils.h (right):

http://codereview.chromium.org/27056/diff/95/1074#newcode5
Line 5: #ifndef WEBKIT_GLUE_WEBINPUTEVENTUTILS_H_
nit: how about webinputevent_util.h instead?  that would be more consistent with
existing file naming.  alternatively, it would be nice to give this file a more
specific name that corresponds better to its contents.

http://codereview.chromium.org/27056/diff/95/1074#newcode11
Line 11: #include "base/string16.h"
nit: both of these includes are unnecessary

[Evan Martin, 2009-02-24 22:24:48]

This looks pretty good to me, but I think it falls on Darin (sorry!) to be the
ultimate decider.  Minor comments follow.

http://codereview.chromium.org/27056/diff/95/1083
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/95/1083#newcode51
Line 51: // respectively. In Cocoa and the various incarnations of Linux, key
events
s/the various incarnations of Linux/Gtk/

http://codereview.chromium.org/27056/diff/95/1074
File webkit/glue/webinputeventutils.h (right):

http://codereview.chromium.org/27056/diff/95/1074#newcode13
Line 13: // By virtue of having historical similarities, the Windows and Linux
code
I don't understand this comment.  Historical within Chrome?  Or?

[Avi (use Gerrit), 2009-02-24 23:05:37]

Fixed unless otherwise noted.

http://codereview.chromium.org/27056/diff/95/1084
File webkit/glue/event_conversion.cc (right):

http://codereview.chromium.org/27056/diff/95/1084#newcode159
Line 159: webkit_glue::StdStringToString(std::string(e.key_identifier));
Fixed.

http://codereview.chromium.org/27056/diff/95/1083
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/95/1083#newcode9
Line 9: #include "base/string16.h"
On 2009/02/24 22:24:36, darin wrote:
> nit: extraneous include

How so?

http://codereview.chromium.org/27056/diff/95/1083#newcode141
Line 141: #define kTextLengthCap        4
On 2009/02/24 22:24:36, darin wrote:
> nit: please follow google style in this file.

In what way am I not?

http://codereview.chromium.org/27056/diff/95/1074
File webkit/glue/webinputeventutils.h (right):

http://codereview.chromium.org/27056/diff/95/1074#newcode5
Line 5: #ifndef WEBKIT_GLUE_WEBINPUTEVENTUTILS_H_
On 2009/02/24 22:24:36, darin wrote:
> nit: how about webinputevent_util.h instead?

I can do that. A more specific name is difficult; I want a place for common code
but it's not entirely common.

http://codereview.chromium.org/27056/diff/95/1074#newcode13
Line 13: // By virtue of having historical similarities, the Windows and Linux
code
On 2009/02/24 22:24:48, Evan Martin wrote:
> I don't understand this comment.  Historical within Chrome?  Or?

Rewritten.

[darin (slow to review), 2009-02-24 23:15:19]

http://codereview.chromium.org/27056/diff/95/1083
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/95/1083#newcode9
Line 9: #include "base/string16.h"
doh, nevermind... i see that you are using char16.  i did a dumb search for
string16 and didn't find any hits :(

http://codereview.chromium.org/27056/diff/95/1083#newcode141
Line 141: #define kTextLengthCap        4
macros should be named FOO_BAR not kFooBar

also, these should probably not be macros.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Macro_...

[Avi (use Gerrit), 2009-02-25 14:35:19]

http://codereview.chromium.org/27056/diff/95/1083
File webkit/glue/webinputevent.h (right):

http://codereview.chromium.org/27056/diff/95/1083#newcode141
Line 141: #define kTextLengthCap        4
On 2009/02/24 23:15:19, darin wrote:
> macros should be named FOO_BAR not kFooBar

But it's a constant... in any case, I was wrong. I thought that |const xxx|
constants weren't good enough to declare array lengths. They are; changed.

[darin (slow to review), 2009-02-25 15:59:40]

LGTM