Cleanup GtkIMContext related code by splitting it into separated source file.

Cleanup GtkIMContext related code by splitting it into separated source file.

This CL splits GtkIMContext related code into separated source files from render_widget_host_view_gtk.{h,cc}, making the code clearer to read. It also fixes issue 18061.
Focus moving issues related to AutocompleteEditViewGtk will be addressed in another CL, see issue 18393 for reference.

BUG=18061
: pressing tab on form textfields goes directly to submit or login buttons
TEST=Starts scim's xim server with command "scim -d", then launches chrome with GTK_IM_MODULE=xim. Opens a web page with multiple input box, such as gmail.com, presses Tab and Shift-Tab multiple times to see if focus moves among focusable elements in the web page.


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

[James Su, 2009-08-04 08:02:41]

Hi,
  Please help review this CL, which makes GtkIMContext related code clearer and
fixes issue 18061.

Regards
James Su

[Hironori Bono, 2009-08-04 09:43:17]

(+deanm)

These are just drive-by nits since my Linux box is not available now. I'm going
to try this change when it is available.

Regards,

http://codereview.chromium.org/160577/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/160577/diff/1/2#newcode543
Line 543: // TODO(suzhe): we shouldn't access GtkTextView's im_context and
nit: it may be better to file a bug when adding a TODO.

http://codereview.chromium.org/160577/diff/1/2#newcode564
Line 564: // We can't rely on GtkTextView to handle focus move for us, which
nit: can't -> cannot

http://codereview.chromium.org/160577/diff/1/4
File chrome/browser/renderer_host/gtk_im_context_wrapper.cc (right):

http://codereview.chromium.org/160577/diff/1/4#newcode8
Line 8: #include <gdk/gdk.h>
nit: it is better to sort these include files in alphabetical order.

http://codereview.chromium.org/160577/diff/1/4#newcode19
Line 19: : host_view_(host_view),
nit: need four spaces before ':'.

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

http://codereview.chromium.org/160577/diff/1/6#newcode21
Line 21: // This class is a convenience wrapper for GtkIMContext.
nit: it is better to add class comments:
<http://www.corp.google.com/eng/doc/cppguide.xml#Class_Comments>.

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

http://codereview.chromium.org/160577/diff/1/3#newcode12
Line 12: #include <algorithm>
nit: need a blank line before C++ include lines.

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

http://codereview.chromium.org/160577/diff/1/5#newcode9
Line 9: #include <map>
nit: need a blank line before C++ include files.

[James Su, 2009-08-04 10:09:24]

Thanks for your comments. CL has been updated accordingly.

http://codereview.chromium.org/160577/diff/1/2
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/160577/diff/1/2#newcode543
Line 543: // TODO(suzhe): we shouldn't access GtkTextView's im_context and
On 2009/08/04 09:43:17, hbono wrote:
> nit: it may be better to file a bug when adding a TODO.

issue 18393 has been filed.

http://codereview.chromium.org/160577/diff/1/2#newcode564
Line 564: // We can't rely on GtkTextView to handle focus move for us, which
On 2009/08/04 09:43:17, hbono wrote:
> nit: can't -> cannot

Done.

http://codereview.chromium.org/160577/diff/1/4
File chrome/browser/renderer_host/gtk_im_context_wrapper.cc (right):

http://codereview.chromium.org/160577/diff/1/4#newcode8
Line 8: #include <gdk/gdk.h>
On 2009/08/04 09:43:17, hbono wrote:
> nit: it is better to sort these include files in alphabetical order.

Done.

http://codereview.chromium.org/160577/diff/1/4#newcode19
Line 19: : host_view_(host_view),
On 2009/08/04 09:43:17, hbono wrote:
> nit: need four spaces before ':'.

Done.

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

http://codereview.chromium.org/160577/diff/1/6#newcode21
Line 21: // This class is a convenience wrapper for GtkIMContext.
On 2009/08/04 09:43:17, hbono wrote:
> nit: it is better to add class comments:
> <http://www.corp.google.com/eng/doc/cppguide.xml#Class_Comments>.

Added some comments, not sure if it's enough.

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

http://codereview.chromium.org/160577/diff/1/5#newcode9
Line 9: #include <map>
On 2009/08/04 09:43:17, hbono wrote:
> nit: need a blank line before C++ include files.

Done.

[James Su, 2009-08-05 09:03:54]

ping.

[Hironori Bono, 2009-08-05 09:55:00]

Sorry for my slow response.
I have been setting up another Linux PC today because my Linux PC is still not
available. I'm going to try this change tomorrow.

