Hitting Tab should always move cursor to end of omnibox text.

Hitting Tab should always move cursor to end of omnibox text.

BUG=66850
TEST=AutocompleteEditViewTest.TabMoveCursorToEnd and InstantTest.TabKey

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

[James Su, 2010-12-24 00:58:37]

This CL is not complete yet. Windows port and many tests yet to be fixed.
However you may try it out now, to see if the behavior is acceptable.

[James Su, 2011-01-04 17:50:14]

This CL is ready for review.

[sky, 2011-01-04 21:19:40]

I haven't looked at the code, but in trying this out it seems as though you're
rerunning the query when tab is pressed, which you shouldn't do.

  -Scott

[James Su, 2011-01-05 01:37:26]

CL has been updated, please try again.

[James Su, 2011-01-06 18:19:20]

This CL still has problem when pressing Tab key to finalize instant suggest in
keyword mode. I'll fix the issue asap.

[sky, 2011-01-06 18:38:02]

Let me know when it's updated, and I'll give it another try.

  -Scott

On Thu, Jan 6, 2011 at 10:19 AM,  <suzhe@chromium.org> wrote:
> This CL still has problem when pressing Tab key to finalize instant suggest
> in
> keyword mode. I'll fix the issue asap.
>
> http://codereview.chromium.org/5966006/
>

[James Su, 2011-01-15 05:57:20]

Sorry for taking so long to update this CL. Please try it again.

Regards
James Su

On 2011/01/06 18:38:02, sky wrote:
> Let me know when it's updated, and I'll give it another try.
> 
>   -Scott
> 
> On Thu, Jan 6, 2011 at 10:19 AM,  <mailto:suzhe@chromium.org> wrote:
> > This CL still has problem when pressing Tab key to finalize instant suggest
> > in
> > keyword mode. I'll fix the issue asap.
> >
> > http://codereview.chromium.org/5966006/
> >

[sky, 2011-01-18 23:06:23]

It seems like tab still triggers rerunning the query. It shouldn't do that.

  -Scott

On Fri, Jan 14, 2011 at 9:57 PM,  <suzhe@chromium.org> wrote:
> Sorry for taking so long to update this CL. Please try it again.
>
> Regards
> James Su
>
> On 2011/01/06 18:38:02, sky wrote:
>>
>> Let me know when it's updated, and I'll give it another try.
>
>>   -Scott
>
>> On Thu, Jan 6, 2011 at 10:19 AM,  <mailto:suzhe@chromium.org> wrote:
>> > This CL still has problem when pressing Tab key to finalize instant
>> > suggest
>> > in
>> > keyword mode. I'll fix the issue asap.
>> >
>> > http://codereview.chromium.org/5966006/
>> >
>
>
>
> http://codereview.chromium.org/5966006/
>

[James Su, 2011-01-18 23:22:04]

On 2011/01/18 23:06:23, sky wrote:
> It seems like tab still triggers rerunning the query. It shouldn't do that.
I'm not sure if I understand this point correctly. Do you mean we shouldn't
update popup when accepting the current instant suggest? But I think we need to
update popup in such case. 

> 
>   -Scott
> 
> On Fri, Jan 14, 2011 at 9:57 PM,  <mailto:suzhe@chromium.org> wrote:
> > Sorry for taking so long to update this CL. Please try it again.
> >
> > Regards
> > James Su
> >
> > On 2011/01/06 18:38:02, sky wrote:
> >>
> >> Let me know when it's updated, and I'll give it another try.
> >
> >> &nbsp; -Scott
> >
> >> On Thu, Jan 6, 2011 at 10:19 AM, &nbsp;<mailto:suzhe@chromium.org> wrote:
> >> > This CL still has problem when pressing Tab key to finalize instant
> >> > suggest
> >> > in
> >> > keyword mode. I'll fix the issue asap.
> >> >
> >> > http://codereview.chromium.org/5966006/
> >> >
> >
> >
> >
> > http://codereview.chromium.org/5966006/
> >

[sky, 2011-01-18 23:36:07]

On Tue, Jan 18, 2011 at 3:22 PM,  <suzhe@chromium.org> wrote:
> On 2011/01/18 23:06:23, sky wrote:
>>
>> It seems like tab still triggers rerunning the query. It shouldn't do
>> that.
>
> I'm not sure if I understand this point correctly. Do you mean we shouldn't
> update popup when accepting the current instant suggest?

