Fix control key and paste behavior in Linux omnibox.

Fix control key and paste behavior in Linux omnibox.

BUG=12316
: Linux Omnibox, autocomplete on paste annoying.
BUG=13096
: Support desired_tld in Linux omnibox
BUG=20166
: Linux omnibox control key behavior is incorrect
TEST=Select all text in omnibox and paste something into omnibox by either ctrl-v, paste item in context menu or middle click, to see if inline autocomplete is not activated.
TEST=Input something in omnibox, eg. "goog", make sure the inline autocomplete is activated, then press ctrl key to see if the inline autocomplete is still there.
TEST=Input something in omnibox, eg. "cnn", press ctrl-Enter to see if www.cnn.com is opened.


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

[James Su, 2009-08-26 06:11:22]

CL to fix control key behavior in Linux omnibox.

Regards
James Su

[James Su, 2009-08-26 13:36:16]

Hi,
  I updated this CL to include the fix of issue 12316: Linux Omnibox,
autocomplete on paste annoying. Please help review.

Regards
James Su

On 2009/08/26 06:11:22, James Su wrote:
> CL to fix control key behavior in Linux omnibox.
> 
> Regards
> James Su

[Dean McNamee, 2009-08-26 15:52:16]

It's hard to review a single CL that does a bunch of unrelated things.  In the
future it would be super helpful to have separated CLs for separate things.

I can't tell what was the bug about control key behavior, verses what you've
changed for copy/paste.

http://codereview.chromium.org/173462/diff/1001/1002
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/173462/diff/1001/1002#newcode456
Line 456: model_->on_paste_replacing_all();
I don't get it, why would all the text be selected?  You modified IsSelectAll()
to include when there is on text?  Does that match Windows?

http://codereview.chromium.org/173462/diff/1001/1002#newcode761
Line 761: gdk_window_get_pointer(text_view_->window, NULL, NULL, &modifiers);
Am I just missing where this is used?

[James Su, 2009-08-27 01:19:40]

Hi,
  Sorry for mixing two fixes into one CL. I thought these were two trivial fixes
and they all relate to one file so merging them into one CL would be ok. And I'm
wondering if it's possible to create two CLs modifying one file at the same
time.

Regards
James Su

http://codereview.chromium.org/173462/diff/1001/1002
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/173462/diff/1001/1002#newcode456
Line 456: model_->on_paste_replacing_all();
On 2009/08/26 15:52:16, Dean McNamee wrote:
> I don't get it, why would all the text be selected?  You modified
IsSelectAll()
> to include when there is on text?  Does that match Windows?

It just follows the Windows behavior. AutocompleteEditViewWin::IsSelectAll()
returns true when there is no text. AutocompleteEditViewWin::OnPaste() calls
model_->on_paste_replacing_all() only when IsSelectAll() returns true.
And the comment of on_paste_replacing_all() method states this behavior clearly.

http://codereview.chromium.org/173462/diff/1001/1002#newcode761
Line 761: gdk_window_get_pointer(text_view_->window, NULL, NULL, &modifiers);
On 2009/08/26 15:52:16, Dean McNamee wrote:
> Am I just missing where this is used?

Oh, my fault. The following line should have been modified as well.

http://codereview.chromium.org/173462/diff/7/8#newcode400
Line 400: model_->StartAutocomplete(sel.cp_max < length && sel.cp_min < length);
This fixes the bug of control key behavior. Above comment states the purpose of
this piece of code clearly.

[Evan Stade, 2009-08-27 01:27:40]

On Wed, Aug 26, 2009 at 6:19 PM, <suzhe@chromium.org> wrote:
> Hi,
> =A0Sorry for mixing two fixes into one CL. I thought these were two
> trivial fixes and they all relate to one file so merging them into one
> CL would be ok. And I'm wondering if it's possible to create two CLs
> modifying one file at the same time.
>
> Regards
> James Su

that's what git is for. If you're using svn you're stuck checking out
the tree twice.