By the way, it seems Rietveld cannot show the svn:eol-style property set to
'gtk_im_context_wrapper.{h,cc}' because of Chinese characters included in your
change.
To prevent potential problems caused by non-ASCII characters, would it be
possible to set the locale to C when you upload your changes? (I'm not sure it
solves this problem, though.)

>   属性改变: chrome/browser/renderer_host/gtk_im_context_wrapper.cc
> ___________________________________________________________________
> 名字: svn:eol-style
>    + LF

Regards,

Hironori Bono

http://codereview.chromium.org/160577/diff/1009/1010
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/160577/diff/1009/1010#newcode545
Line 545: // eventually be hidden in future gtk release.
By the way, to read "gtktextview.c", its gtk_text_view_key_press_event() calls
gtk_im_context_filter_keypress() before calling this function. So, I'm wondering
if we need to call gtk_im_context_filter_keypress() here. (I'm going to try
removing this gtk_im_context_filter_keypress() call and see it works with IMEs.)

[James Su, 2009-08-05 10:01:16]

Hi,
  Patch uploaded again with English locale.

Regards
James Su

On 2009/08/05 09:55:00, hbono wrote:
> Sorry for my slow response.
> I have been setting up another Linux PC today because my Linux PC is still not
> available. I'm going to try this change tomorrow.
> 
> By the way, it seems Rietveld cannot show the svn:eol-style property set to
> 'gtk_im_context_wrapper.{h,cc}' because of Chinese characters included in your
> change.
> To prevent potential problems caused by non-ASCII characters, would it be
> possible to set the locale to C when you upload your changes? (I'm not sure it
> solves this problem, though.)
> 
> >   属性改变: chrome/browser/renderer_host/gtk_im_context_wrapper.cc
> > ___________________________________________________________________
> > 名字: svn:eol-style
> >    + LF
> 
> Regards,
> 
> Hironori Bono
> 
> http://codereview.chromium.org/160577/diff/1009/1010
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> 
> http://codereview.chromium.org/160577/diff/1009/1010#newcode545
> Line 545: // eventually be hidden in future gtk release.
> By the way, to read "gtktextview.c", its gtk_text_view_key_press_event() calls
> gtk_im_context_filter_keypress() before calling this function. So, I'm
wondering
> if we need to call gtk_im_context_filter_keypress() here. (I'm going to try
> removing this gtk_im_context_filter_keypress() call and see it works with
IMEs.)

[Hironori Bono, 2009-08-06 09:13:26]

deanm,

As you know, "gtktextview.c" calls gtk_im_context_filter_keypress() in its
gtk_text_view_key_press_event(). So, IMEs work good without calling
gtk_im_context_filter_keypress() in AutoCompleteEditViewGtk::HandleKeyPress().
On the other hand, when we call gtk_im_context_filter_keypress() in
AutocompleteEditViewGtk::HandleKeyPress(), some IMEs receive two identical key
events and get confused. So, I think it is better to remove the
gtk_im_context_filter_keypress() call in this
AutocompleteEditViewGtk::HandleKeyPress(). If we remove the
gtk_im_context_filter_keypress() call, we don't have to call
g_signal_emit_by_name() and discard the signals as did in this change.
Would it be possible to give me your opinions?

Regards,

Hironori Bono

[James Su, 2009-08-06 10:01:37]

Hi,
  As I understand, the purpose of im context related code in
AutocompleteEditViewGtk::HandleKeyPress() is to prevent GtkTextView from
handling Enter key, while still allow IME to handle it. Without this piece of
code, pressing Enter will input '\n' into the text view. But if we simply
intercept Enter key and don't call gtk_im_context_filter_keypress(), then IME
may not receive it and may cause problem if IME needs it. For example, an IME
may commit preedit string with Enter key.
  But existing code can't prevent an IME from committing '\n' into the text
view. A better solution would be using a customized GtkTextBuffer object which
override the default handler "insert_text" signal and filter out all unaccepted
characters. If you think it's reasonable and feasible, I'd like to prepare
another CL to implement it.

Regards
James Su

On 2009/08/06 09:13:26, hbono wrote:
> deanm,
> 
> As you know, "gtktextview.c" calls gtk_im_context_filter_keypress() in its
> gtk_text_view_key_press_event(). So, IMEs work good without calling
> gtk_im_context_filter_keypress() in AutoCompleteEditViewGtk::HandleKeyPress().
> On the other hand, when we call gtk_im_context_filter_keypress() in
> AutocompleteEditViewGtk::HandleKeyPress(), some IMEs receive two identical key
> events and get confused. So, I think it is better to remove the
> gtk_im_context_filter_keypress() call in this
> AutocompleteEditViewGtk::HandleKeyPress(). If we remove the
> gtk_im_context_filter_keypress() call, we don't have to call
> g_signal_emit_by_name() and discard the signals as did in this change.
> Would it be possible to give me your opinions?
> 
> Regards,
> 
> Hironori Bono