That's right. It should be no different than once suggest text becomes
autocomplete text (the fade).

> But I think we need
> to
> update popup in such case.

Why? We should wait until they type something more.

  -Scott
>
>
>>   -Scott
>
>> On Fri, Jan 14, 2011 at 9:57 PM,  <mailto:suzhe@chromium.org> wrote:
>> > Sorry for taking so long to update this CL. Please try it again.
>> >
>> > Regards
>> > James Su
>> >
>> > On 2011/01/06 18:38:02, sky wrote:
>> >>
>> >> Let me know when it's updated, and I'll give it another try.
>> >
>> >> &nbsp; -Scott
>> >
>> >> On Thu, Jan 6, 2011 at 10:19 AM, &nbsp;<mailto:suzhe@chromium.org>
>> >> wrote:
>> >> > This CL still has problem when pressing Tab key to finalize instant
>> >> > suggest
>> >> > in
>> >> > keyword mode. I'll fix the issue asap.
>> >> >
>> >> > http://codereview.chromium.org/5966006/
>> >> >
>> >
>> >
>> >
>> > http://codereview.chromium.org/5966006/
>> >
>
>
>
> http://codereview.chromium.org/5966006/
>

[James Su, 2011-01-18 23:51:25]

On 2011/01/18 23:36:07, sky wrote:
> On Tue, Jan 18, 2011 at 3:22 PM,  <mailto:suzhe@chromium.org> wrote:
> > On 2011/01/18 23:06:23, sky wrote:
> >>
> >> It seems like tab still triggers rerunning the query. It shouldn't do
> >> that.
> >
> > I'm not sure if I understand this point correctly. Do you mean we shouldn't
> > update popup when accepting the current instant suggest?
> 
> That's right. It should be no different than once suggest text becomes
> autocomplete text (the fade).
We still need to update popup when confirming the autocomplete text (removing
the blue background), at least the high light part will be changed.

> 
> > But I think we need
> > to
> > update popup in such case.
> 
> Why? We should wait until they type something more.
> 
>   -Scott
> >
> >
> >> &nbsp; -Scott
> >
> >> On Fri, Jan 14, 2011 at 9:57 PM, &nbsp;<mailto:suzhe@chromium.org> wrote:
> >> > Sorry for taking so long to update this CL. Please try it again.
> >> >
> >> > Regards
> >> > James Su
> >> >
> >> > On 2011/01/06 18:38:02, sky wrote:
> >> >>
> >> >> Let me know when it's updated, and I'll give it another try.
> >> >
> >> >> &nbsp; -Scott
> >> >
> >> >> On Thu, Jan 6, 2011 at 10:19 AM, &nbsp;<mailto:suzhe@chromium.org>
> >> >> wrote:
> >> >> > This CL still has problem when pressing Tab key to finalize instant
> >> >> > suggest
> >> >> > in
> >> >> > keyword mode. I'll fix the issue asap.
> >> >> >
> >> >> > http://codereview.chromium.org/5966006/
> >> >> >
> >> >
> >> >
> >> >
> >> > http://codereview.chromium.org/5966006/
> >> >
> >
> >
> >
> > http://codereview.chromium.org/5966006/
> >

[sky, 2011-01-19 00:29:16]

On Tue, Jan 18, 2011 at 3:51 PM,  <suzhe@chromium.org> wrote:
> On 2011/01/18 23:36:07, sky wrote:
>>
>> On Tue, Jan 18, 2011 at 3:22 PM,  <mailto:suzhe@chromium.org> wrote:
>> > On 2011/01/18 23:06:23, sky wrote:
>> >>
>> >> It seems like tab still triggers rerunning the query. It shouldn't do
>> >> that.
>> >
>> > I'm not sure if I understand this point correctly. Do you mean we
>> > shouldn't
>> > update popup when accepting the current instant suggest?
>
>> That's right. It should be no different than once suggest text becomes
>> autocomplete text (the fade).
>
> We still need to update popup when confirming the autocomplete text
> (removing
> the blue background), at least the high light part will be changed.

Yes, the highlight should change, but not the contents of the popup.

  -Scott

