Fixing WebKeyboardEvent.

Fixing WebKeyboardEvent. Third time's the charm.

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

[Avi (use Gerrit), 2009-03-02 18:59:47]

OK, this should take care of the layout tests. The only changes this time are a
few tweaks in the event_sending_controller to better match what really happens
with events.

Mark: Please review the gyp file change.
Darin: Please review event_sending_controller, as you've seen the rest already
(twice).

[Avi (use Gerrit), 2009-03-02 19:12:06]

gyp file fixed, as discussed offline.

[darin (slow to review), 2009-03-02 19:13:19]

i only reviewed event_sending_controller.cc

http://codereview.chromium.org/27332/diff/1010/52
File webkit/tools/test_shell/event_sending_controller.cc (right):

http://codereview.chromium.org/27332/diff/1010/52#newcode32
Line 32: #include "webkit/glue/webinputevent_utils.h"
nit: please use _util.h instead of _utils.h to be consistent with existing
naming conventions.

looks like you did not include his new file in the CL.  please be sure to put
its contents in the webkit_glue namespace.  it may not be obvious, but we are
trying to do that for all new code added to webkit/glue.  thanks!

http://codereview.chromium.org/27332/diff/1010/52#newcode386
Line 386: #if defined(OS_WIN)
nit: might be good to drop a comment here to explain this platform difference.

[Mark Mentovai, 2009-03-02 19:16:13]

.gyp file LGTM

http://codereview.chromium.org/27332/diff/88/107
File webkit/tools/test_shell/test_shell.gyp (right):

http://codereview.chromium.org/27332/diff/88/107#newcode91
Line 91: '../../glue/webinputevent_utils.cc',
This is correct!

http://codereview.chromium.org/27332/diff/88/107#newcode93
Line 93: }, {  # OS!=mac
I usually write |{  # else: OS!="mac"| for the comment so that (1) it's obvious
that this is an "else" and (2) when people (me) search for it using code-like
syntax, they'll get the right hit.

(Will there be a GYP style guide?)

[Avi (use Gerrit), 2009-03-02 19:52:23]

All mentioned issues addressed.

[Avi (use Gerrit), 2009-03-02 21:28:41]

On 2009/03/02 19:52:23, Avi wrote:
> All mentioned issues addressed.

Please review.

[darin (slow to review), 2009-03-02 21:56:11]

I gave this a complete once-over...  found some minor issues:

http://codereview.chromium.org/27332/diff/168/1043
File webkit/glue/event_conversion.cc (right):

http://codereview.chromium.org/27332/diff/168/1043#newcode158
Line 158: m_text = webkit_glue::String16ToString(string16(e.text));
oh, these can just be:

m_text = WebCore::String(e.text);

etc.  there's no reason to create the intermediate string16 object.

http://codereview.chromium.org/27332/diff/168/1043#newcode161
Line 161: webkit_glue::StdStringToString(std::string(e.key_identifier));
does e.key_identifier need to be treated as UTF-8 input?  if so, then you want
WebString::fromUTF8(e.key_identifier).  otherwise, if it is ASCII, then
WebString(e.key_identifier) will work.

http://codereview.chromium.org/27332/diff/168/1036
File webkit/glue/webinputevent_mac.mm (right):

http://codereview.chromium.org/27332/diff/168/1036#newcode948
Line 948: NSString* textString = WebCore::textFromEvent(event);
should these be text_string, unmodified_str, ident_str?

http://codereview.chromium.org/27332/diff/168/1036#newcode952
Line 952: // Begin Apple code, copied from KeyEventMac.mm
looks like you need to update the license header.

http://codereview.chromium.org/27332/diff/168/1036#newcode969
Line 969: // Always use 9 for tab -- we don't want to use AppKit's different
character for shift-tab.
nit: 80 chars

http://codereview.chromium.org/27332/diff/168/1038
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/27332/diff/168/1038#newcode344
Line 344: unmodified_text[0] = windows_key_code;
nit: improperly indented

[Avi (use Gerrit), 2009-03-02 22:19:15]

http://codereview.chromium.org/27332/diff/168/1043
File webkit/glue/event_conversion.cc (right):

http://codereview.chromium.org/27332/diff/168/1043#newcode161
Line 161: webkit_glue::StdStringToString(std::string(e.key_identifier));
On 2009/03/02 21:56:11, darin wrote:
> does e.key_identifier need to be treated as UTF-8 input?

They're ASCII.

http://codereview.chromium.org/27332/diff/168/1036
File webkit/glue/webinputevent_mac.mm (right):

http://codereview.chromium.org/27332/diff/168/1036#newcode948
Line 948: NSString* textString = WebCore::textFromEvent(event);
On 2009/03/02 21:56:11, darin wrote:
> should these be text_string, unmodified_str, ident_str?

Copied from Apple code. But I'll rework it for our style.

http://codereview.chromium.org/27332/diff/168/1038
File webkit/glue/webinputevent_win.cc (right):

http://codereview.chromium.org/27332/diff/168/1038#newcode344
Line 344: unmodified_text[0] = windows_key_code;
On 2009/03/02 21:56:11, darin wrote:
> nit: improperly indented

That's a subtle way of putting it; thanks.

[darin (slow to review), 2009-03-02 22:56:40]

LGTM