>
>
> http://codereview.chromium.org/173462/diff/1001/1002
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
>
> http://codereview.chromium.org/173462/diff/1001/1002#newcode456
> Line 456: model_->on_paste_replacing_all();
> On 2009/08/26 15:52:16, Dean McNamee wrote:
>>
>> I don't get it, why would all the text be selected? =A0You modified
>
> IsSelectAll()
>>
>> to include when there is on text? =A0Does that match Windows?
>
> It just follows the Windows behavior.
> AutocompleteEditViewWin::IsSelectAll() returns true when there is no
> text. AutocompleteEditViewWin::OnPaste() calls
> model_->on_paste_replacing_all() only when IsSelectAll() returns true.
> And the comment of on_paste_replacing_all() method states this behavior
> clearly.
>
> http://codereview.chromium.org/173462/diff/1001/1002#newcode761
> Line 761: gdk_window_get_pointer(text_view_->window, NULL, NULL,
> &modifiers);
> On 2009/08/26 15:52:16, Dean McNamee wrote:
>>
>> Am I just missing where this is used?
>
> Oh, my fault. The following line should have been modified as well.
>
> http://codereview.chromium.org/173462/diff/7/8#newcode400
> Line 400: model_->StartAutocomplete(sel.cp_max < length && sel.cp_min <
> length);
> This fixes the bug of control key behavior. Above comment states the
> purpose of this piece of code clearly.
>
> http://codereview.chromium.org/173462
>

[James Su, 2009-08-27 01:30:59]

Seems it's time to switch to git. Is gcl ready for working with git? 

Regards
James Su

On 2009/08/27 01:27:40, Evan Stade wrote:
> On Wed, Aug 26, 2009 at 6:19 PM, <mailto:suzhe@chromium.org> wrote:
> > Hi,
> > =A0Sorry for mixing two fixes into one CL. I thought these were two
> > trivial fixes and they all relate to one file so merging them into one
> > CL would be ok. And I'm wondering if it's possible to create two CLs
> > modifying one file at the same time.
> >
> > Regards
> > James Su
> 
> that's what git is for. If you're using svn you're stuck checking out
> the tree twice.
> 
> >
> >
> > http://codereview.chromium.org/173462/diff/1001/1002
> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> >
> > http://codereview.chromium.org/173462/diff/1001/1002#newcode456
> > Line 456: model_->on_paste_replacing_all();
> > On 2009/08/26 15:52:16, Dean McNamee wrote:
> >>
> >> I don't get it, why would all the text be selected? =A0You modified
> >
> > IsSelectAll()
> >>
> >> to include when there is on text? =A0Does that match Windows?
> >
> > It just follows the Windows behavior.
> > AutocompleteEditViewWin::IsSelectAll() returns true when there is no
> > text. AutocompleteEditViewWin::OnPaste() calls
> > model_->on_paste_replacing_all() only when IsSelectAll() returns true.
> > And the comment of on_paste_replacing_all() method states this behavior
> > clearly.
> >
> > http://codereview.chromium.org/173462/diff/1001/1002#newcode761
> > Line 761: gdk_window_get_pointer(text_view_->window, NULL, NULL,
> > &modifiers);
> > On 2009/08/26 15:52:16, Dean McNamee wrote:
> >>
> >> Am I just missing where this is used?
> >
> > Oh, my fault. The following line should have been modified as well.
> >
> > http://codereview.chromium.org/173462/diff/7/8#newcode400
> > Line 400: model_->StartAutocomplete(sel.cp_max < length && sel.cp_min <
> > length);
> > This fixes the bug of control key behavior. Above comment states the
> > purpose of this piece of code clearly.
> >
> > http://codereview.chromium.org/173462
> >

[Evan Stade, 2009-08-27 01:33:39]

http://code.google.com/p/chromium/wiki/UsingGit

-- Evan Stade



