Improve key event handling of AutocompleteEditViewGtk....

Improve key event handling of AutocompleteEditViewGtk.

This CL improves key event handling of AutocompleteEditViewGtk class, following changes have been made:
1. Avoid accessing private data member of GtkTextView object, including im_context, need_im_reset etc.
2. Always send key events to the default handler of GtkTextView before handling them by ourselves, to make sure the behavior of IME and GtkTextView are always correct. This fixes the issue of moving focus from omnibox to webpage by pressing Tab key when using XIM im module.
3. Intercept "insert-text" signal of GtkTextBuffer object to prevent any unwanted characters from being inserted into omnibox.
4. Intercept "backspace" signal of GtkTextView object to fix issue 19068: [Linux] Search keywords: Backspace should exit out of search mode

BUG=18393
: AutocompleteEditViewGtk should not access private members of GtkTextView
BUG=19068
: [Linux] Search keywords: Backspace should exit out of search mode
TEST=Starts a XIM input method, such as scim then open chrome with GTK_IM_MODULE=xim. Opens a webpage and press tab and shift-tab in omnibox to see if focus can be moved to/from webpage correctly.
TEST=Copy & paste some text with new line characters to omnibox to see if new line characters are filtered.
TEST=Turn on keyword search mode, press backspace to see if it can exit keyword search mode.
TEST= Turn on keyword search mode, input some text then press backspace to delete a character, to see if it still in keyword search mode. Then move the cursor to the beginning of the text and press backspace, to see if it exits keyword search mode correctly.


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

[James Su, 2009-08-07 10:17:31]

Hi,
  This CL fixes issue 18393. Please help review.

Regards
James Su

[Dean McNamee, 2009-08-07 13:52:50]

I am a bit worried about this change, since I believe we used to do something
more like this and had some trouble.  Also, should we merge the code for the
buffer-changed signal and insert-text signal?

I will spend some more time on this today, thanks for all the good work.

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

