Integrating GtkIMContext into the RenderWidgetHostViewGtk class....

Integrating GtkIMContext into the RenderWidgetHostViewGtk class.This change implements signal handers of the GtkIMContext object to support IMEs and dead-keys. Also, to improve compatibility with Windows Chrome, this change emulates IPC messages sent on Windows when we input characters and fixes Issue 13604 as well as Issue 10953 and 11226. Even though I notice we need more work for fixing edge cases (e.g. disabling IMEs on a password input) on Linux, I think this is the good starting point. (Supporting edge-cases requires complicated code and it makes hard to review.)

BUG=10953
"IME support"
BUG=11226
"Dead keys and accents input not working"
BUG=13604
"Hotkeys not working in non-us keyboard layout"
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=Change the keyboard layout to Hebrew (or Russian), open a web page which contains an <input> form, input some characters in the <input> form, type control+a, and see the text in the <input> form is selected.

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

[Evan Martin, 2009-06-15 16:31:51]

+estade and dean, who have looked at input stuff

At first glance this looks really good.  I will do a real review in a few hours.

[Evan Martin, 2009-06-15 17:53:43]

I think we need Darin to look at the webkit api changes.

http://codereview.chromium.org/126118/diff/1/6
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/126118/diff/1/6#newcode262
Line 262: // Send an IME event to the renderer.
Are these overrides of a parent class?  In that case, could you mark them
virtual?  Otherwise, could you add comments to each one describing what it does
and what the arguments are?

http://codereview.chromium.org/126118/diff/1/3
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/126118/diff/1/3#newcode486
Line 486: // TODO(hbono): this code is not perfect.
Could you describe what needs to be done in this TODO?  No code is perfect.  :)

http://codereview.chromium.org/126118/diff/1/5
File chrome/browser/renderer_host/render_widget_host_view_gtk.h (right):

http://codereview.chromium.org/126118/diff/1/5#newcode124
Line 124: // an IME. To emulate this behavior, we shoul dmonitor the status of
this
typo: "should"

http://codereview.chromium.org/126118/diff/1/5#newcode128
Line 128: bool im_is_composing_;
Could you document this member variable?  (I'm a little surprised you can't get
this state out of the GtkIMContext*...)

http://codereview.chromium.org/126118/diff/1/2
File webkit/api/src/gtk/WebInputEventFactory.cpp (right):