>
>
>> > But I think we need
>> > to
>> > update popup in such case.
>
>> Why? We should wait until they type something more.
>
>>   -Scott
>> >
>> >
>> >> &nbsp; -Scott
>> >
>> >> On Fri, Jan 14, 2011 at 9:57 PM, &nbsp;<mailto:suzhe@chromium.org>
>> >> wrote:
>> >> > Sorry for taking so long to update this CL. Please try it again.
>> >> >
>> >> > Regards
>> >> > James Su
>> >> >
>> >> > On 2011/01/06 18:38:02, sky wrote:
>> >> >>
>> >> >> Let me know when it's updated, and I'll give it another try.
>> >> >
>> >> >> &nbsp; -Scott
>> >> >
>> >> >> On Thu, Jan 6, 2011 at 10:19 AM, &nbsp;<mailto:suzhe@chromium.org>
>> >> >> wrote:
>> >> >> > This CL still has problem when pressing Tab key to finalize
>> >> >> > instant
>> >> >> > suggest
>> >> >> > in
>> >> >> > keyword mode. I'll fix the issue asap.
>> >> >> >
>> >> >> > http://codereview.chromium.org/5966006/
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >> > http://codereview.chromium.org/5966006/
>> >> >
>> >
>> >
>> >
>> > http://codereview.chromium.org/5966006/
>> >
>
>
>
> http://codereview.chromium.org/5966006/
>

[James Su, 2011-01-19 00:35:19]

On 2011/01/19 00:29:16, sky wrote:
> On Tue, Jan 18, 2011 at 3:51 PM,  <mailto:suzhe@chromium.org> wrote:
> > On 2011/01/18 23:36:07, sky wrote:
> >>
> >> On Tue, Jan 18, 2011 at 3:22 PM, &nbsp;<mailto:suzhe@chromium.org> wrote:
> >> > On 2011/01/18 23:06:23, sky wrote:
> >> >>
> >> >> It seems like tab still triggers rerunning the query. It shouldn't do
> >> >> that.
> >> >
> >> > I'm not sure if I understand this point correctly. Do you mean we
> >> > shouldn't
> >> > update popup when accepting the current instant suggest?
> >
> >> That's right. It should be no different than once suggest text becomes
> >> autocomplete text (the fade).
> >
> > We still need to update popup when confirming the autocomplete text
> > (removing
> > the blue background), at least the high light part will be changed.
> 
> Yes, the highlight should change, but not the contents of the popup.
I mean the highlight part in result items, i.e. the |matches_| of each results.

> 
>   -Scott
> 
> >
> >
> >> > But I think we need
> >> > to
> >> > update popup in such case.
> >
> >> Why? We should wait until they type something more.
> >
> >> &nbsp; -Scott
> >> >
> >> >
> >> >> &nbsp; -Scott
> >> >
> >> >> On Fri, Jan 14, 2011 at 9:57 PM, &nbsp;<mailto:suzhe@chromium.org>
> >> >> wrote:
> >> >> > Sorry for taking so long to update this CL. Please try it again.
> >> >> >
> >> >> > Regards
> >> >> > James Su
> >> >> >
> >> >> > On 2011/01/06 18:38:02, sky wrote:
> >> >> >>
> >> >> >> Let me know when it's updated, and I'll give it another try.
> >> >> >
> >> >> >> &nbsp; -Scott
> >> >> >
> >> >> >> On Thu, Jan 6, 2011 at 10:19 AM, &nbsp;<mailto:suzhe@chromium.org>
> >> >> >> wrote:
> >> >> >> > This CL still has problem when pressing Tab key to finalize
> >> >> >> > instant
> >> >> >> > suggest
> >> >> >> > in
> >> >> >> > keyword mode. I'll fix the issue asap.
> >> >> >> >
> >> >> >> > http://codereview.chromium.org/5966006/
> >> >> >> >
> >> >> >
> >> >> >
> >> >> >
> >> >> > http://codereview.chromium.org/5966006/
> >> >> >
> >> >
> >> >
> >> >
> >> > http://codereview.chromium.org/5966006/
> >> >
> >
> >
> >
> > http://codereview.chromium.org/5966006/
> >

[sky, 2011-01-19 01:34:00]