http://codereview.chromium.org/164142/diff/1/2#newcode775
Line 775: for (const gchar* p = text; *p && len > 0; ++p, --len) {
shouldn't we allow inserting null characters?  Is there a reason to check *p,
when we already have the length?

http://codereview.chromium.org/164142/diff/1/2#newcode786
Line 786: klass->insert_text(buffer, location, filtered_text.c_str(),
It's more clear to use .data() when you're also passing a length.

http://codereview.chromium.org/164142/diff/1/2#newcode794
Line 794: }
can't this just be static guint singal_id = g_signal_lookup ... ?

[James Su, 2009-08-07 14:59:43]

Thanks for your feedback. CL has been updated accordingly.
I just removed the redundant code in
AutocompleteEditViewGtk::HandleEndUserAction().

And what's your opinion about filtering control characters? Is it necessary to
filter all chars between U+0001-U+001F and U+007F-U+009F?

Regards
James Su

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

http://codereview.chromium.org/164142/diff/1/2#newcode775
Line 775: for (const gchar* p = text; *p && len > 0; ++p, --len) {
On 2009/08/07 13:52:50, Dean McNamee wrote:
> shouldn't we allow inserting null characters?  Is there a reason to check *p,
> when we already have the length?

GtkTextBuffer does not accept null character. However I just checked gtk's
source code and found that the correctness of text and len will always be
guaranteed. So the check can be removed.

http://codereview.chromium.org/164142/diff/1/2#newcode786
Line 786: klass->insert_text(buffer, location, filtered_text.c_str(),
On 2009/08/07 13:52:50, Dean McNamee wrote:
> It's more clear to use .data() when you're also passing a length.

Ok. Actually they are same, at least on Linux.

http://codereview.chromium.org/164142/diff/1/2#newcode794
Line 794: }
On 2009/08/07 13:52:50, Dean McNamee wrote:
> can't this just be static guint singal_id = g_signal_lookup ... ?

Done.

[Dean McNamee, 2009-08-07 15:07:33]

Will review in full today.

Thanks

http://codereview.chromium.org/164142/diff/1004/12
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/164142/diff/1004/12#newcode752
Line 752: for (; len > 0; ++text, --len) {
Can we do this for loop the normal way?

for (int i = 0; i < len; ++i) {
  gchar c = text[i];
}

This also avoid modifying the input parameters, which is nice in case we need
|text| or |len| later on in the code.

http://codereview.chromium.org/164142/diff/1004/12#newcode763
Line 763: klass->insert_text(buffer, location, filtered_text.data(),
Yeah, I realize c_str and data are the same in all common implementations, but
it's more about communicating intent, and insert_text doesn't actually expect
the string to be null terminate.

[James Su, 2009-08-07 15:20:40]

Thanks for review. CL updated.

Regards
James Su

http://codereview.chromium.org/164142/diff/1004/12
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/164142/diff/1004/12#newcode752
Line 752: for (; len > 0; ++text, --len) {
On 2009/08/07 15:07:34, Dean McNamee wrote:
> Can we do this for loop the normal way?
> 
> for (int i = 0; i < len; ++i) {
>   gchar c = text[i];
> }
> 
> This also avoid modifying the input parameters, which is nice in case we need
> |text| or |len| later on in the code.

Done.

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

ping.

[Dean McNamee, 2009-08-10 23:50:28]

I don't understand what's better about handling this in insert-text vs
buffer-changed like it was before.  Also, how does the efficiency compare
between the old and new code?

[James Su, 2009-08-11 01:21:40]

On 2009/08/10 23:50:28, Dean McNamee wrote:
> I don't understand what's better about handling this in insert-text vs
> buffer-changed like it was before.  Also, how does the efficiency compare
> between the old and new code?

With the old code, the string is already inserted into GtkTextBuffer before
emitting buffer-changed, and then new lines are removed in signal handler by
using GtkTextIter.
While in the new code, the text got filtered before inserting into
GtkTextBuffer, which is simpler and more effective than old code, because no
GtkTextIter is involved.
The old code also doesn't handle Tab characters and new line at the beginning of
text buffer, while the new code does.

[Dean McNamee, 2009-08-11 16:53:23]

insert-text is still emitted when you paste right?

I kinda liked the old code using iterators, because then I just asked if there
was a new paragraph, and I didn't really need to operate on the utf-8 level (are
there line breaks, etc, in the unicode space we should also filter?).

Overall I think it LGs though.  Maybe Dan or Evan might also have comments.

http://codereview.chromium.org/164142/diff/16/1007
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/164142/diff/16/1007#newcode10
Line 10: #include <algorithm>
what is algorithm for?

http://codereview.chromium.org/164142/diff/16/1007#newcode161
Line 161: gtk_text_view_set_accepts_tab(GTK_TEXT_VIEW(text_view_), TRUE);
You should note that we do this because we want to be able to handle tab for tab
to search?

http://codereview.chromium.org/164142/diff/16/1007#newcode180
Line 180: // Intercepts "insert-text" signal to filter out any unwanted
characters.
I'd drop this comment.

http://codereview.chromium.org/164142/diff/16/1007#newcode751
Line 751: // these characters in omnibox, firefox and IE do allow it.
Remove the TODO, I would just filter what we want to filter (new lines and
tabs).

http://codereview.chromium.org/164142/diff/16/1007#newcode752
Line 752: for (gint i = 0; i < len; ++i) {
Should maybe note that yes, this could be UTF-8, but the way we work on it is
safe.  But in the future people should realize we're going byte by byte and not
character by character.

[Evan Stade, 2009-08-11 18:18:38]

http://codereview.chromium.org/164142/diff/16/1007
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/164142/diff/16/1007#newcode767
Line 767: // Cache signal id for better performance.
this comment is extraneous

http://codereview.chromium.org/164142/diff/16/1007#newcode772
Line 772: g_signal_stop_emission(buffer, signal_id, 0);
is this really significantly better than g_signal_stop_emission_by_name? If so,
can you change the other use(s) of g_signal_stop_emission_by_name in this file
to use this technique?

[Daniel Erat, 2009-08-11 23:20:07]

LGTM

http://codereview.chromium.org/164142/diff/16/1007
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/164142/diff/16/1007#newcode754
Line 754: if (c == '\n' || c == '\r' || c =='\t')
space between == and '\t'

[James Su, 2009-08-12 08:29:39]

On 2009/08/11 16:53:23, Dean McNamee wrote:
> insert-text is still emitted when you paste right?
Yes. "insert-text" signal is the only entry for inserting content into
GtkTextBuffer.

> 
> I kinda liked the old code using iterators, because then I just asked if there
> was a new paragraph, and I didn't really need to operate on the utf-8 level
(are
> there line breaks, etc, in the unicode space we should also filter?).
AFAIK, there is no other line break defined in Unicode space. The old code can't
handle Tab characters, and the performance is not as good as the new code.

> 
> Overall I think it LGs though.  Maybe Dan or Evan might also have comments.
> 
> http://codereview.chromium.org/164142/diff/16/1007
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> 
> http://codereview.chromium.org/164142/diff/16/1007#newcode10
> Line 10: #include <algorithm>
> what is algorithm for?
> 
> http://codereview.chromium.org/164142/diff/16/1007#newcode161
> Line 161: gtk_text_view_set_accepts_tab(GTK_TEXT_VIEW(text_view_), TRUE);
> You should note that we do this because we want to be able to handle tab for
tab
> to search?
> 
> http://codereview.chromium.org/164142/diff/16/1007#newcode180
> Line 180: // Intercepts "insert-text" signal to filter out any unwanted
> characters.
> I'd drop this comment.
> 
> http://codereview.chromium.org/164142/diff/16/1007#newcode751
> Line 751: // these characters in omnibox, firefox and IE do allow it.
> Remove the TODO, I would just filter what we want to filter (new lines and
> tabs).
> 
> http://codereview.chromium.org/164142/diff/16/1007#newcode752
> Line 752: for (gint i = 0; i < len; ++i) {
> Should maybe note that yes, this could be UTF-8, but the way we work on it is
> safe.  But in the future people should realize we're going byte by byte and
not
> character by character.

[James Su, 2009-08-12 08:33:49]

Hi,
  I updated the CL according to your comments, and also made some improvement to
fix issue 19068. Please kindly review again.

Regards
James Su

http://codereview.chromium.org/164142/diff/16/1007
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/164142/diff/16/1007#newcode10
Line 10: #include <algorithm>
On 2009/08/11 16:53:23, Dean McNamee wrote:
> what is algorithm for?

For std::min(). gcl lint would complain without it.

http://codereview.chromium.org/164142/diff/16/1007#newcode161
Line 161: gtk_text_view_set_accepts_tab(GTK_TEXT_VIEW(text_view_), TRUE);
On 2009/08/11 16:53:23, Dean McNamee wrote:
> You should note that we do this because we want to be able to handle tab for
tab
> to search?

Done.

http://codereview.chromium.org/164142/diff/16/1007#newcode751
Line 751: // these characters in omnibox, firefox and IE do allow it.
On 2009/08/11 16:53:23, Dean McNamee wrote:
> Remove the TODO, I would just filter what we want to filter (new lines and
> tabs).

Done.

http://codereview.chromium.org/164142/diff/16/1007#newcode752
Line 752: for (gint i = 0; i < len; ++i) {
On 2009/08/11 16:53:23, Dean McNamee wrote:
> Should maybe note that yes, this could be UTF-8, but the way we work on it is
> safe.  But in the future people should realize we're going byte by byte and
not
> character by character.

Done.

http://codereview.chromium.org/164142/diff/16/1007#newcode754
Line 754: if (c == '\n' || c == '\r' || c =='\t')
On 2009/08/11 23:20:07, Daniel Erat wrote:
> space between == and '\t'

Done.

http://codereview.chromium.org/164142/diff/16/1007#newcode772
Line 772: g_signal_stop_emission(buffer, signal_id, 0);
On 2009/08/11 18:18:38, Evan Stade wrote:
> is this really significantly better than g_signal_stop_emission_by_name? If
so,
> can you change the other use(s) of g_signal_stop_emission_by_name in this file
> to use this technique?

It would be better, but don't know how significant it is.
g_signal_stop_emission_by_name() needs parse the signal name and lookup the
signal_id in an array with bsearch each time. The signal array is global, so it
would be very large.

Done.

[Dean McNamee, 2009-08-12 15:17:55]

Old code LGTM, but one question about the backspace code you've just added. 
Also, make sure to update the CL description, BUG= lines, etc.

http://codereview.chromium.org/164142/diff/1020/23
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/164142/diff/1020/23#newcode794
Line 794: return;  // Propgate into GtkTextView.
I am missing why we look at the selection.  Don't we just want to check if the
text buffer is empty?

[James Su, 2009-08-12 15:40:10]

Description and BUG lines have been updated.

Regards
James Su

http://codereview.chromium.org/164142/diff/1020/23
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/164142/diff/1020/23#newcode794
Line 794: return;  // Propgate into GtkTextView.
On 2009/08/12 15:17:55, Dean McNamee wrote:
> I am missing why we look at the selection.  Don't we just want to check if the
> text buffer is empty?

This piece of code just follows the logic of windows code.

[Dean McNamee, 2009-08-12 15:44:21]

LG

[tony, 2009-08-12 20:56:18]

Accelerators no longer work when the omnibox has focus.  Reverting this change
causes them to work again.  James can you look into it?