[Hironori Bono, 2009-08-06 10:23:01]

Hi James,

Let me clarify your message.
Does this "preparing another change" mean "preparing another change which
depends on this change", or "spinning off the AutocompleteEditViewGtk part of
this change into another change"?

If you are creating another change for the AutocompleteEditViewGtk class, it may
be better to revert the AutocompleteEditViewGtk part of this change and make
this change focus on refactoring. (A change which fixes more changes needs more
reviewers. Retrieving approvals of more reviewers takes more time.)

Regards,

Hironori Bono

On 2009/08/06 10:01:37, James Su wrote:
> Hi,
>   As I understand, the purpose of im context related code in
> AutocompleteEditViewGtk::HandleKeyPress() is to prevent GtkTextView from
> handling Enter key, while still allow IME to handle it. Without this piece of
> code, pressing Enter will input '\n' into the text view. But if we simply
> intercept Enter key and don't call gtk_im_context_filter_keypress(), then IME
> may not receive it and may cause problem if IME needs it. For example, an IME
> may commit preedit string with Enter key.
>   But existing code can't prevent an IME from committing '\n' into the text
> view. A better solution would be using a customized GtkTextBuffer object which
> override the default handler "insert_text" signal and filter out all
unaccepted
> characters. If you think it's reasonable and feasible, I'd like to prepare
> another CL to implement it.
> 
> Regards
> James Su
> 
> On 2009/08/06 09:13:26, hbono wrote:
> > deanm,
> > 
> > As you know, "gtktextview.c" calls gtk_im_context_filter_keypress() in its
> > gtk_text_view_key_press_event(). So, IMEs work good without calling
> > gtk_im_context_filter_keypress() in
AutoCompleteEditViewGtk::HandleKeyPress().
> > On the other hand, when we call gtk_im_context_filter_keypress() in
> > AutocompleteEditViewGtk::HandleKeyPress(), some IMEs receive two identical
key
> > events and get confused. So, I think it is better to remove the
> > gtk_im_context_filter_keypress() call in this
> > AutocompleteEditViewGtk::HandleKeyPress(). If we remove the
> > gtk_im_context_filter_keypress() call, we don't have to call
> > g_signal_emit_by_name() and discard the signals as did in this change.
> > Would it be possible to give me your opinions?
> > 
> > Regards,
> > 
> > Hironori Bono

[James Su, 2009-08-06 10:25:55]

I think it's better to create a new change for AutocompleteEditViewGtk class and
revert relevant part in this change.
The new change is for issue 18393.

Is it ok for you?

Regards
James Su

On 2009/08/06 10:23:01, hbono wrote:
> Hi James,
> 
> Let me clarify your message.
> Does this "preparing another change" mean "preparing another change which
> depends on this change", or "spinning off the AutocompleteEditViewGtk part of
> this change into another change"?
> 
> If you are creating another change for the AutocompleteEditViewGtk class, it
may
> be better to revert the AutocompleteEditViewGtk part of this change and make
> this change focus on refactoring. (A change which fixes more changes needs
more
> reviewers. Retrieving approvals of more reviewers takes more time.)
> 
> Regards,
> 
> Hironori Bono
> 
> On 2009/08/06 10:01:37, James Su wrote:
> > Hi,
> >   As I understand, the purpose of im context related code in
> > AutocompleteEditViewGtk::HandleKeyPress() is to prevent GtkTextView from
> > handling Enter key, while still allow IME to handle it. Without this piece
of
> > code, pressing Enter will input '\n' into the text view. But if we simply
> > intercept Enter key and don't call gtk_im_context_filter_keypress(), then
IME
> > may not receive it and may cause problem if IME needs it. For example, an
IME
> > may commit preedit string with Enter key.
> >   But existing code can't prevent an IME from committing '\n' into the text
> > view. A better solution would be using a customized GtkTextBuffer object
which
> > override the default handler "insert_text" signal and filter out all
> unaccepted
> > characters. If you think it's reasonable and feasible, I'd like to prepare
> > another CL to implement it.
> > 
> > Regards
> > James Su
> > 
> > On 2009/08/06 09:13:26, hbono wrote:
> > > deanm,
> > > 
> > > As you know, "gtktextview.c" calls gtk_im_context_filter_keypress() in its
> > > gtk_text_view_key_press_event(). So, IMEs work good without calling
> > > gtk_im_context_filter_keypress() in
> AutoCompleteEditViewGtk::HandleKeyPress().
> > > On the other hand, when we call gtk_im_context_filter_keypress() in
> > > AutocompleteEditViewGtk::HandleKeyPress(), some IMEs receive two identical
> key
> > > events and get confused. So, I think it is better to remove the
> > > gtk_im_context_filter_keypress() call in this
> > > AutocompleteEditViewGtk::HandleKeyPress(). If we remove the
> > > gtk_im_context_filter_keypress() call, we don't have to call
> > > g_signal_emit_by_name() and discard the signals as did in this change.
> > > Would it be possible to give me your opinions?
> > > 
> > > Regards,
> > > 
> > > Hironori Bono