On Tue, Jan 18, 2011 at 4:35 PM,  <suzhe@chromium.org> wrote:
> On 2011/01/19 00:29:16, sky wrote:
>>
>> On Tue, Jan 18, 2011 at 3:51 PM,  <mailto:suzhe@chromium.org> wrote:
>> > On 2011/01/18 23:36:07, sky wrote:
>> >>
>> >> On Tue, Jan 18, 2011 at 3:22 PM, &nbsp;<mailto:suzhe@chromium.org>
>> >> wrote:
>> >> > On 2011/01/18 23:06:23, sky wrote:
>> >> >>
>> >> >> It seems like tab still triggers rerunning the query. It shouldn't
>> >> >> do
>> >> >> that.
>> >> >
>> >> > I'm not sure if I understand this point correctly. Do you mean we
>> >> > shouldn't
>> >> > update popup when accepting the current instant suggest?
>> >
>> >> That's right. It should be no different than once suggest text becomes
>> >> autocomplete text (the fade).
>> >
>> > We still need to update popup when confirming the autocomplete text
>> > (removing
>> > the blue background), at least the high light part will be changed.
>
>> Yes, the highlight should change, but not the contents of the popup.
>
> I mean the highlight part in result items, i.e. the |matches_| of each
> results.


I noticed that google updates the popup when you hit tab. Let me talk
with Nicholas/Glen and get their opinion.

  -Scott
>

>
>>   -Scott
>
>> >
>> >
>> >> > But I think we need
>> >> > to
>> >> > update popup in such case.
>> >
>> >> Why? We should wait until they type something more.
>> >
>> >> &nbsp; -Scott
>> >> >
>> >> >
>> >> >> &nbsp; -Scott
>> >> >
>> >> >> On Fri, Jan 14, 2011 at 9:57 PM, &nbsp;<mailto:suzhe@chromium.org>
>> >> >> wrote:
>> >> >> > Sorry for taking so long to update this CL. Please try it again.
>> >> >> >
>> >> >> > Regards
>> >> >> > James Su
>> >> >> >
>> >> >> > On 2011/01/06 18:38:02, sky wrote:
>> >> >> >>
>> >> >> >> Let me know when it's updated, and I'll give it another try.
>> >> >> >
>> >> >> >> &nbsp; -Scott
>> >> >> >
>> >> >> >> On Thu, Jan 6, 2011 at 10:19 AM,
>> >> >> >> &nbsp;<mailto:suzhe@chromium.org>
>> >> >> >> wrote:
>> >> >> >> > This CL still has problem when pressing Tab key to finalize
>> >> >> >> > instant
>> >> >> >> > suggest
>> >> >> >> > in
>> >> >> >> > keyword mode. I'll fix the issue asap.
>> >> >> >> >
>> >> >> >> > http://codereview.chromium.org/5966006/
>> >> >> >> >
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >> > http://codereview.chromium.org/5966006/
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >> > http://codereview.chromium.org/5966006/
>> >> >
>> >
>> >
>> >
>> > http://codereview.chromium.org/5966006/
>> >
>
>
>
> http://codereview.chromium.org/5966006/
>

[James Su, 2011-01-19 02:01:01]

On 2011/01/19 00:35:19, James Su wrote:
> On 2011/01/19 00:29:16, sky wrote:
> > On Tue, Jan 18, 2011 at 3:51 PM,  <mailto:suzhe@chromium.org> wrote:
> > > On 2011/01/18 23:36:07, sky wrote:
> > >>
> > >> On Tue, Jan 18, 2011 at 3:22 PM, &nbsp;<mailto:suzhe@chromium.org> wrote:
> > >> > On 2011/01/18 23:06:23, sky wrote:
> > >> >>
> > >> >> It seems like tab still triggers rerunning the query. It shouldn't do
> > >> >> that.
> > >> >
> > >> > I'm not sure if I understand this point correctly. Do you mean we
> > >> > shouldn't
> > >> > update popup when accepting the current instant suggest?
> > >
> > >> That's right. It should be no different than once suggest text becomes
> > >> autocomplete text (the fade).
> > >
> > > We still need to update popup when confirming the autocomplete text
> > > (removing
> > > the blue background), at least the high light part will be changed.
> > 
> > Yes, the highlight should change, but not the contents of the popup.
> I mean the highlight part in result items, i.e. the |matches_| of each
results.
Note that popup will also be updated when accepting ordinary inline autocomplete
by pressing Right.