On Wed, Aug 26, 2009 at 6:30 PM, <suzhe@chromium.org> wrote:
> Seems it's time to switch to git. Is gcl ready for working with git?
>
> Regards
> James Su
>
> On 2009/08/27 01:27:40, Evan Stade wrote:
>>
>> On Wed, Aug 26, 2009 at 6:19 PM, <mailto:suzhe@chromium.org> wrote:
>> > Hi,
>> > =A0Sorry for mixing two fixes into one CL. I thought these were two
>> > trivial fixes and they all relate to one file so merging them into
>
> one
>>
>> > CL would be ok. And I'm wondering if it's possible to create two CLs
>> > modifying one file at the same time.
>> >
>> > Regards
>> > James Su
>
>> that's what git is for. If you're using svn you're stuck checking out
>> the tree twice.
>
>> >
>> >
>> > http://codereview.chromium.org/173462/diff/1001/1002
>> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc
>
> (right):
>>
>> >
>> > http://codereview.chromium.org/173462/diff/1001/1002#newcode456
>> > Line 456: model_->on_paste_replacing_all();
>> > On 2009/08/26 15:52:16, Dean McNamee wrote:
>> >>
>> >> I don't get it, why would all the text be selected? =A0You modified
>> >
>> > IsSelectAll()
>> >>
>> >> to include when there is on text? =A0Does that match Windows?
>> >
>> > It just follows the Windows behavior.
>> > AutocompleteEditViewWin::IsSelectAll() returns true when there is no
>> > text. AutocompleteEditViewWin::OnPaste() calls
>> > model_->on_paste_replacing_all() only when IsSelectAll() returns
>
> true.
>>
>> > And the comment of on_paste_replacing_all() method states this
>
> behavior
>>
>> > clearly.
>> >
>> > http://codereview.chromium.org/173462/diff/1001/1002#newcode761
>> > Line 761: gdk_window_get_pointer(text_view_->window, NULL, NULL,
>> > &modifiers);
>> > On 2009/08/26 15:52:16, Dean McNamee wrote:
>> >>
>> >> Am I just missing where this is used?
>> >
>> > Oh, my fault. The following line should have been modified as well.
>> >
>> > http://codereview.chromium.org/173462/diff/7/8#newcode400
>> > Line 400: model_->StartAutocomplete(sel.cp_max < length &&
>
> sel.cp_min <
>>
>> > length);
>> > This fixes the bug of control key behavior. Above comment states the
>> > purpose of this piece of code clearly.
>> >
>> > http://codereview.chromium.org/173462
>> >
>
>
>
> http://codereview.chromium.org/173462
>

[Dean McNamee, 2009-08-27 10:25:11]

Can you please add a comment to IsSelectAll in the header, stating that it
returns true when empty.

More booleans, more better ?

LG

http://codereview.chromium.org/173462/diff/7/8
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/173462/diff/7/8#newcode400
Line 400: model_->StartAutocomplete(sel.cp_max < length && sel.cp_min < length);
On 2009/08/27 01:19:40, James Su wrote:
> This fixes the bug of control key behavior. Above comment states the purpose
of
> this piece of code clearly.

Just to make sure I understand.  Would this also be fixed by sorting cp_max and
cp_min?  Or also

std::max(sel.cp_max, sel.cp_min) < GetTextLength ?

http://codereview.chromium.org/173462/diff/7/8#newcode762
Line 762: model_->OnSetFocus(modifiers & GDK_CONTROL_MASK);
Do you want to explicitly make this a boolean?

(...) != 0

[James Su, 2009-08-27 11:12:50]

CL updated.

An off-topic question: I just found that AutocompleteEditView (the interface)
does not have a virtual destructor, is it a problem?

Regards
James Su

http://codereview.chromium.org/173462/diff/7/8
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/173462/diff/7/8#newcode400
Line 400: model_->StartAutocomplete(sel.cp_max < length && sel.cp_min < length);
On 2009/08/27 10:25:11, Dean McNamee wrote:
> On 2009/08/27 01:19:40, James Su wrote:
> > This fixes the bug of control key behavior. Above comment states the purpose
> of
> > this piece of code clearly.
> 
> Just to make sure I understand.  Would this also be fixed by sorting cp_max
and
> cp_min?  Or also
> 
> std::max(sel.cp_max, sel.cp_min) < GetTextLength ?
> 
> 