[James Su, 2009-08-06 10:43:14]

Changes to AutocompleteEditViewGtk has been reverted. Another CL will be created
for that part. See issue 18393 for reference.

Regards
James Su

[Hironori Bono, 2009-08-06 11:05:10]

Hi Jamese,

Is it possible to remove the "BUG=18061..." line and the "TEST=..." line of this
description? Without removing this line, our bugdroid sends a message that this
change fixes an Issue 18061.

By the way, I'm a little wondering why you have not sent the "Patch Set
2",...,"Patch Set 4" of this change to try bots. These try-bot results are
definitely helpful for reviewers to check whether or not your change works on
all platforms without any serious problems. (Even though changes since your
"Patch Set 1" are obvious for you, they may not be obvious for reviewers.) Are
there any problems while using try bots?

Regards,

Hironori Bono

[James Su, 2009-08-06 12:44:04]

Hi,
  This CL does fix issue 18061. This issue only mentions focus moving problem
inside a web page. Moving focus between omnibox and webpage isn't covered. I
mentioned this problem in issue 18393.
  And the CL has been sent to try-bot.

Regards
James Su

On 2009/08/06 11:05:10, hbono wrote:
> Hi Jamese,
> 
> Is it possible to remove the "BUG=18061..." line and the "TEST=..." line of
this
> description? Without removing this line, our bugdroid sends a message that
this
> change fixes an Issue 18061.
> 
> By the way, I'm a little wondering why you have not sent the "Patch Set
> 2",...,"Patch Set 4" of this change to try bots. These try-bot results are
> definitely helpful for reviewers to check whether or not your change works on
> all platforms without any serious problems. (Even though changes since your
> "Patch Set 1" are obvious for you, they may not be obvious for reviewers.) Are
> there any problems while using try bots?
> 
> Regards,
> 
> Hironori Bono

[Hironori Bono, 2009-08-07 03:10:52]

LGTM.
Thank you for your IME work and sorry for my misunderstanding.

Regards,

Hironori Bono

On 2009/08/06 12:44:04, James Su wrote:
> Hi,
>   This CL does fix issue 18061. This issue only mentions focus moving problem
> inside a web page. Moving focus between omnibox and webpage isn't covered. I
> mentioned this problem in issue 18393.
>   And the CL has been sent to try-bot.
> 
> Regards
> James Su
> 
> On 2009/08/06 11:05:10, hbono wrote:
> > Hi Jamese,
> > 
> > Is it possible to remove the "BUG=18061..." line and the "TEST=..." line of
> this
> > description? Without removing this line, our bugdroid sends a message that
> this
> > change fixes an Issue 18061.
> > 
> > By the way, I'm a little wondering why you have not sent the "Patch Set
> > 2",...,"Patch Set 4" of this change to try bots. These try-bot results are
> > definitely helpful for reviewers to check whether or not your change works
on
> > all platforms without any serious problems. (Even though changes since your
> > "Patch Set 1" are obvious for you, they may not be obvious for reviewers.)
Are
> > there any problems while using try bots?
> > 
> > Regards,
> > 
> > Hironori Bono

[James Su, 2009-08-10 22:19:43]

ping.

[Dean McNamee, 2009-08-10 23:02:34]

I didn't work on this code originally.  Hbono's review is probably
good enough, maybe ask avi or estade also?

On Mon, Aug 10, 2009 at 6:19 PM, <suzhe@chromium.org> wrote:
> ping.
>
> http://codereview.chromium.org/160577
>

[Evan Stade, 2009-08-10 23:16:48]

I stand behind hbono's LGTM

[James Su, 2009-08-11 01:36:05]

On 2009/08/10 23:16:48, Evan Stade wrote:
> I stand behind hbono's LGTM

Thanks for review. CL submitted.

Regards
James Su