> 
> > 
> >   -Scott
> > 
> > >
> > >
> > >> > But I think we need
> > >> > to
> > >> > update popup in such case.
> > >
> > >> Why? We should wait until they type something more.
> > >
> > >> &nbsp; -Scott
> > >> >
> > >> >
> > >> >> &nbsp; -Scott
> > >> >
> > >> >> On Fri, Jan 14, 2011 at 9:57 PM, &nbsp;<mailto:suzhe@chromium.org>
> > >> >> wrote:
> > >> >> > Sorry for taking so long to update this CL. Please try it again.
> > >> >> >
> > >> >> > Regards
> > >> >> > James Su
> > >> >> >
> > >> >> > On 2011/01/06 18:38:02, sky wrote:
> > >> >> >>
> > >> >> >> Let me know when it's updated, and I'll give it another try.
> > >> >> >
> > >> >> >> &nbsp; -Scott
> > >> >> >
> > >> >> >> On Thu, Jan 6, 2011 at 10:19 AM, &nbsp;<mailto:suzhe@chromium.org>
> > >> >> >> wrote:
> > >> >> >> > This CL still has problem when pressing Tab key to finalize
> > >> >> >> > instant
> > >> >> >> > suggest
> > >> >> >> > in
> > >> >> >> > keyword mode. I'll fix the issue asap.
> > >> >> >> >
> > >> >> >> > http://codereview.chromium.org/5966006/
> > >> >> >> >
> > >> >> >
> > >> >> >
> > >> >> >
> > >> >> > http://codereview.chromium.org/5966006/
> > >> >> >
> > >> >
> > >> >
> > >> >
> > >> > http://codereview.chromium.org/5966006/
> > >> >
> > >
> > >
> > >
> > > http://codereview.chromium.org/5966006/
> > >

[James Su, 2011-01-20 23:19:07]

Any update?

[sky, 2011-01-21 00:21:01]

Sorry, Nicholas was out yesterday and I'm out today. The more I think
about it, the more I think we shouldn't run the query. The problem is
if we rerun the query you might a new suggestion and new autocomplete
text. I think that would be supremely annoying, don't you?

I will talk with Nicholas/Glen as soon as I can though.

  -Scott

On Thu, Jan 20, 2011 at 3:19 PM,  <suzhe@chromium.org> wrote:
> Any update?
>
> http://codereview.chromium.org/5966006/
>

[James Su, 2011-01-21 00:47:16]

On 2011/01/21 00:21:01, sky wrote:
> Sorry, Nicholas was out yesterday and I'm out today. The more I think
> about it, the more I think we shouldn't run the query. The problem is
> if we rerun the query you might a new suggestion and new autocomplete
> text. I think that would be supremely annoying, don't you?
I was afraid of this issue as well. But I never noticed such behavior. I didn't
noticed such behavior in Google Search either.
And if we automatically get a new suggestion after rerunning the query with the
previously suggested result, then IMO it makes more sense for the search engine
to give us the final suggestion at the first time.
So I believe Google search guarantees not to give us further suggestions in such
situation.

Two concrete examples:
1. When you input "tsingh", Google Search will return "ua university" as the
instant suggest, instead of just "ua". And you won't get more suggest after
confirming "tsinghua university" by either pressing Tab key or typing the
instant suggest one by one character. In another word, the effect of pressing
Tab key is actually same as typing the instant suggest one by one character.
Then if you press space after confirming "tsinghua university", you will get the
next suggestion "beijing". And so on.

2. When you input "nswi", you will get instant suggest "ndow". And you won't get
anything after finishing "nswindow". But after inputting an extra space, you
will get the next suggest "reference".

> 
> I will talk with Nicholas/Glen as soon as I can though.
> 
>   -Scott
> 
> On Thu, Jan 20, 2011 at 3:19 PM,  <mailto:suzhe@chromium.org> wrote:
> > Any update?
> >
> > http://codereview.chromium.org/5966006/
> >

[James Su, 2011-01-21 22:04:08]

As M10 deadline is coming. I'm wondering if it's better to postpone the bug to
M11 so that we can have more time to think about it?

[sky, 2011-01-24 16:56:08]

Yes, that's fine by me.

  -Scott

On Fri, Jan 21, 2011 at 2:04 PM,  <suzhe@chromium.org> wrote:
> As M10 deadline is coming. I'm wondering if it's better to postpone the bug
> to
> M11 so that we can have more time to think about it?
>
> http://codereview.chromium.org/5966006/
>

[James Su, 2011-01-27 19:22:45]

