Implement the NSTextInput protocol....

Implement the NSTextInput protocol.

This change implements the NSTextInput protocol to integrate dead-keys and IME support into Mac Chromium.
Same as Linux, to improve compatibility with Windows Chrome, this change emulates IPC messages sent on Windows when we input characters to fix Issue 11952 and Issue 11981.
Even though I notice we need more work for fixing edge cases (e.g. disabling IMEs on a password input) also on Mac, it is the good starting point. (Supporting edge-cases requires complicated code and it makes hard to review.)

BUG=11952
"IME support is not implemented"
BUG=11981
"Deadkeys do not work"
BUG=16393
"Mac: Not able to insert any letter using "Special Characters" pallet"
TEST=Open a web page which contains an <input> form (e.g. <http://www.google.com/>), type a '[{' key and an 'A' key on a Canadian-French keyboard, and see a Latin character "U+00E2" is displayed in the <input> form.
TEST=Open a web page which contains an <input> form (e.g. <http://www.google.com/>), enable an Chinese Pinyin IME, type a 'W' key, type an 'O' key, and see a Chinese character is displayed in the <input> form.
TEST=Open a web page which contains a <textarea> form, type a return key, and see a new line is inserted.

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

[Hironori Bono, 2009-07-02 07:19:04]

Even though this code works with all IMEs and keyboards installed on Leopard,
I'm wondering if this is my first objective-c++ code and I'm wondering if this
is good implementation.
Please feel free to give me your comments.

[Hironori Bono, 2009-07-06 09:36:53]

Thank you for your review and comments.
Is it possible to review this updated one only if you are convinced of my
answers for your questions sent via an e-mail?

http://codereview.chromium.org/150206/diff/1011/1014
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/150206/diff/1011/1014#newcode134
Line 134: // implementation, we define all variables as public member varibales
of
On 2009/07/02 15:05:06, Avi wrote:
> typo: variables
> 

Done. Sorry for my typo.

http://codereview.chromium.org/150206/diff/1011/1015
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/150206/diff/1011/1015#newcode206
Line 206: // The rendeer updates its IME status.
On 2009/07/02 15:05:06, Avi wrote:
> typo: renderer

Done. Thank you for noticing this.

http://codereview.chromium.org/150206/diff/1011/1015#newcode427
Line 427: // while an input method is compsing a text.
On 2009/07/02 15:05:06, Avi wrote:
> typo:composing
> Why do we need to emulate Windows here? Please explain.

Done. Sorry for my poor description.
Gmail (and many other sites) checks this code in its onkeydown() handlers to
stop auto-completing a string while an input method is composing it. I added a
comment that describes this.

http://codereview.chromium.org/150206/diff/1011/1015#newcode600
Line 600: CFRetain(renderWidgetHostView_->im_attributes_);
On 2009/07/02 15:05:06, Avi wrote:
> This yields a retain count of 2 (one objc, one CF). How does this not leak?

Done. Thank you for noticing this.
Even though I copied this code from WebKit, I'm also wondering if this is
correct. I removed this CFRetain() calls since I confirmed IMEs works without
it.

http://codereview.chromium.org/150206/diff/1011/1015#newcode734
Line 734: // * when we type an ASCII character (without using input methods).
On 2009/07/02 15:05:06, Avi wrote:
> From your comments it sounds like we're getting a keyEvent: as well as an
> insertText:. In both we call ForwardKeyboardEvent. Why does this work?

Sorry for my poor description.
This is a reason why my change uses WebInputEvent::RawKeyDown events and
WebInputEvent::Char events instead of WebInputEvent::KeyDown events.
In brief, a WebInputEvent::KeyDown event consists of a WebInputEvent::RawKeyDown
event and a WebInputEvent::Char event, i.e. when we send a
WebInputEvent::KeyDown event to the renderer, the renderer dispatches this event
to onkeydown() handlers, onkeypress() handlers, and inserts its characters.
Safari uses WebInputEvent::KeyDown events and avoid sending events in
insertText: when the function receives ASCII characters. (It calls WebKit IME
functions when it receives characters composed by an IME.)

On the other hand, this change uses WebInputEvent::RawKeyDown events and
WebInputEvent::Char events directly.
In keyEvent:, we send a WebInputEvent::RawKeyDown event to the renderer so that
the renderer just dispatches this event to onkeydown() JavaScript handlers.
In insertText:, we send a WebInputEvent::Char event to the renderer so that the
renderer dispatches this event to onkeypress() JavaScript handlers and inserts
its characters.
As written in keyEvent:, this change calls interpretKeyEvents: AFTER sending a
WebInputEvent::RawKeyDown event.
Since insertText: is called after an interpretKeyEvents: call, we can ensure
onkeypress() handlers are called AFTER onkeydown() handlers even when we use
IMEs.

http://codereview.chromium.org/150206/diff/1011/1012
File webkit/api/src/mac/WebInputEventFactory.mm (right):

http://codereview.chromium.org/150206/diff/1011/1012#newcode849
Line 849: isKeyUpEvent(event) ? WebInputEvent::KeyUp :
WebInputEvent::RawKeyDown;
On 2009/07/02 15:05:06, Avi wrote:
> Whoa, whoa.

I wrote a long description about this WebInputEvent::RawKeyDown at your comment
in insertText:. Would it be possible to read it?

http://codereview.chromium.org/150206/diff/1011/1012#newcode908
Line 908: WebKeyboardEvent WebInputEventFactory::keyboardEvent(wchar_t
character, int modifiers, double timeStampSeconds)
On 2009/07/02 15:05:06, Avi wrote:
> >80

I'm wondering if we should split this line since this file is under webkit and
we should follow the coding rules of WebKit for this file. If you prefer
splitting this line, I'm going to update my change and split it.

http://codereview.chromium.org/150206/diff/1011/1012#newcode910
Line 910: // keyboardEvent(const GdkEventKey*) depends on the NSEvent object and
On 2009/07/02 15:05:06, Avi wrote:
> GdkEventKey?

Sorry. This is a typo of NSEvent.

[Hironori Bono, 2009-07-14 02:55:52]

darin,

Is it possible to review the WebKit API part of this change? (It is almost the
same as Linux one, though.)

Regards,

[Hironori Bono, 2009-07-17 11:06:49]

ping.

On 2009/07/14 02:55:52, hbono wrote:
> darin,
> 
> Is it possible to review the WebKit API part of this change? (It is almost the
> same as Linux one, though.)
> 
> Regards,

[Avi (use Gerrit), 2009-07-17 13:20:31]

Darin has yet to comment, and while I'm mostly OK with your scheme, you
really would have to update all the comments and rip out the old machinery.
If we're going for split events you can't leave comments in there that say
we're not.

Avi

On Fri, Jul 17, 2009 at 7:06 AM, <hbono@chromium.org> wrote:

> ping.
>
>
> On 2009/07/14 02:55:52, hbono wrote:
>
>> darin,
>>
>
>  Is it possible to review the WebKit API part of this change? (It is
>>
> almost the
>
>> same as Linux one, though.)
>>
>
>  Regards,
>>
>
>
>
> http://codereview.chromium.org/150206
>

[darin (slow to review), 2009-07-17 17:39:06]

WebKit API changes look good to me.  Sorry for the delayed response.

[jeremy, 2009-07-17 17:53:30]

Darin: Sorry if you've answered this already, but how bad do you think a
sync ipc message from the renderer to the browser is for keyboard input?
That's what it looks like we'd need to use the same mechanism as Safari.

Best regards,
Jeremy

On Fri, Jul 17, 2009 at 8:39 PM, <darin@chromium.org> wrote:

>
> WebKit API changes look good to me.  Sorry for the delayed response.
>
>
> http://codereview.chromium.org/150206
>

[darin (slow to review), 2009-07-17 19:51:34]

That would probably be very bad.  Today we send overlapping keystroke events
to the renderer.  If those were each interrupted with a sync IPC back to the
browser UI thread, it would probably result in major key-typing jank.
-Darin

On Fri, Jul 17, 2009 at 10:52 AM, Jeremy Moskovich <jeremy@chromium.org>wrote:

> Darin: Sorry if you've answered this already, but how bad do you think a
> sync ipc message from the renderer to the browser is for keyboard input?
> That's what it looks like we'd need to use the same mechanism as Safari.
>
> Best regards,
> Jeremy
>
> On Fri, Jul 17, 2009 at 8:39 PM, <darin@chromium.org> wrote:
>
>>
>> WebKit API changes look good to me.  Sorry for the delayed response.
>>
>>
>> http://codereview.chromium.org/150206
>>
>
>

[Hironori Bono, 2009-07-22 05:40:18]

Avi,

Thank you for your comments and sorry for my very slow response. (I needed to
focus on Linux code and cannot afford to work for this change. I'm very sorry
not to have answered your comments immediately.)

By the way, I'm wondering about the meaning of your comment because I don't have
good knowledges about Mac code or Objective C. Does this mean I need to create a
new class and rip out my input-method code into the class?

Thank you for your help in advance.

Regards,

Hironori Bono

On 2009/07/17 13:20:31, Avi wrote:
> Darin has yet to comment, and while I'm mostly OK with your scheme, you
> really would have to update all the comments and rip out the old machinery.
> If we're going for split events you can't leave comments in there that say
> we're not.
> 
> Avi
> 
> On Fri, Jul 17, 2009 at 7:06 AM, <hbono@chromium.org> wrote:
> 
> > ping.
> >
> >
> > On 2009/07/14 02:55:52, hbono wrote:
> >
> >> darin,
> >>
> >
> >  Is it possible to review the WebKit API part of this change? (It is
> >>
> > almost the
> >
> >> same as Linux one, though.)
> >>
> >
> >  Regards,
> >>
> >
> >
> >
> > http://codereview.chromium.org/150206
> >
>

[Avi (use Gerrit), 2009-07-22 14:45:43]

What I mean is that there are comments (//, /*) scattered through the code
(mostly WebInputEventFactory) that explain how keyboard events flow. Any
comments that are obsoleted by your code change need to be altered too.

In addition, since you're removing things like keydown, you need to alter
the events to remove them (since no one is using them any more).

Avi

On Wed, Jul 22, 2009 at 1:40 AM, <hbono@chromium.org> wrote:

> Avi,
>
> Thank you for your comments and sorry for my very slow response. (I
> needed to focus on Linux code and cannot afford to work for this change.
> I'm very sorry not to have answered your comments immediately.)
>
> By the way, I'm wondering about the meaning of your comment because I
> don't have good knowledges about Mac code or Objective C. Does this mean
> I need to create a new class and rip out my input-method code into the
> class?
>
> Thank you for your help in advance.
>
> Regards,
>
> Hironori Bono
>
>
> On 2009/07/17 13:20:31, Avi wrote:
>
>> Darin has yet to comment, and while I'm mostly OK with your scheme,
>>
> you
>
>> really would have to update all the comments and rip out the old
>>
> machinery.
>
>> If we're going for split events you can't leave comments in there that
>>
> say
>
>> we're not.
>>
>
>  Avi
>>
>
>  On Fri, Jul 17, 2009 at 7:06 AM, <hbono@chromium.org> wrote:
>>
>
>  > ping.
>> >
>> >
>> > On 2009/07/14 02:55:52, hbono wrote:
>> >
>> >> darin,
>> >>
>> >
>> >  Is it possible to review the WebKit API part of this change? (It is
>> >>
>> > almost the
>> >
>> >> same as Linux one, though.)
>> >>
>> >
>> >  Regards,
>> >>
>> >
>> >
>> >
>> > http://codereview.chromium.org/150206
>> >
>>
>
>
>
>
> http://codereview.chromium.org/150206
>

[Hironori Bono, 2009-07-23 10:57:03]

Hi Avi,

Thank you for your explanation.
Before removing obsoleted comments, I added a comment that describes
keyboard-event flow of Safari and Chromium in render_widget_host_view_mac.mm.

By the way, it seems most comments in WebInputEventFactory.mm are enclosed with
"// Begin Apple code" and "// End Apple code". Is it possible for me to change
them?

Regards,

Hironori Bono

[Hironori Bono, 2009-07-29 10:22:18]

ping.

On 2009/07/23 10:57:03, hbono wrote:
> Hi Avi,
> 
> Thank you for your explanation.
> Before removing obsoleted comments, I added a comment that describes
> keyboard-event flow of Safari and Chromium in render_widget_host_view_mac.mm.
> 
> By the way, it seems most comments in WebInputEventFactory.mm are enclosed
with
> "// Begin Apple code" and "// End Apple code". Is it possible for me to change
> them?
> 
> Regards,
> 
> Hironori Bono

[Avi (use Gerrit), 2009-07-29 14:49:09]

You've already eliminated the use of KeyDown events in favor of splitting them
into RawKeyDown and Char (via IME) in Linux, and this patch does it for the Mac.

It's important that you alter the comments that explain this, and remove code
that's no longer used. As an example, look at webkit/api/public/WebInputEvent.h.
The comment on line 58 would be obsoleted by this patch, and therefore you must
update it with this patch to match what the code is really doing. Similarly, if
we're no longer using KeyDown (line 86) please either remove it or comment why
we're keeping it around, etc.

It's very important to keep the comments up to date with the code.

[Hironori Bono, 2009-07-30 10:24:14]

Thank you so much for your explanation. It totally makes sense.
Since I was focused on files under "mac" directory, I forgot checking the
"webkit/api/public/WebInputEvent.h".
I have updated the comment in "webkit/api/public/WebInputEvent.h", filed an
issue 16393, and added a TODO comment that we need to delete the KeyDown type.
Is it possible to review the updated one?

Regards,

Hironori Bono

[Avi (use Gerrit), 2009-07-30 16:56:31]

Just some tweaking of the comments here and there for a bit of readability. Let
me patch this into my client to play with it, but if nothing big comes up, LGTM.

http://codereview.chromium.org/150206/diff/5008/4030
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/150206/diff/5008/4030#newcode137
Line 137: // the application, That is, an application cannot control an input
method
"// the application. That is..."

http://codereview.chromium.org/150206/diff/5008/4030#newcode146
Line 146: // this C++ class so both the C++ methods and the objective-c methods
can
Capitalize Objective-C

http://codereview.chromium.org/150206/diff/5008/4027
File webkit/api/public/WebInputEvent.h (right):

http://codereview.chromium.org/150206/diff/5008/4027#newcode58
Line 58: // When we use an input method (or an input method editor), we receives
we receive

http://codereview.chromium.org/150206/diff/5008/4027#newcode67
Line 67: // input methods.
A slight rephrasing if you will (smoother English):

To support input methods, Safari sends keydown events to WebKit for filtering.
WebKit sends filtered keydown events back to Safari, which sends them to input
methods.

http://codereview.chromium.org/150206/diff/5008/4027#newcode80
Line 80: // TODO(hbono): Issue 18064: to be removed the KeyDown type since it
Issue 18064: remove the KeyDown type...

http://codereview.chromium.org/150206/diff/5008/4028
File webkit/api/src/mac/WebInputEventFactory.mm (right):

http://codereview.chromium.org/150206/diff/5008/4028#newcode908
Line 908: WebKeyboardEvent WebInputEventFactory::keyboardEvent(wchar_t
character, int modifiers, double timeStampSeconds)
80 col

[Hironori Bono, 2009-07-31 12:03:29]

Hi Avi,

Thank you for your review and comments.
I have updated this change to apply your comments. If you have troubles in
evaluating this change, please feel free to blame me.

Regards,

Hironori Bono

http://codereview.chromium.org/150206/diff/5008/4030
File chrome/browser/renderer_host/render_widget_host_view_mac.h (right):

http://codereview.chromium.org/150206/diff/5008/4030#newcode137
Line 137: // the application, That is, an application cannot control an input
method
On 2009/07/30 16:56:31, Avi wrote:
> "// the application. That is..."

Done. Thank you for fixing my typo.

http://codereview.chromium.org/150206/diff/5008/4030#newcode146
Line 146: // this C++ class so both the C++ methods and the objective-c methods
can
On 2009/07/30 16:56:31, Avi wrote:
> Capitalize Objective-C

Done. Thank you for fixing my typo.

http://codereview.chromium.org/150206/diff/5008/4027
File webkit/api/public/WebInputEvent.h (right):

http://codereview.chromium.org/150206/diff/5008/4027#newcode58
Line 58: // When we use an input method (or an input method editor), we receives
On 2009/07/30 16:56:31, Avi wrote:
> we receive

Done. Thank you for fixing my typo.

http://codereview.chromium.org/150206/diff/5008/4027#newcode67
Line 67: // input methods.
On 2009/07/30 16:56:31, Avi wrote:
> A slight rephrasing if you will (smoother English):
> 
> To support input methods, Safari sends keydown events to WebKit for filtering.
> WebKit sends filtered keydown events back to Safari, which sends them to input
> methods.

Done. Thank you for fixing my poor English.

http://codereview.chromium.org/150206/diff/5008/4027#newcode80
Line 80: // TODO(hbono): Issue 18064: to be removed the KeyDown type since it
On 2009/07/30 16:56:31, Avi wrote:
> Issue 18064: remove the KeyDown type...

Done. Thank you for fixing my typo.

[Avi (use Gerrit), 2009-07-31 13:38:26]

Awesome. Let's get this landed. Do you have commit privileges?

[Hironori Bono, 2009-08-03 08:52:05]

Hi Avi,

Thank you for your review. Your comments are definitely helpful for improving
this change. I have submitted this change as r22262.

Regards,

Hironori Bono

On 2009/07/31 13:38:26, Avi wrote:
> Awesome. Let's get this landed. Do you have commit privileges?

[brettw, 2009-08-24 17:00:25]

On Mon, Aug 24, 2009 at 9:56 AM, Matt Tolton<matt@tolton.com> wrote:
> hbono,
>
> Out of curiosity, why is it ok for you to submit a change like
> http://codereview.chromium.org/150206 apparently without a single line
> of test code, but it's not ok for James to do so?

The reviewer for that patch should have requested a unit test. It's
usually easier to see this need in other's patches than your own,
which is why all reviewers need to be on the lookout for this.

That doesn't affect this patch at all, and I think the tone of the
question was very disrespectful.

Brett