http://codereview.chromium.org/126118/diff/1/2#newcode183
Line 183: result.type = WebInputEvent::RawKeyDown;
I'm not sure this is correct.  How does a normal (non-IME) key press ever become
a character event after this change?  I really don't understand this part of the
code very well, unfortunately.  :(

[Evan Stade, 2009-06-15 18:32:32]

http://codereview.chromium.org/126118/diff/1/3
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/126118/diff/1/3#newcode71
Line 71: // Create an GtkIMContext instance and attach its signal handlers.
s/an/a

http://codereview.chromium.org/126118/diff/1/3#newcode73
Line 73: if (host_view->im_context_) {
is this check necessary? When will im_context_ be null? I see that you
frequently check

if (host_view->im_context_)

but after reading the code for gtk_im_multicontext_new I don't understand how it
could be null.

http://codereview.chromium.org/126118/diff/1/3#newcode125
Line 125: // It will send a "commit" signal it has a character to be inserted
even
s/signal it/signal when it

http://codereview.chromium.org/126118/diff/1/3#newcode303
Line 303: // hanlders even though WebInputEventFactory::keyboardEvent() heavily
s/hanlders/handlers
s/GtkEventKey/GdkEventKey

I think this whole comment is rather confusing. Can you move this code to
WebInputEventFactory?

http://codereview.chromium.org/126118/diff/1/3#newcode338
Line 338: RenderWidgetHostViewGtk::~RenderWidgetHostViewGtk() {
don't we want to unref the im_context_?

[Hironori Bono, 2009-06-16 07:47:09]

Thank you for your comments.
Even though I added (and updated) comments in this change, please feel free to
give me your comments, questions, and thoughts. (I think it is important for you
to figure out the behaviors of the GtkIMContext object for future changes.)

http://codereview.chromium.org/126118/diff/1/6
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/126118/diff/1/6#newcode262
Line 262: // Send an IME event to the renderer.
Sorry for confusing you with the lack of my comments.
I just moved some code that should be shared among
RenderWidgetHostView{Win,Gtk,Mac} classes here not only for Linux but also for
Mac and Windows. (I'm going to refactor Windows code with the functions and use
them for implementing IME support on Mac.) So, they are not virtual functions.
To describe the functions, I also added some comments about them.

http://codereview.chromium.org/126118/diff/1/3
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/126118/diff/1/3#newcode71
Line 71: // Create an GtkIMContext instance and attach its signal handlers.
On 2009/06/15 18:32:32, Evan Stade wrote:
> s/an/a

Done. Thank you for noticing this.

http://codereview.chromium.org/126118/diff/1/3#newcode73
Line 73: if (host_view->im_context_) {
On 2009/06/15 18:32:32, Evan Stade wrote:
> is this check necessary? When will im_context_ be null? I see that you
frequently check
> 
> if (host_view->im_context_)
> 
> but after reading the code for gtk_im_multicontext_new I don't understand how
it could be null.

Done. I implemented this change without reading the code code for
gtk_im_multicontext_new(). (I only read through the document of GTK to implement
it yesterday.) FWIW, I added this check because I'm not sure the implementation
of gtk_im_multicontext_new().
Even though I removed some NULL checks of |im_context_|, I'm wondering whether
or not we should remove the NULL checks in the RenderWidgetHostViewGtk member
functions since I'm not sure they are called only after creating |im_context_|.

http://codereview.chromium.org/126118/diff/1/3#newcode125
Line 125: // It will send a "commit" signal it has a character to be inserted
even
On 2009/06/15 18:32:32, Evan Stade wrote:
> s/signal it/signal when it

Done. Thank you for noticing this.

http://codereview.chromium.org/126118/diff/1/3#newcode303
Line 303: // hanlders even though WebInputEventFactory::keyboardEvent() heavily
On 2009/06/15 18:32:32, Evan Stade wrote:
> s/hanlders/handlers
> s/GtkEventKey/GdkEventKey
> 
> I think this whole comment is rather confusing. Can you move this code to
> WebInputEventFactory?

Done. Moved this comment to at the beginning of
WebInputEventFactory::keyboardEvent().

http://codereview.chromium.org/126118/diff/1/3#newcode338
Line 338: RenderWidgetHostViewGtk::~RenderWidgetHostViewGtk() {
On 2009/06/15 18:32:32, Evan Stade wrote:
> don't we want to unref the im_context_?

Done. Yes, we need a g_object_unref() call here. Thank you for noticing this.

http://codereview.chromium.org/126118/diff/1/3#newcode486
Line 486: // TODO(hbono): this code is not perfect.
On 2009/06/15 17:53:43, Evan Martin wrote:
> Could you describe what needs to be done in this TODO?  No code is perfect. 
:)

Done. I have updated the comments in this function. Sadly, we need more code,
though. :)

http://codereview.chromium.org/126118/diff/1/5
File chrome/browser/renderer_host/render_widget_host_view_gtk.h (right):

http://codereview.chromium.org/126118/diff/1/5#newcode124
Line 124: // an IME. To emulate this behavior, we shoul dmonitor the status of
this
On 2009/06/15 17:53:43, Evan Martin wrote:
> typo: "should"

Done. Thank you.

http://codereview.chromium.org/126118/diff/1/5#newcode128
Line 128: bool im_is_composing_;
On 2009/06/15 17:53:43, Evan Martin wrote:
> Could you document this member variable?  (I'm a little surprised you can't
get
> this state out of the GtkIMContext*...)

Done. I agree this variable is confusing. (GtkIMContext is definitely for
composing a text.) I renamed this variable and added its comments.

http://codereview.chromium.org/126118/diff/1/2
File webkit/api/src/gtk/WebInputEventFactory.cpp (right):

http://codereview.chromium.org/126118/diff/1/2#newcode183
Line 183: result.type = WebInputEvent::RawKeyDown;
On 2009/06/15 17:53:43, Evan Martin wrote:
> I'm not sure this is correct.  How does a normal (non-IME) key press ever
become a character event after this change?  I really don't understand this part
of the code very well, unfortunately.  :(

This change creates a Char event without calling this function in
RenderWidgetHostViewGtkWidget::ForwardCharEvent() and send it when GtkIMContext
sends a "commit" signal in RenderWidgetHostViewGtkWidget::InputMethodCommit().
GtkIMContext signals don't use GdkEventKey objects and I don't have any good
idea how to handle this case in this function.
Even though I have added a comment which describes this, it is definitely
helpful to give me your thoughts.

[Evan Stade, 2009-06-16 19:17:30]

lgtm

http://codereview.chromium.org/126118/diff/37/1037
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/126118/diff/37/1037#newcode485
Line 485: if (im_context_) {
in the interest of keeping things as untabbed as possible, can you make this
check

if (!im_context_)
  return;

http://codereview.chromium.org/126118/diff/37/1036
File webkit/api/src/gtk/WebInputEventFactory.cpp (right):

http://codereview.chromium.org/126118/diff/37/1036#newcode178
Line 178: // that we can create a WebKeyboardEvent instance without a
GdkEventKey
what I meant to ask was that you create a second function

WebInputFactory::keyboardEvent(wchar_t character) {

}

that creates the event for you. It's just a suggestion though, if you feel it
won't be helpful anywhere else you can leave the code where it is. In either
case though this comment should go with the code. Sorry for the confusion.

[Hironori Bono, 2009-06-17 02:04:26]

Thank you for your comment.
Even though I prefer your option, I would like to hear opinions of WebKit
experts before implementing your comment since I don't have good knowledge about
WebKit.

http://codereview.chromium.org/126118/diff/37/1036
File webkit/api/src/gtk/WebInputEventFactory.cpp (right):

http://codereview.chromium.org/126118/diff/37/1036#newcode178
Line 178: // that we can create a WebKeyboardEvent instance without a
GdkEventKey
On 2009/06/16 19:17:30, Evan Stade wrote:
> what I meant to ask was that you create a second function
> WebInputFactory::keyboardEvent(wchar_t character) {
> }

Even though we should use WebKit coding style for this file, I'm a little
wondering if it is OK to create overloaded functions in this file. (An you know,
the coding style of Google allows overloaded functions only "in cases where
input can be specified in different types that contain the same information":
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...)

Even though I personally prefer this option, I would like to hear opinions of
WebKit experts.

[Evan Stade, 2009-06-17 02:09:24]

http://codereview.chromium.org/126118/diff/37/1036
File webkit/api/src/gtk/WebInputEventFactory.cpp (right):

http://codereview.chromium.org/126118/diff/37/1036#newcode178
Line 178: // that we can create a WebKeyboardEvent instance without a
GdkEventKey
On 2009/06/17 02:04:27, hbono wrote:
> On 2009/06/16 19:17:30, Evan Stade wrote:
> > what I meant to ask was that you create a second function
> > WebInputFactory::keyboardEvent(wchar_t character) {
> > }
> 
> Even though we should use WebKit coding style for this file, I'm a little
> wondering if it is OK to create overloaded functions in this file. (An you
know,
> the coding style of Google allows overloaded functions only "in cases where
> input can be specified in different types that contain the same information":
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...)
> 
> Even though I personally prefer this option, I would like to hear opinions of
> WebKit experts.

you could always call it something else (keyboardCharEvent() springs to mind)

[darin (slow to review), 2009-06-17 04:40:53]

http://codereview.chromium.org/126118/diff/37/1036
File webkit/api/src/gtk/WebInputEventFactory.cpp (right):

http://codereview.chromium.org/126118/diff/37/1036#newcode76
Line 76: static const unsigned int kHardwareCodeToGdkKeyval[] = {
nit: webkit coding style does not put a "k" in the front of the name of a
constant.  i think the convention for webkit is to just use variable naming
conventions, and "Gdk" should be "GDK" since it is an acronym [*].  so
"hardwareCodeToGDKKeyval" would probably be better.

[*] when an acronym is at the start of a function, it should be lowercase. 
e.g., gdkStateToWebEventModifiers.

[Hironori Bono, 2009-06-17 09:55:08]

Thank you for your comments.
I have added a WebInputEventFactory::keyboardEvent(wchar_t, double) for a Char
event, changed this change to use the function, and removed 'k' from constants
in "WebInputEventFactory.cpp".
Since this update needed to change some more files, is it possible to review the
latest one again?

http://codereview.chromium.org/126118/diff/37/1036
File webkit/api/src/gtk/WebInputEventFactory.cpp (right):

http://codereview.chromium.org/126118/diff/37/1036#newcode76
Line 76: static const unsigned int kHardwareCodeToGdkKeyval[] = {
On 2009/06/17 04:40:53, darin wrote:
> nit: webkit coding style does not put a "k" in the front of the name of a
> constant.  i think the convention for webkit is to just use variable naming
> conventions, and "Gdk" should be "GDK" since it is an acronym [*].  so
> "hardwareCodeToGDKKeyval" would probably be better.
> 
> [*] when an acronym is at the start of a function, it should be lowercase. 
> e.g., gdkStateToWebEventModifiers.

Done. Thank you so much for this detailed description.

http://codereview.chromium.org/126118/diff/37/1036#newcode178
Line 178: // that we can create a WebKeyboardEvent instance without a
GdkEventKey
On 2009/06/17 02:09:24, Evan Stade wrote:
> you could always call it something else (keyboardCharEvent() springs to mind)

Thank you for your suggestion.
I noticed the WebInputEventFactory class had already had overloaded functions,
WebInputEventFactory::mouseEvent(). So, I think it is better to add an
overloaded function also for keyboardEvent().

[Evan Martin, 2009-06-18 00:41:10]

(should I be reviewing this? or darin? or who?)

[Hironori Bono, 2009-06-18 04:11:54]

On 2009/06/18 00:41:10, Evan Martin wrote:
> (should I be reviewing this? or darin? or who?)

Sorry for the lack of my explanation.

I'm wondering who can review my changes for
"chrome/common/native_web_keyboard_event.h" and
"chrome/common/native_web_keyboard_event_linux.cc".

To read their change logs, they are mostly changed by darin. So, I would like to
hear his opinion whether or not I can add a platform-specific function there.

[darin (slow to review), 2009-06-18 06:34:05]

http://codereview.chromium.org/126118/diff/58/1080
File chrome/common/native_web_keyboard_event.h (right):

http://codereview.chromium.org/126118/diff/58/1080#newcode34
Line 34: void SetWebKeyboardEvent(const WebKit::WebKeyboardEvent& event);
why not just have another constructor here?

I think this class was trying to match the various flavors of
WebInputEventFactory methods for creating a WebKeyboardEvent.

http://codereview.chromium.org/126118/diff/58/1073
File webkit/api/public/gtk/WebInputEventFactory.h (right):

http://codereview.chromium.org/126118/diff/58/1073#newcode50
Line 50: WEBKIT_API static WebKeyboardEvent keyboardEvent(wchar_t, double);
nit: these parameters should be named since their types do not give much
indication as to what they mean.  that is the rule in webkit style: leave out
the parameter name if the type name is sufficient to document the parameter.

[Hironori Bono, 2009-06-18 09:05:03]

Thank you for your comments. They are definitely helpful for me.

http://codereview.chromium.org/126118/diff/58/1080
File chrome/common/native_web_keyboard_event.h (right):

http://codereview.chromium.org/126118/diff/58/1080#newcode34
Line 34: void SetWebKeyboardEvent(const WebKit::WebKeyboardEvent& event);
On 2009/06/18 06:34:05, darin wrote:
> why not just have another constructor here?
>
> I think this class was trying to match the various flavors of
> WebInputEventFactory methods for creating a WebKeyboardEvent.

Done. Thank you for your comment. This is exactly what I would like to clarify
because Google C++ coding style writes "Use overloaded functions (including
constructors) only in cases where input can be specified in different types that
contain the same information."
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...>
Even though I prefer adding another constructor, I was afraid it violated the
coding style. So, I needed your opinion.
I have updated this change to add a constructor to NativeWebKeyboardEvent class
and use it.

http://codereview.chromium.org/126118/diff/58/1073
File webkit/api/public/gtk/WebInputEventFactory.h (right):

http://codereview.chromium.org/126118/diff/58/1073#newcode50
Line 50: WEBKIT_API static WebKeyboardEvent keyboardEvent(wchar_t, double);
On 2009/06/18 06:34:05, darin wrote:
> nit: these parameters should be named since their types do not give much
> indication as to what they mean.  that is the rule in webkit style: leave out
> the parameter name if the type name is sufficient to document the parameter.

Done. Thank you for noticing this.

[darin (slow to review), 2009-06-22 16:06:51]

LGTM

My review of render_widget_host_view_gtk.{h,cc} was fairly superficial, so you
probably want to get someone else who knows that file better to comment that
those changes look good too.

http://codereview.chromium.org/126118/diff/3003/2007
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/126118/diff/3003/2007#newcode305
Line 305: void ImeSetComposition(const std::wstring& ime_string,
nit: we should use string16 instead of wstring in new code.  if it is not
convenient to change to string16 now, then it can be postponed to a follow-up
CL.

http://codereview.chromium.org/126118/diff/3003/2004
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/126118/diff/3003/2004#newcode330
Line 330: im_context_ = NULL;
nit: no need to null im_context_ since we're being destroyed anyways.

http://codereview.chromium.org/126118/diff/3003/2004#newcode505
Line 505: GdkRectangle cursor_rect;
nit: use caret_rect.ToGdkRectangle() instead

http://codereview.chromium.org/126118/diff/3003/2009
File chrome/common/native_web_keyboard_event.h (right):

http://codereview.chromium.org/126118/diff/3003/2009#newcode34
Line 34: NativeWebKeyboardEvent(wchar_t character, double timeStampSeconds);
nit: timeStampSeconds -> time_stamp_seconds

[Evan Martin, 2009-06-22 21:56:30]

_gtk.* LGTM as well

[Hironori Bono, 2009-06-23 15:00:51]

Thank you for your comments.
Even though I have committed (and reverted) this change, I would like to reply a
comment of yours.

On 2009/06/22 16:06:51, darin wrote:
> nit: we should use string16 instead of wstring in new code.  if it is not
convenient to change to string16 now, then it can be postponed to a follow-up
CL.

Thank you for noticing this.
I'm refactoring our IME back-end (and front-ends) to use string16 instead of
std::wstring. I'm going to send a review request as soon as possible.