The CL has been rebased. Add oshima for AutocompleteEditViewViews part.

[sky, 2011-01-27 19:55:31]

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1653:
OnBeforePossibleChange();
Do we need the OnBefore/after here?

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1888: gint cursor_pos;
Can this be implemented in terms of GetSelection?

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right):

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:208:
OnBeforePossibleChange();
Same question about the need for OnBefore/after here.

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:1980: ScopedFreeze
freeze(this, GetTextObjectModel());
Again, do you need onbefore/after here?

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:2577: long length =
GetTextLength();
For consistency it would be nice if you had this on all platforms.

[James Su, 2011-01-27 20:07:16]

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc (right):

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1653:
OnBeforePossibleChange();
On 2011/01/27 19:55:31, sky wrote:
> Do we need the OnBefore/after here?

Yes we need it to inform the model possible selection range change. It may cause
the model to accept the inline autocomplete and update popup accordingly. Just
like what Right key does.

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_gtk.cc:1888: gint cursor_pos;
On 2011/01/27 19:55:31, sky wrote:
> Can this be implemented in terms of GetSelection?

I'll recheck this part to see how we can do it better. After migrating to
string16, the code may have problem handling non-BMP characters.

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:2577: long length =
GetTextLength();
On 2011/01/27 19:55:31, sky wrote:
> For consistency it would be nice if you had this on all platforms.

This is a method of CRichEditCtrl, we probably need to use a different name if
we want to add it into AutocompleteEditView interface. Do you have any
suggestion?

[oshima, 2011-01-27 21:17:44]

James, I was working on refactoring a while ago, that
eliminates round trip to controller to commit instant. I put it on hold because
I didn't have time to test it on mac/win, but am going to revive soon. Just
heads up and you can go ahead with your change.

http://codereview.chromium.org/6148004/

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right):

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:212: }
instant is not working on views impl and i'd rather leave it unimplemented
rather than having something incomplete.

[sky, 2011-01-27 21:47:20]

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:2577: long length =
GetTextLength();
On 2011/01/27 20:07:16, James Su wrote:
> On 2011/01/27 19:55:31, sky wrote:
> > For consistency it would be nice if you had this on all platforms.
> 
> This is a method of CRichEditCtrl, we probably need to use a different name if
> we want to add it into AutocompleteEditView interface. Do you have any
> suggestion?

I wasn't suggesting to make it part of the interface, just that you make the
implementation on the other platforms similiar using the same name. For example,
on the mac you have:

NSRange selected_range = GetSelectedRange();
  825     NSUInteger length = [[[field_ currentEditor] string] length];
  826     if (selected_range.length != 0 || selected_range.location < length) {


Which could be the same as if (!IsCaretAtEnd).

[James Su, 2011-01-27 21:55:52]

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_views.cc (right):

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_views.cc:212: }
On 2011/01/27 21:17:44, oshima wrote:
> instant is not working on views impl and i'd rather leave it unimplemented
> rather than having something incomplete.

Above code is nothing to do with instant support. It only changes the behavior
of Tab key when the cursor is not at the line end.

[James Su, 2011-01-27 22:04:59]

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):

http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
chrome/browser/autocomplete/autocomplete_edit_view_win.cc:2577: long length =
GetTextLength();
On 2011/01/27 21:47:20, sky wrote:
> On 2011/01/27 20:07:16, James Su wrote:
> > On 2011/01/27 19:55:31, sky wrote:
> > > For consistency it would be nice if you had this on all platforms.
> > 
> > This is a method of CRichEditCtrl, we probably need to use a different name
if
> > we want to add it into AutocompleteEditView interface. Do you have any
> > suggestion?
> 
> I wasn't suggesting to make it part of the interface, just that you make the
> implementation on the other platforms similiar using the same name. For
example,
> on the mac you have:
> 
> NSRange selected_range = GetSelectedRange();
>   825     NSUInteger length = [[[field_ currentEditor] string] length];
>   826     if (selected_range.length != 0 || selected_range.location < length)
{
> 
> 
> Which could be the same as if (!IsCaretAtEnd).

Oh I see. How about add IsCaretAtEnd and PlaceCaretAt to the interface?

[oshima, 2011-01-27 22:13:40]

On Thu, Jan 27, 2011 at 1:55 PM, <suzhe@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
> File chrome/browser/autocomplete/autocomplete_edit_view_views.cc
> (right):
>
>
>
http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete_edit_view_views.cc:212: }
>
> On 2011/01/27 21:17:44, oshima wrote:
>
>> instant is not working on views impl and i'd rather leave it
>>
> unimplemented
>
>> rather than having something incomplete.
>>
>
> Above code is nothing to do with instant support. It only changes the
> behavior of Tab key when the cursor is not at the line end.
>
> Oh, sorry. I somehow thought this change is for instant case.  (probably
because my comment below :)
I have no further question than the one sky made and if he gives lgtm, lgtm.


