Allow keyboard input to work in Mac plugins....

Allow keyboard input to work in Mac plugins.  Filed bug 25183 to revive "figure out why windows needs to invert the setDefaultHandled() logic", which has been a very long-standing question.
BUG=19194
TEST=typing into text fields in plugin areas should
work once they are focused by clicking on the plugin.

jam: primary reviewer
pkasting: FYI, since one of the changes is a TODO(pkasting)
hbono: FYI, since this may improve IME compatibility as well


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

[Amanda Walker, 2009-10-14 21:12:53]

There's something strange going on (see my comments added near pkasting's TODO),
but I was unable to find a single change that worked on both Macs and Windows. 
I think there's a flipped test somewhere else in the code.

[Peter Kasting, 2009-10-14 21:35:18]

Any chance we can get to the bottom of the Windows side of this?  This makes me
so sad :(

http://codereview.chromium.org/269084/diff/1004/1005
File webkit/api/src/WebPluginContainerImpl.cpp (right):

http://codereview.chromium.org/269084/diff/1004/1005#newcode356
Line 356: // backwards, but it matches Safari's code, and if I reverse it,
giving
Nit: It sounds from your comments below like this code no longer matches
Safari's?

[darin (slow to review), 2009-10-14 21:37:11]

http://codereview.chromium.org/269084/diff/1004/1006
File webkit/api/src/WebInputEventConversion.cpp (right):

http://codereview.chromium.org/269084/diff/1004/1006#newcode249
Line 249: uint numChars = event.keyEvent()->text().length();
use std::min?  (that's what webkit folks would normally do)

nit: please use unsigned instead of uint in webkit code

http://codereview.chromium.org/269084/diff/1004/1005
File webkit/api/src/WebPluginContainerImpl.cpp (right):

http://codereview.chromium.org/269084/diff/1004/1005#newcode353
Line 353: WebCursorInfo cursorInfo;
nit: indentation should be 4 white spaces

http://codereview.chromium.org/269084/diff/1004/1005#newcode360
Line 360: #else
instead of forking all of the code, how about just capturing the return value of
handleInputEvent, and then only invert it on mac (or non-mac)?

I suggest a variable named handledEvent:

  bool handledEvent = m_webPlugin->handleInputEvent(webEvent, cursorInfo);

#if !PLATFORM(DARWIN)
  // insert peter's comment and your comment.
  handledEvent = !handledEvent;
#endif

  if (handledEvent)
      event->setDefaultHandled();

NOTE: you cannot use OS_MACOSX in this file.

http://codereview.chromium.org/269084/diff/1004/1005#newcode370
Line 370: event->setDefaultHandled();
nit: indent 4 white spaces

http://codereview.chromium.org/269084/diff/1004/1005#newcode389
Line 389: #else
ditto

[jam, 2009-10-14 21:53:46]

lgtm with the other comments

[Amanda Walker, 2009-10-14 22:04:16]

I'll take another look at windows before checking this in, though if it takes
more than a day or so to chase down, I'm inclined to check it in with the TODO
intact and file a separate (lower priority) bug to track down the discrepancy.

http://codereview.chromium.org/269084/diff/1004/1005
File webkit/api/src/WebPluginContainerImpl.cpp (right):

http://codereview.chromium.org/269084/diff/1004/1005#newcode353
Line 353: WebCursorInfo cursorInfo;
Fixed.

http://codereview.chromium.org/269084/diff/1004/1005#newcode356
Line 356: // backwards, but it matches Safari's code, and if I reverse it,
giving
Edited comment.

http://codereview.chromium.org/269084/diff/1004/1005#newcode360
Line 360: #else
Done.

http://codereview.chromium.org/269084/diff/1004/1005#newcode370
Line 370: event->setDefaultHandled();
Fixed.

http://codereview.chromium.org/269084/diff/1004/1005#newcode389
Line 389: #else
Done.

[Hironori Bono, 2009-10-15 02:26:35]

Greetings,

Thank you for noticing this change.
Your change for "WebInputEventConversion.cpp" is exactly the same one as my
change for Issue 13233. So, it looks great to me. (I don't have good idea about
other part of this change, though.)

By the way, if you don't mind, would it be possible for me to write a UI test
for this issue as well as Issue 13233?

Regards,

Hironori Bono

http://codereview.chromium.org/269084/diff/6001/7002
File webkit/api/src/WebInputEventConversion.cpp (right):

http://codereview.chromium.org/269084/diff/6001/7002#newcode248
Line 248: if (event.keyEvent()->text() != NULL) {
nit: The WebKit style guide (*1) writes "3. Tests for true/false, null/non-null,
and zero/non-zero should all be done without equality comparisons." So this line
should be "if (event->keyEvent()->text())"?

(*1) <http://webkit.org/coding/coding-style.html>

[Amanda Walker, 2009-10-15 02:53:28]

On 2009/10/15 02:26:35, hbono wrote:
> By the way, if you don't mind, would it be possible for me to write a UI test
> for this issue as well as Issue 13233?

If you have a good idea for a UI test for this, please do!  It wasn't very clear
to me how to write a test for this, which is why I didn't include one in this
CL.

> nit: The WebKit style guide (*1) writes "3. Tests for true/false,
null/non-null,
> and zero/non-zero should all be done without equality comparisons." So this
line
> should be "if (event->keyEvent()->text())"?

I had it that way originally, but it caused a compiler error.  Perhaps we have a
build option wrong somewhere, but adding "!= NULL" was how I got it to build.

[darin (slow to review), 2009-10-15 08:10:30]

http://codereview.chromium.org/269084/diff/6001/7002
File webkit/api/src/WebInputEventConversion.cpp (right):

http://codereview.chromium.org/269084/diff/6001/7002#newcode248
Line 248: if (event.keyEvent()->text() != NULL) {
On 2009/10/15 02:26:35, hbono wrote:
> nit: The WebKit style guide (*1) writes "3. Tests for true/false,
null/non-null,
> and zero/non-zero should all be done without equality comparisons." So this
line
> should be "if (event->keyEvent()->text())"?
> 
> (*1) <http://webkit.org/coding/coding-style.html>

Also, PlatformKeyboardEvent::text() returns a String.  It is strange to compare
a String to NULL.

(Note, also that WebKit style uses 0 instead of NULL, but this case should not
use either.)

I suspect you should be using the isEmpty() method.

[Amanda Walker, 2009-10-15 12:34:51]

On 2009/10/15 08:10:30, darin wrote:
> Also, PlatformKeyboardEvent::text() returns a String.  It is strange to
compare
> a String to NULL.

Yes, I misunderstood the comment "Will be null for KeyUp and RawKeyDown events".
 Will change it to simply remove that check and just rely on the length.