Fix issue 20934: Omnibox keyboard behavior wrong for "See recent pages in his...

Fix issue 20934: Omnibox keyboard behavior wrong for "See recent pages in history"

This CL fixes issue 20934 by eliminating the side effect caused by Enter key press event.

BUG=20934
: Omnibox keyboard behavior wrong for "See recent pages in history"
TEST=Input something in omnibox, make sure it triggers "See N recent pages in history containing XXX", then select this item and press Enter to see if chrome://history/#q=XXX is opened.


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

[James Su, 2009-09-04 02:46:24]

Please help review this CL. I'll cover this case in the automated test I'm
working on.

Regards
James Su

[Daniel Erat, 2009-09-04 22:15:08]

I fear that my understanding of this code isn't very good at this point. :-(

Can you make sure that you run through all of the test cases from your previous
omnibox changes to try to catch possible regressions?  (I've been doing this
with my omnibox changes and it seems to help; see e.g. the testing section of
http://codereview.chromium.org/176029/show)

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

http://codereview.chromium.org/196020/diff/1/2#oldcode644
Line 644: gtk_text_buffer_begin_user_action(text_buffer_);
Why is this no longer necessary?  Does this make it so that several
begin-user-action signals get emitted in the call to gtk_text_buffer_set_text()
now, instead of just one?

[James Su, 2009-09-05 01:25:36]

Hi,
  In the old code, if GtkTextView handles an Enter key press event, and modifies
its contents, then On{Before|After}PossibleChange() will be triggered, but then
in HandleKeyPress(), the change will be reverted and
On{Before|After}PossibleChange() will be trigger again by
gtk_text_buffer_{begin|end}_user_action(). The result is that the content is not
changed but On{Before|After}PossibleChange() is triggered twice, which causes
|model_| to re-calculate the autocomplete result with the same content, which is
not necessary and causes the issue.
  This CL gets rid off this side effect by eliminating the unnecessary calls to
OnAfterPossibleChange(), so that the change caused by an Enter key press event
will be reverted silently.

  I'm writing an automated test for autocomplete edit view, hope it can cover
most of these cases.

Regards
James Su

On 2009/09/04 22:15:08, Daniel Erat wrote:
> I fear that my understanding of this code isn't very good at this point. :-(
> 
> Can you make sure that you run through all of the test cases from your
previous
> omnibox changes to try to catch possible regressions?  (I've been doing this
> with my omnibox changes and it seems to help; see e.g. the testing section of
> http://codereview.chromium.org/176029/show)
> 
> http://codereview.chromium.org/196020/diff/1/2
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (left):
> 
> http://codereview.chromium.org/196020/diff/1/2#oldcode644
> Line 644: gtk_text_buffer_begin_user_action(text_buffer_);
> Why is this no longer necessary?  Does this make it so that several
> begin-user-action signals get emitted in the call to
gtk_text_buffer_set_text()
> now, instead of just one?

[James Su, 2009-09-05 01:42:06]

Another appoach is to revert the unexpected changed in OnAfterPossibleChange()
rather than in HandleKeyPress(). It might make the code clearer. I'll try this
approach and update this CL asap.

Regards
James Su

On 2009/09/05 01:25:36, James Su wrote:
> Hi,
>   In the old code, if GtkTextView handles an Enter key press event, and
modifies
> its contents, then On{Before|After}PossibleChange() will be triggered, but
then
> in HandleKeyPress(), the change will be reverted and
> On{Before|After}PossibleChange() will be trigger again by
> gtk_text_buffer_{begin|end}_user_action(). The result is that the content is
not
> changed but On{Before|After}PossibleChange() is triggered twice, which causes
> |model_| to re-calculate the autocomplete result with the same content, which
is
> not necessary and causes the issue.
>   This CL gets rid off this side effect by eliminating the unnecessary calls
to
> OnAfterPossibleChange(), so that the change caused by an Enter key press event
> will be reverted silently.
> 
>   I'm writing an automated test for autocomplete edit view, hope it can cover
> most of these cases.
> 
> Regards
> James Su
> 
> On 2009/09/04 22:15:08, Daniel Erat wrote:
> > I fear that my understanding of this code isn't very good at this point. :-(
> > 
> > Can you make sure that you run through all of the test cases from your
> previous
> > omnibox changes to try to catch possible regressions?  (I've been doing this
> > with my omnibox changes and it seems to help; see e.g. the testing section
of
> > http://codereview.chromium.org/176029/show)
> > 
> > http://codereview.chromium.org/196020/diff/1/2
> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (left):
> > 
> > http://codereview.chromium.org/196020/diff/1/2#oldcode644
> > Line 644: gtk_text_buffer_begin_user_action(text_buffer_);
> > Why is this no longer necessary?  Does this make it so that several
> > begin-user-action signals get emitted in the call to
> gtk_text_buffer_set_text()
> > now, instead of just one?

[James Su, 2009-09-05 09:07:07]

Hi,
  CL updated to follow this new approach. Please help review.

Regards
James Su

On 2009/09/05 01:42:06, James Su wrote:
> Another appoach is to revert the unexpected changed in OnAfterPossibleChange()
> rather than in HandleKeyPress(). It might make the code clearer. I'll try this
> approach and update this CL asap.
> 
> Regards
> James Su
> 
> On 2009/09/05 01:25:36, James Su wrote:
> > Hi,
> >   In the old code, if GtkTextView handles an Enter key press event, and
> modifies
> > its contents, then On{Before|After}PossibleChange() will be triggered, but
> then
> > in HandleKeyPress(), the change will be reverted and
> > On{Before|After}PossibleChange() will be trigger again by
> > gtk_text_buffer_{begin|end}_user_action(). The result is that the content is
> not
> > changed but On{Before|After}PossibleChange() is triggered twice, which
causes
> > |model_| to re-calculate the autocomplete result with the same content,
which
> is
> > not necessary and causes the issue.
> >   This CL gets rid off this side effect by eliminating the unnecessary calls
> to
> > OnAfterPossibleChange(), so that the change caused by an Enter key press
event
> > will be reverted silently.
> > 
> >   I'm writing an automated test for autocomplete edit view, hope it can
cover
> > most of these cases.
> > 
> > Regards
> > James Su
> > 
> > On 2009/09/04 22:15:08, Daniel Erat wrote:
> > > I fear that my understanding of this code isn't very good at this point.
:-(
> > > 
> > > Can you make sure that you run through all of the test cases from your
> > previous
> > > omnibox changes to try to catch possible regressions?  (I've been doing
this
> > > with my omnibox changes and it seems to help; see e.g. the testing section
> of
> > > http://codereview.chromium.org/176029/show)
> > > 
> > > http://codereview.chromium.org/196020/diff/1/2
> > > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (left):
> > > 
> > > http://codereview.chromium.org/196020/diff/1/2#oldcode644
> > > Line 644: gtk_text_buffer_begin_user_action(text_buffer_);
> > > Why is this no longer necessary?  Does this make it so that several
> > > begin-user-action signals get emitted in the call to
> > gtk_text_buffer_set_text()
> > > now, instead of just one?

[Daniel Erat, 2009-09-05 15:31:53]

LGTM