>
> http://codereview.chromium.org/5966006/
>

[sky, 2011-01-27 22:19:40]

On Thu, Jan 27, 2011 at 2:04 PM,  <suzhe@chromium.org> wrote:
>
>
http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
> File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):
>
>
http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
> chrome/browser/autocomplete/autocomplete_edit_view_win.cc:2577: long
> length = GetTextLength();
> On 2011/01/27 21:47:20, sky wrote:
>>
>> On 2011/01/27 20:07:16, James Su wrote:
>> > On 2011/01/27 19:55:31, sky wrote:
>> > > For consistency it would be nice if you had this on all platforms.
>> >
>> > This is a method of CRichEditCtrl, we probably need to use a
>
> different name if
>>
>> > we want to add it into AutocompleteEditView interface. Do you have
>
> any
>>
>> > suggestion?
>
>> I wasn't suggesting to make it part of the interface, just that you
>
> make the
>>
>> implementation on the other platforms similiar using the same name.
>
> For example,
>>
>> on the mac you have:
>
>> NSRange selected_range = GetSelectedRange();
>>   825     NSUInteger length = [[[field_ currentEditor] string]
>
> length];
>>
>>   826     if (selected_range.length != 0 || selected_range.location <
>
> length) {
>
>
>> Which could be the same as if (!IsCaretAtEnd).
>
> Oh I see. How about add IsCaretAtEnd and PlaceCaretAt to the interface?

You needn't make them part of the interface, just name the methods the
same on all platforms so that the implementations look as close a
possible.

  -Scott

[James Su, 2011-01-27 22:28:02]

On 2011/01/27 22:19:40, sky wrote:
> On Thu, Jan 27, 2011 at 2:04 PM,  <mailto:suzhe@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
> > File chrome/browser/autocomplete/autocomplete_edit_view_win.cc (right):
> >
> >
>
http://codereview.chromium.org/5966006/diff/49001/chrome/browser/autocomplete...
> > chrome/browser/autocomplete/autocomplete_edit_view_win.cc:2577: long
> > length = GetTextLength();
> > On 2011/01/27 21:47:20, sky wrote:
> >>
> >> On 2011/01/27 20:07:16, James Su wrote:
> >> > On 2011/01/27 19:55:31, sky wrote:
> >> > > For consistency it would be nice if you had this on all platforms.
> >> >
> >> > This is a method of CRichEditCtrl, we probably need to use a
> >
> > different name if
> >>
> >> > we want to add it into AutocompleteEditView interface. Do you have
> >
> > any
> >>
> >> > suggestion?
> >
> >> I wasn't suggesting to make it part of the interface, just that you
> >
> > make the
> >>
> >> implementation on the other platforms similiar using the same name.
> >
> > For example,
> >>
> >> on the mac you have:
> >
> >> NSRange selected_range = GetSelectedRange();
> >> &nbsp; 825 &nbsp; &nbsp; NSUInteger length = [[[field_ currentEditor]
string]
> >
> > length];
> >>
> >> &nbsp; 826 &nbsp; &nbsp; if (selected_range.length != 0 ||
selected_range.location <
> >
> > length) {
> >
> >
> >> Which could be the same as if (!IsCaretAtEnd).
> >
> > Oh I see. How about add IsCaretAtEnd and PlaceCaretAt to the interface?
> 
> You needn't make them part of the interface, just name the methods the
> same on all platforms so that the implementations look as close a
> possible.
Ok, I'll update the CL asap. But actually I do want to add GetTextLength() to
the interface. There are several GetText().size() calls in autocomplete_model.cc
and test code.

> 
>   -Scott

[James Su, 2011-01-28 00:59:34]

Please help review again.

[sky, 2011-01-28 01:12:40]

LGTM