This looks better, thanks.

http://codereview.chromium.org/173462/diff/7/8#newcode762
Line 762: model_->OnSetFocus(modifiers & GDK_CONTROL_MASK);
On 2009/08/27 10:25:11, Dean McNamee wrote:
> Do you want to explicitly make this a boolean?
> 
> (...) != 0

Done.

[Dean McNamee, 2009-08-27 22:22:21]

LG

I think we just use AutocompleteEditView as an interface (I don't
think we delete through it), so we should be safe without a virtual
destructor.  Would have to double check that though.

On Thu, Aug 27, 2009 at 4:12 AM,  <suzhe@chromium.org> wrote:
> CL updated.
>
> An off-topic question: I just found that AutocompleteEditView (the
> interface) does not have a virtual destructor, is it a problem?
>
> Regards
> James Su
>
>
> http://codereview.chromium.org/173462/diff/7/8
> File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
>
> http://codereview.chromium.org/173462/diff/7/8#newcode400
> Line 400: model_->StartAutocomplete(sel.cp_max < length && sel.cp_min <
> length);
> On 2009/08/27 10:25:11, Dean McNamee wrote:
>>
>> On 2009/08/27 01:19:40, James Su wrote:
>> > This fixes the bug of control key behavior. Above comment states the
>
> purpose
>>
>> of
>> > this piece of code clearly.
>
>> Just to make sure I understand. =A0Would this also be fixed by sorting
>
> cp_max and
>>
>> cp_min? =A0Or also
>
>> std::max(sel.cp_max, sel.cp_min) < GetTextLength ?
>
>
>
> This looks better, thanks.
>
> http://codereview.chromium.org/173462/diff/7/8#newcode762
> Line 762: model_->OnSetFocus(modifiers & GDK_CONTROL_MASK);
> On 2009/08/27 10:25:11, Dean McNamee wrote:
>>
>> Do you want to explicitly make this a boolean?
>
>> (...) !=3D 0
>
> Done.
>
> http://codereview.chromium.org/173462
>

[James Su, 2009-08-28 00:36:12]

I checked the source code and you are right, AutocompleteEditView{Gtk|Win|Mac}
are not deleted through AutocompleteEditView.

Thanks
James Su

On 2009/08/27 22:22:21, Dean McNamee wrote:
> LG
> 
> I think we just use AutocompleteEditView as an interface (I don't
> think we delete through it), so we should be safe without a virtual
> destructor.  Would have to double check that though.
> 
> On Thu, Aug 27, 2009 at 4:12 AM,  <mailto:suzhe@chromium.org> wrote:
> > CL updated.
> >
> > An off-topic question: I just found that AutocompleteEditView (the
> > interface) does not have a virtual destructor, is it a problem?
> >
> > Regards
> > James Su
> >
> >
> > http://codereview.chromium.org/173462/diff/7/8
> > File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):
> >
> > http://codereview.chromium.org/173462/diff/7/8#newcode400
> > Line 400: model_->StartAutocomplete(sel.cp_max < length && sel.cp_min <
> > length);
> > On 2009/08/27 10:25:11, Dean McNamee wrote:
> >>
> >> On 2009/08/27 01:19:40, James Su wrote:
> >> > This fixes the bug of control key behavior. Above comment states the
> >
> > purpose
> >>
> >> of
> >> > this piece of code clearly.
> >
> >> Just to make sure I understand. =A0Would this also be fixed by sorting
> >
> > cp_max and
> >>
> >> cp_min? =A0Or also
> >
> >> std::max(sel.cp_max, sel.cp_min) < GetTextLength ?
> >
> >
> >
> > This looks better, thanks.
> >
> > http://codereview.chromium.org/173462/diff/7/8#newcode762
> > Line 762: model_->OnSetFocus(modifiers & GDK_CONTROL_MASK);
> > On 2009/08/27 10:25:11, Dean McNamee wrote:
> >>
> >> Do you want to explicitly make this a boolean?
> >
> >> (...) !=3D 0
> >
> > Done.
> >
> > http://codereview.chromium.org/173462
> >