Make Google Search autocomplete provider cursor aware.

Make Google Search autocomplete provider cursor aware.

Specifically:
(1) Introduce a new AutocompleteInput field, cursor_position.
(2) Change AutocompleteController::Start() interface to take
    const AutocompleteInput& input
(3) Introduce a new Google specific replacement, {google:cursorPosition}
    and update the corresponding search provider template
(4) Refactor all existing callers of AC::Start() to take AI param instead

BUG=29999
TEST=AutocompleteInputTest.InputTypeWithCursorPosition,TemplateURLTest.ReplaceCursorPosition

Also tested a local chrome build by sending various queries, e.g. "it|pasta recipes" (| indicates cursor position). 


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173131

[Bart N., 2012-12-04 01:33:03]

This is ready for review.

Please, don't get set back by the big number of changed files; unfortunately, it
was necessary to fix the AutocompleteController::Start interface, but I hope it
was for better!

[Bart N., 2012-12-04 01:37:52]

Mark - I forgot to mention - I'm planning to update omnibox debug console to
support cp, but in the followup CL; this one is already too big to my like.

[Peter Kasting, 2012-12-04 03:33:26]

I'm not going to get to this until Wednesday.

[Bart N., 2012-12-04 17:55:26]

On 2012/12/04 03:33:26, Peter Kasting wrote:
> I'm not going to get to this until Wednesday.
Thanks for the heads-up and no worries.

I'll have one more change for review (experiment logging, which you've already
seen) and I'm targeting both changes for M25 granted that you will have some
time for the review.  The logging change is more important than this one (if I
were to choose), but it'd be nice to get both in M25 :) Thanks again!

B.

[Peter Kasting, 2012-12-05 20:49:38]

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_browsertest.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_browsertest.cc:124:
AutocompleteInput(ASCIIToUTF16("chrome"), string16::npos, string16(),
Nit: You can save a line by leaving the wrapping like it was and putting
"AutocompleteInput(" on the end of the previous line.  (Same comment applies
other places)

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_classifier.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_classifier.cc:55:
controller_->Start(AutocompleteInput(text, string16::npos, desired_tld, true,
Nit: For example, here I'd probably break after "AutocompleteInput(" and indent
4, so I could fit all the remaining args on fewer lines.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:19: void
AdjustCursorPositionIfNecessary(size_t num_leading_chars_removed,
You should be able to get by without this by using the OffsetAdjuster class (and
possibly the LimitOffset class) found in base/utf_offset_string_conversions.h,
which are designed to help adjust offsets into strings as those strings get
transformed.

One difference between that class' behavior and this function's is that it will
set an offset to npos if the offset points into a section of string that is
removed.  This is probably more correct in this case; otherwise we'll treat the
cursor the same before/after leading whitespace/question mark, which seems
slightly wrong to me.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:85: string16* text) {
Nit: Fix alignment

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:86: if (type == FORCED_QUERY
&& !text->empty() && (*text)[0] == L'?') {
Nit: Reverse conditional and early-return to eliminate {}

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:51: // the cursor with the
query respectively.
Nit: Add "|cursor_position| may be set to string16::npos if the input string
doesn't come directly from the user's typing."

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:124: // used or past the last
character.
Seems like for a string "abc", we should allow the following:

|abc -> 0
a|bc -> 1
ab|c -> 2
abc| -> 3

This comment makes it sound like the last case isn't valid, and we just use npos
there.  Isn't there value in distinguishing "cursor at end" from "no cursor --
text didn't come from user typing"?

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:130: void UpdateText(const
string16& text, size_t cursor_position,
Nit: One arg per line

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_provider.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/history_provider.cc:139: // cursor aware (which is
true as of Nov 2012).
We probably should do the right thing here, since Mark wants to make the history
system cursor-aware (separate bug on file).  We can mostly handle things with
manual adjustments or using the conversion functions in
utf_offset_conversions.h.  The big piece is that we probably have to make
FixupURL() handle offset adjustment, which is nasty.

This could all be done as a separate change to limit scope creep.  In that case
I'd change the TODO text here to refer to what needs to be done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 47
There's another CL outstanding elsewhere which also revs this version number --
make sure if you land after it does that you still bump the number, as otherwise
the merge process will probably happily leave this at 47.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:439: const std::string
url;
Nit: Indenting

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:423: size_t cursor_position_;
I don't think we need to track this as a member (or should).  We should just
fetch the cursor position dynamically when needed, e.g. by calling
OmniboxView::GetSelectionBounds() and using the |end| value it returns.

[Bart N., 2012-12-06 21:43:32]

Done all, PTAL.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_browsertest.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_browsertest.cc:124:
AutocompleteInput(ASCIIToUTF16("chrome"), string16::npos, string16(),
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Nit: You can save a line by leaving the wrapping like it was and putting
> "AutocompleteInput(" on the end of the previous line.  (Same comment applies
> other places)

Done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_classifier.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_classifier.cc:55:
controller_->Start(AutocompleteInput(text, string16::npos, desired_tld, true,
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Nit: For example, here I'd probably break after "AutocompleteInput(" and
indent
> 4, so I could fit all the remaining args on fewer lines.
Yup. Done in all places.

BTW, you realize that the current style is totally not consistent across various
files?

My approach, while doing refactorings like this one, has been to keep it
consistent with whatever I encounter in a given file. Otherwise, we may end up
with mixed style within same file.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:19: void
AdjustCursorPositionIfNecessary(size_t num_leading_chars_removed,
On 2012/12/05 20:49:38, Peter Kasting wrote:
> You should be able to get by without this by using the OffsetAdjuster class
(and
> possibly the LimitOffset class) found in base/utf_offset_string_conversions.h,
> which are designed to help adjust offsets into strings as those strings get
> transformed.
In this particular case, I strongly disagree with you. I looked at the class you
mentioned and it seems overkill to use it in this particular case. Using the
class requires defining a vector of offsets and I don't think it was intended to
be used in simple cases like this. This sounds like asking to replace
std::min(a,b) with a vector of two elements, then calling std::sort and picking
the first element :)

> 
> One difference between that class' behavior and this function's is that it
will
> set an offset to npos if the offset points into a section of string that is
> removed.  This is probably more correct in this case; otherwise we'll treat
the
> cursor the same before/after leading whitespace/question mark, which seems
> slightly wrong to me.
So this is about what to do in my case below - whether to have *cursor_position
= 0 or *cursor_position = string::npos.

The reason why I prefer 0 is because this is the current behavior of
searchbox+CS. searchbox doesn't alter query in any way and sends it along with
raw cursor position. Then, we normalize the query on the server side (CS) and
set it to 0 if <=0 after normalization.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:85: string16* text) {
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Nit: Fix alignment

Done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:86: if (type == FORCED_QUERY
&& !text->empty() && (*text)[0] == L'?') {
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Nit: Reverse conditional and early-return to eliminate {}

Done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:86: if (type == FORCED_QUERY
&& !text->empty() && (*text)[0] == L'?') {
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Nit: Reverse conditional and early-return to eliminate {}

Done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:51: // the cursor with the
query respectively.
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Nit: Add "|cursor_position| may be set to string16::npos if the input string
> doesn't come directly from the user's typing."

Done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:51: // the cursor with the
query respectively.
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Nit: Add "|cursor_position| may be set to string16::npos if the input string
> doesn't come directly from the user's typing."

Done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:124: // used or past the last
character.
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Seems like for a string "abc", we should allow the following:
> 
> |abc -> 0
> a|bc -> 1
> ab|c -> 2
> abc| -> 3
> 
> This comment makes it sound like the last case isn't valid, and we just use
npos
> there.  Isn't there value in distinguishing "cursor at end" from "no cursor --
> text didn't come from user typing"?
It isn't a bug, it was done intentionally. However, I consulted searchbox folks,
and it seems that cp is always send from JS (details omitted). We agreed to make
it consistent, so I've just changed the comment as well as the implementation.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:130: void UpdateText(const
string16& text, size_t cursor_position,
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Nit: One arg per line

Done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_provider.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/autocomple...
chrome/browser/autocomplete/history_provider.cc:139: // cursor aware (which is
true as of Nov 2012).
On 2012/12/05 20:49:38, Peter Kasting wrote:
> We probably should do the right thing here, since Mark wants to make the
history
> system cursor-aware (separate bug on file).  We can mostly handle things with
> manual adjustments or using the conversion functions in
> utf_offset_conversions.h.  The big piece is that we probably have to make
> FixupURL() handle offset adjustment, which is nasty.
> 
> This could all be done as a separate change to limit scope creep.  In that
case
> I'd change the TODO text here to refer to what needs to be done.

Done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 47
On 2012/12/05 20:49:38, Peter Kasting wrote:
> There's another CL outstanding elsewhere which also revs this version number
--
> make sure if you land after it does that you still bump the number, as
otherwise
> the merge process will probably happily leave this at 47.
Yup, thanks for the reminder.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/search_eng...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/search_eng...
chrome/browser/search_engines/template_url_unittest.cc:439: const std::string
url;
On 2012/12/05 20:49:38, Peter Kasting wrote:
> Nit: Indenting

Done.

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:423: size_t cursor_position_;
On 2012/12/05 20:49:38, Peter Kasting wrote:
> I don't think we need to track this as a member (or should).  We should just
> fetch the cursor position dynamically when needed, e.g. by calling
> OmniboxView::GetSelectionBounds() and using the |end| value it returns.
Great suggestion, thanks! I wonder why it is end rather than start?

[Peter Kasting, 2012-12-06 22:16:35]

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11414303/diff/13002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:423: size_t cursor_position_;
On 2012/12/06 21:43:32, Bart N. wrote:
> On 2012/12/05 20:49:38, Peter Kasting wrote:
> > I don't think we need to track this as a member (or should).  We should just
> > fetch the cursor position dynamically when needed, e.g. by calling
> > OmniboxView::GetSelectionBounds() and using the |end| value it returns.
> Great suggestion, thanks! I wonder why it is end rather than start?

Well, think about how you create a selection (with keys or mouse), and then if
you hit an arrow key after that, where you'd expect the cursor to have moved
from.

[Bart N., 2012-12-06 22:24:26]

On 2012/12/06 22:16:35, Peter Kasting wrote:
>
https://codereview.chromium.org/11414303/diff/13002/chrome/browser/ui/omnibox...
> File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):
> 
>
https://codereview.chromium.org/11414303/diff/13002/chrome/browser/ui/omnibox...
> chrome/browser/ui/omnibox/omnibox_edit_model.h:423: size_t cursor_position_;
> On 2012/12/06 21:43:32, Bart N. wrote:
> > On 2012/12/05 20:49:38, Peter Kasting wrote:
> > > I don't think we need to track this as a member (or should).  We should
just
> > > fetch the cursor position dynamically when needed, e.g. by calling
> > > OmniboxView::GetSelectionBounds() and using the |end| value it returns.
> > Great suggestion, thanks! I wonder why it is end rather than start?
> 
> Well, think about how you create a selection (with keys or mouse), and then if
> you hit an arrow key after that, where you'd expect the cursor to have moved
> from.
Is your assumption based on fact that you select from left to right? It's not
always the case.

In fact, I often select (using keyboard) from right to left, if I want to
correct a typo.

[Peter Kasting, 2012-12-06 22:26:23]

On 2012/12/06 22:24:26, Bart N. wrote:
> > On 2012/12/06 21:43:32, Bart N. wrote:
> > > On 2012/12/05 20:49:38, Peter Kasting wrote:
> > > > I don't think we need to track this as a member (or should).  We should
> just
> > > > fetch the cursor position dynamically when needed, e.g. by calling
> > > > OmniboxView::GetSelectionBounds() and using the |end| value it returns.
> > > Great suggestion, thanks! I wonder why it is end rather than start?
> > 
> > Well, think about how you create a selection (with keys or mouse), and then
if
> > you hit an arrow key after that, where you'd expect the cursor to have moved
> > from.
> Is your assumption based on fact that you select from left to right? It's not
> always the case.
> 
> In fact, I often select (using keyboard) from right to left, if I want to
> correct a typo.

In which case the end of the selection is earlier than the start.

The end of the selection is the endpoint, not the rightmost point.

[Bart N., 2012-12-06 22:39:40]

On 2012/12/06 22:26:23, Peter Kasting wrote:
> On 2012/12/06 22:24:26, Bart N. wrote:
> > > On 2012/12/06 21:43:32, Bart N. wrote:
> > > > On 2012/12/05 20:49:38, Peter Kasting wrote:
> > > > > I don't think we need to track this as a member (or should).  We
should
> > just
> > > > > fetch the cursor position dynamically when needed, e.g. by calling
> > > > > OmniboxView::GetSelectionBounds() and using the |end| value it
returns.
> > > > Great suggestion, thanks! I wonder why it is end rather than start?
> > > 
> > > Well, think about how you create a selection (with keys or mouse), and
then
> if
> > > you hit an arrow key after that, where you'd expect the cursor to have
moved
> > > from.
> > Is your assumption based on fact that you select from left to right? It's
not
> > always the case.
> > 
> > In fact, I often select (using keyboard) from right to left, if I want to
> > correct a typo.
> 
> In which case the end of the selection is earlier than the start.
> 
> The end of the selection is the endpoint, not the rightmost point.
Odd - in my test code, I specifically put LOG(INFO) and printed cursor (before I
changed to your suggestion) and no matter how I would select (left->right,
right->left) selection_start <= selection_end.

Now as you pointed this out - I looked at the header and it claims that start
may be > end. Let me verify it in practice.

[Peter Kasting, 2012-12-06 22:52:25]

On 2012/12/06 22:39:40, Bart N. wrote:
> Now as you pointed this out - I looked at the header and it claims that start
> may be > end. Let me verify it in practice.

Note the code in OmniboxViewWin::GetSelection() that ensures we properly reverse
the selection if need be.  You don't get that same effect from just calling
GetSel().

[Bart N., 2012-12-06 23:10:31]

On 2012/12/06 22:52:25, Peter Kasting wrote:
> On 2012/12/06 22:39:40, Bart N. wrote:
> > Now as you pointed this out - I looked at the header and it claims that
start
> > may be > end. Let me verify it in practice.
> 
> Note the code in OmniboxViewWin::GetSelection() that ensures we properly
reverse
> the selection if need be.  You don't get that same effect from just calling
> GetSel().
Ah, that would explain it. However, I'm testing on Linux and it's always start
<= end, any idea why?

Come to think about it, I don't think that relying on selection is what we want.
We really want textfield_->GetCursorPosition() which is current used by
GetSelectionBounds().

  if (range.is_empty()) {
    // Omnibox API expects that selection bounds is at cursor position
    // if there is no selection.
    *start = textfield_->GetCursorPosition();
    *end = textfield_->GetCursorPosition();
  } else {
    *start = static_cast<size_t>(range.end());
    *end = static_cast<size_t>(range.start());
  }

Any idea why GetCursorPosition is not exposed in OmniboxView interface?

Anyway, please let me know if you are OK with end. It sounds like in some cases
it may be wrong, at least on Linux.

[Peter Kasting, 2012-12-07 00:03:29]

On 2012/12/06 23:10:31, Bart N. wrote:
> Ah, that would explain it. However, I'm testing on Linux and it's always start
> <= end, any idea why?

That seems bad.  That ought to cause various bugs on Linux unless people are
compensating for this elsewhere.  I forget precisely which bugs, unfortunately
:(.  But I know this is supposed to be important!

I suspect this has never been caught because you're looking at
omnibox_view_views.cc -- that's by far the most poorly-tested of the
implementations.  We should fix this bug immediately.

>   if (range.is_empty()) {
>     // Omnibox API expects that selection bounds is at cursor position
>     // if there is no selection.
>     *start = textfield_->GetCursorPosition();
>     *end = textfield_->GetCursorPosition();
>   } else {
>     *start = static_cast<size_t>(range.end());
>     *end = static_cast<size_t>(range.start());
>   }

^^^ Hey, what's up with end and start seemingly being reversed in that second
arm?  That just looks blatantly wrong?

> Any idea why GetCursorPosition is not exposed in OmniboxView interface?

That's a views-ism implementation detail, so it doesn't exist on other
platforms.  Other platforms could emulate it by calling through to
GetSelectionBounds() and doing I-don't-know-what if the bounds weren't
collapsed, but at that point you're not really saving anything over looking at
the selection bounds yourself like the current way.

Honestly I don't know why the views textfield even exposes this.  This is
probably another artifact of how ill-tested and poorly-owned the whole views
textfield control chain is.

> Anyway, please let me know if you are OK with end. It sounds like in some
cases
> it may be wrong, at least on Linux.

We should use end, but we should fix it to be right.

If you're already debugging this, can you trace into the textfield call and see
if there's some obvious culprit?  It seems like this ought to be easy since
we're implementing the textfield entirely ourselves.

[Bart N., 2012-12-07 17:49:22]

I finally got around looking at this more closely and the problem is no existent
(unlike before, with my original implementation) - now, after I had moved the
logic as you suggested to before autocomplete_controller_->Start(...), we never
reach this point with start != end. I think this is because when you select
something either with keys or mouse, we don't fetch autocompletions unless the
text changes, which requires entering a character or deleting it, which removes
selection. Even pasting text clears selection. Perhaps there are some obscure
cases where selection remains, but I wasn't able to find any case.

Anyway, this CL is ready for review.

[Bart N., 2012-12-11 17:50:59]

On 2012/12/07 17:49:22, Bart N. wrote:
> I finally got around looking at this more closely and the problem is no
existent
> (unlike before, with my original implementation) - now, after I had moved the
> logic as you suggested to before autocomplete_controller_->Start(...), we
never
> reach this point with start != end. I think this is because when you select
> something either with keys or mouse, we don't fetch autocompletions unless the
> text changes, which requires entering a character or deleting it, which
removes
> selection. Even pasting text clears selection. Perhaps there are some obscure
> cases where selection remains, but I wasn't able to find any case.
> 
> Anyway, this CL is ready for review.
Friendly ping?

[Peter Kasting, 2012-12-11 19:27:01]

LGTM

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:21: if
(num_leading_chars_removed == 0 || *cursor_position == string16::npos)
Nit: I don't think the added fast-path condition here is important to
performance.  It seems simpler and clearer to me to just leave it out and let us
subtract zero below.

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:54: if (cursor_position !=
string16::npos && cursor_position >= text.length()) {
When does this conditional actually fire?  I wonder if maybe whatever callers
need this should be adjusting the cursor position prior to entry and we should
just eliminate this here or convert to a DCHECK.

Also, no need for {}.

[Mark P, 2012-12-11 21:23:20]

lgtm

This is an exciting change!  Thank you for doing it.

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:54: //
Please restored the comment inadvertently deleted in the comment move:
// See desired_tld() for meaning of |desired_tld|.

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 47
Remember to resync and rebase your change and edit this number before
submitting.  (Other people have already incremented it.)

*** rebasing might not report this as an error; remember to change this number
yourself ***

[Bart N., 2012-12-11 21:47:47]

Thanks for the review. I've merged and resolved a few conflicts (including the
data version field) and I'm planning to submit it soon after running a final
end2end test.

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:21: if
(num_leading_chars_removed == 0 || *cursor_position == string16::npos)
On 2012/12/11 19:27:01, Peter Kasting wrote:
> Nit: I don't think the added fast-path condition here is important to
> performance.  It seems simpler and clearer to me to just leave it out and let
us
> subtract zero below.

Done.

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:54: if (cursor_position !=
string16::npos && cursor_position >= text.length()) {
On 2012/12/11 19:27:01, Peter Kasting wrote:
> When does this conditional actually fire?  I wonder if maybe whatever callers
> need this should be adjusting the cursor position prior to entry and we should
> just eliminate this here or convert to a DCHECK.
I don't think this is possible, but I don't really want to go to each place
where AutucompleteInput is constructed and ensure this is true. Instead, I think
it's safer to simply normalize the cursor (which is done anyway later in this
method) to avoid obscure cases where for instance selection goes beyond the text
etc.

I also believe that DCHECKs are evil and should be avoided as much as possible
(details omitted). For instance, Search SREs don't allow code with DCHECKs in
prod :)

> Also, no need for {}.
Done.

[Bart N., 2012-12-11 21:54:02]

Ooopps. I haven't noticed Mark's comments before I replied to Peter's feedback.
Replying in a sec.

[Bart N., 2012-12-11 22:03:05]

On 2012/12/11 21:23:20, Mark P wrote:
> lgtm
> 
> This is an exciting change!  Thank you for doing it.
Yup, I'm excited too! Especially in the context of looking at the affected slice
only, it will be fun :)

[Bart N., 2012-12-11 22:03:16]

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.h (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.h:54: //
On 2012/12/11 21:23:20, Mark P wrote:
> Please restored the comment inadvertently deleted in the comment move:
> // See desired_tld() for meaning of |desired_tld|.
Good catch! Done.

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 47
On 2012/12/11 21:23:20, Mark P wrote:
> Remember to resync and rebase your change and edit this number before
> submitting.  (Other people have already incremented it.)
> 
> *** rebasing might not report this as an error; remember to change this number
> yourself ***
Yup - I remembered this (we had very similar issues in our CS code). However, I
was nicely surprised to actually see a conflict. The filed changed to 48
meanwhile, yet the merge was smart enough to detect it rather than thinking that
I simply wanted to decrease the number.

[Peter Kasting, 2012-12-11 22:15:50]

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:54: if (cursor_position !=
string16::npos && cursor_position >= text.length()) {
On 2012/12/11 21:47:47, Bart N. wrote:
> On 2012/12/11 19:27:01, Peter Kasting wrote:
> > When does this conditional actually fire?  I wonder if maybe whatever
callers
> > need this should be adjusting the cursor position prior to entry and we
should
> > just eliminate this here or convert to a DCHECK.
> I don't think this is possible, but I don't really want to go to each place
> where AutucompleteInput is constructed and ensure this is true.

:(

That's never going to be an argument that wins my support.  Plus, you're adding
all the code that sets the cursor position in this very changelist.  You're
effectively saying you don't know what your additions do.  That doesn't make
much sense.

> I also believe that DCHECKs are evil and should be avoided as much as possible
> (details omitted). For instance, Search SREs don't allow code with DCHECKs in
> prod :)

Different teams may have different intents for how code should be interpreted. 
In Chrome, DCHECKs mean "this is a function pre/post-condition" and are
effectively a form of documentation.  Because of this we want to encourage
DCHECKs wherever possible to guide readers in understanding the invariants in
play at any given time.

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 47
On 2012/12/11 22:03:16, Bart N. wrote:
> On 2012/12/11 21:23:20, Mark P wrote:
> > Remember to resync and rebase your change and edit this number before
> > submitting.  (Other people have already incremented it.)
> > 
> > *** rebasing might not report this as an error; remember to change this
number
> > yourself ***
> Yup - I remembered this (we had very similar issues in our CS code). However,
I
> was nicely surprised to actually see a conflict. The filed changed to 48
> meanwhile, yet the merge was smart enough to detect it rather than thinking
that
> I simply wanted to decrease the number.

Merge will reject anything other than a server-side change that duplicates your
client-side change.  The worrisome case is when someone else's CL changes this
to 47 :)

[Bart N., 2012-12-11 22:34:15]

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:54: if (cursor_position !=
string16::npos && cursor_position >= text.length()) {
On 2012/12/11 22:15:50, Peter Kasting wrote:
> On 2012/12/11 21:47:47, Bart N. wrote:
> > On 2012/12/11 19:27:01, Peter Kasting wrote:
> > > When does this conditional actually fire?  I wonder if maybe whatever
> callers
> > > need this should be adjusting the cursor position prior to entry and we
> should
> > > just eliminate this here or convert to a DCHECK.
> > I don't think this is possible, but I don't really want to go to each place
> > where AutucompleteInput is constructed and ensure this is true.
> 
> :(
> 
> That's never going to be an argument that wins my support.  Plus, you're
adding
> all the code that sets the cursor position in this very changelist.  You're
> effectively saying you don't know what your additions do.  That doesn't make
> much sense.
You may remember your comment regarding selection and views implementation (on
this thread). Just by seeing the buggy behavior in views_views.cc (start/end are
reversed) and your remark regarding the code quality and lack of unit tests, I
really have no trust regarding the various view implementations (another example
is Linux view behavior). Rather than letting it fail in some cases, I much
prefer to ensure that 100% of the time that cursor is set properly, because it
may affect the results quality. There is nothing wrong with this, because not
only is this extra check cheap, but it's also conceptually in the right place,
because below we are going to normalize the cursor position anyway. The logic is
kept in one place and is easy to understand.

Saying that I don't know what my code does is a little bit unfair :)
> 
> > I also believe that DCHECKs are evil and should be avoided as much as
possible
> > (details omitted). For instance, Search SREs don't allow code with DCHECKs
in
> > prod :)
> 
> Different teams may have different intents for how code should be interpreted.

> In Chrome, DCHECKs mean "this is a function pre/post-condition" and are
> effectively a form of documentation.  Because of this we want to encourage
> DCHECKs wherever possible to guide readers in understanding the invariants in
> play at any given time.
While this is perhaps a good argument, I've seen enough cases where people put
in DCHECK non-trivial code, which resulted later in a very difficult to find
bugs. For this very reason, IMHO, I'd rather avoid risking this happening, but I
respect the Chrome's policy. Moreover, you may remember a bug I introduced by
not having DCHECKs enabled which is the default, at least in my git setup.

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/search_eng...
File chrome/browser/search_engines/prepopulated_engines.json (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/search_eng...
chrome/browser/search_engines/prepopulated_engines.json:29:
"kCurrentDataVersion": 47
On 2012/12/11 22:15:50, Peter Kasting wrote:
> On 2012/12/11 22:03:16, Bart N. wrote:
> > On 2012/12/11 21:23:20, Mark P wrote:
> > > Remember to resync and rebase your change and edit this number before
> > > submitting.  (Other people have already incremented it.)
> > > 
> > > *** rebasing might not report this as an error; remember to change this
> number
> > > yourself ***
> > Yup - I remembered this (we had very similar issues in our CS code).
However,
> I
> > was nicely surprised to actually see a conflict. The filed changed to 48
> > meanwhile, yet the merge was smart enough to detect it rather than thinking
> that
> > I simply wanted to decrease the number.
> 
> Merge will reject anything other than a server-side change that duplicates
your
> client-side change.  The worrisome case is when someone else's CL changes this
> to 47 :)
Ah yes, it makes sense. Thanks.

[commit-bot: I haz the power, 2012-12-11 23:33:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartn@chromium.org/11414303/43001

[commit-bot: I haz the power, 2012-12-11 23:33:45]

Presubmit check for 11414303-43001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/extensions/api/omnibox

Presubmit checks took 2.9s to calculate.

[Bart N., 2012-12-11 23:59:07]

Matt, can you stamp api/omnibox change, please?

[Matt Perry, 2012-12-12 00:06:21]

chrome/browser/extensions/api/omnibox/omnibox_apitest.cc LGTM

[Peter Kasting, 2012-12-12 00:24:11]

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:54: if (cursor_position !=
string16::npos && cursor_position >= text.length()) {
On 2012/12/11 22:34:15, Bart N. wrote:
> You may remember your comment regarding selection and views implementation (on
> this thread). Just by seeing the buggy behavior in views_views.cc (start/end
are
> reversed) and your remark regarding the code quality and lack of unit tests, I
> really have no trust regarding the various view implementations (another
example
> is Linux view behavior).

If we have bugs, we need to fix them.  Especially since omnibox_view_win.cc is
going to disappear and omnibox_view_views.cc is going to be the implementation
across Windows, Linux, and ChromeOS.

DCHECKing conditions that ought to hold here is a way to find these bugs.  The
Canary/Dev/Beta channel process is designed to ensure that we can check in
things like this if we are pretty sure they are correct, and nail any
outstanding issues we missed.

> There is nothing wrong with this, because not
> only is this extra check cheap, but it's also conceptually in the right place,
> because below we are going to normalize the cursor position anyway. The logic
is
> kept in one place and is easy to understand.

It's easy to understand what the code is doing.  What's misleading is what it
implies about the callers.  This is the same as a NULL-check on a variable that
can't be NULL.  If the situation shouldn't be able to happen, we need to make
that clear.

I know you will be unhappy at this, but I am simply not OK with checking this
code in as-is.  This code really should be

  DCHECK((cursor_position <= text.length()) ||
      (cursor_position == string16::npos));

We should also make UpdateText() DCHECK the same thing.

I just audited all the callsites for these.  Other than your new unit test, no
one should violate this condition.  Indeed, if anyone did, it would have to be
because GetSelectionBounds() returned a position outside the string.  If this
were to happen then all kinds of sites all over the omnibox code would behave
very badly (probably by crashing the browser).  Even were the start/end values
not properly directional (which I thought you said turned out not to be
happening after all in omnibox_view_views.cc?), that could not cause such a bug.

> Moreover, you may remember a bug I introduced by
> not having DCHECKs enabled which is the default, at least in my git setup.

That is not a good thing.  All developer checkouts are supposed to enable
DCHECKs when you build in Debug mode.  If this isn't true for you, please file a
bug against maruel.

[Bart N., 2012-12-12 02:16:35]

I'll ping back this thread when I make the changes you asked for.

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_input.cc (right):

https://codereview.chromium.org/11414303/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_input.cc:54: if (cursor_position !=
string16::npos && cursor_position >= text.length()) {
On 2012/12/12 00:24:11, Peter Kasting wrote:
> On 2012/12/11 22:34:15, Bart N. wrote:
> > You may remember your comment regarding selection and views implementation
(on
> > this thread). Just by seeing the buggy behavior in views_views.cc (start/end
> are
> > reversed) and your remark regarding the code quality and lack of unit tests,
I
> > really have no trust regarding the various view implementations (another
> example
> > is Linux view behavior).
> 
> If we have bugs, we need to fix them.  Especially since omnibox_view_win.cc is
> going to disappear and omnibox_view_views.cc is going to be the implementation
> across Windows, Linux, and ChromeOS.
> 
> DCHECKing conditions that ought to hold here is a way to find these bugs.  The
> Canary/Dev/Beta channel process is designed to ensure that we can check in
> things like this if we are pretty sure they are correct, and nail any
> outstanding issues we missed.
Are Canary/Dev/Beta channel binaries built with DCHECK on?
> 
> > There is nothing wrong with this, because not
> > only is this extra check cheap, but it's also conceptually in the right
place,
> > because below we are going to normalize the cursor position anyway. The
logic
> is
> > kept in one place and is easy to understand.
> 
> It's easy to understand what the code is doing.  What's misleading is what it
> implies about the callers.  This is the same as a NULL-check on a variable
that
> can't be NULL.  If the situation shouldn't be able to happen, we need to make
> that clear.
> 
> I know you will be unhappy at this, but I am simply not OK with checking this
> code in as-is.  This code really should be
> 
>   DCHECK((cursor_position <= text.length()) ||
>       (cursor_position == string16::npos));
> 
> We should also make UpdateText() DCHECK the same thing.
I'll make the changes before I submit this CL and ping you back one more time in
case you want to take a look.
> 
> I just audited all the callsites for these.  Other than your new unit test, no
> one should violate this condition.  Indeed, if anyone did, it would have to be
> because GetSelectionBounds() returned a position outside the string.  If this
> were to happen then all kinds of sites all over the omnibox code would behave
> very badly (probably by crashing the browser).  Even were the start/end values
> not properly directional (which I thought you said turned out not to be
> happening after all in omnibox_view_views.cc?), that could not cause such a
bug.
> 
> > Moreover, you may remember a bug I introduced by
> > not having DCHECKs enabled which is the default, at least in my git setup.
> 
> That is not a good thing.  All developer checkouts are supposed to enable
> DCHECKs when you build in Debug mode.  If this isn't true for you, please file
a
> bug against maruel.
I build in Release mode as I found Debug mode extremely slow for the
development, at least on my Linux box.

[Peter Kasting, 2012-12-12 02:37:38]

On 2012/12/12 02:16:35, Bart N. wrote:
> Are Canary/Dev/Beta channel binaries built with DCHECK on?

Canary might be, but that's not precisely what I meant; I more meant that things
that would cause bugs if the DCHECKs were wrong would hopefully show up.

> > > Moreover, you may remember a bug I introduced by
> > > not having DCHECKs enabled which is the default, at least in my git setup.
> > 
> > That is not a good thing.  All developer checkouts are supposed to enable
> > DCHECKs when you build in Debug mode.  If this isn't true for you, please
file
> a
> > bug against maruel.
> I build in Release mode as I found Debug mode extremely slow for the
> development, at least on my Linux box.

I can't save you from not seeing DCHECK failures if you don't build in Debug
mode :)

[Bart N., 2012-12-12 02:41:01]

On 2012/12/12 02:37:38, Peter Kasting wrote:
> On 2012/12/12 02:16:35, Bart N. wrote:
> > Are Canary/Dev/Beta channel binaries built with DCHECK on?
> 
> Canary might be, but that's not precisely what I meant; I more meant that
things
> that would cause bugs if the DCHECKs were wrong would hopefully show up.
Let's just not discuss DCHECKs anymore; you are not going to convince me :) feel
free to stop by, we can chat with a SRE on why DCHECKs are bad.
> 
> > > > Moreover, you may remember a bug I introduced by
> > > > not having DCHECKs enabled which is the default, at least in my git
setup.
> > > 
> > > That is not a good thing.  All developer checkouts are supposed to enable
> > > DCHECKs when you build in Debug mode.  If this isn't true for you, please
> file
> > a
> > > bug against maruel.
> > I build in Release mode as I found Debug mode extremely slow for the
> > development, at least on my Linux box.
> 
> I can't save you from not seeing DCHECK failures if you don't build in Debug
> mode :)
If you think about it, it makes more sense to test in Release mode; you want
experience of millions, not a few developers :) I hope someone is testing in the
release mode.

[Peter Kasting, 2012-12-12 02:43:11]

Almost all our testing happens in release mode.  We get plenty of testing there
:)

[Bart N., 2012-12-12 17:35:02]

I've added the DCHECKs.

[Bart N., 2012-12-12 21:03:28]

On 2012/12/12 17:35:02, Bart N. wrote:
> I've added the DCHECKs.
Quick update. After adding DCHECKs and removing normalization, there is one test
failing:  OmniboxViewTest.CtrlKeyPressedWithInlineAutocompleteTest. It fails on
mac/win only.  After some investigation, I found out that this is happening
because of different behavior of GetSelectionBounds() between Linux and Mac/Win.
On Linux, selection is reverted (start > end) while in case of Mac/Win start <
end.

We discussed this very issue at the beginning of this thread and found out one
potential bug in omnibox_view_views.cc.

Stay tuned.

[Bart N., 2012-12-12 21:10:25]

On 2012/12/12 21:03:28, Bart N. wrote:
> On 2012/12/12 17:35:02, Bart N. wrote:
> > I've added the DCHECKs.
> Quick update. After adding DCHECKs and removing normalization, there is one
test
> failing:  OmniboxViewTest.CtrlKeyPressedWithInlineAutocompleteTest. It fails
on
> mac/win only.  After some investigation, I found out that this is happening
> because of different behavior of GetSelectionBounds() between Linux and
Mac/Win.
> On Linux, selection is reverted (start > end) while in case of Mac/Win start <
> end.
> 
> We discussed this very issue at the beginning of this thread and found out one
> potential bug in omnibox_view_views.cc.
> 
> Stay tuned.
BTW, there are two issues here:
(a) different selection direction between Linux and Mac/Win
(b) For inlineable suggestions, GetSelectionBound returns selection for the
completed text. This is actually a real bug, because we want to send cursor
after user_text, not including inline_autocomplete_text_.

(a) is practically not important, as I explained in one of my early comments
(when editing selection is always empty).

[Mark P, 2012-12-12 21:17:23]

On Wed, Dec 12, 2012 at 1:03 PM, <bartn@chromium.org> wrote:

> Quick update. After adding DCHECKs and removing normalization, there is
> one test
> failing:  OmniboxViewTest.**CtrlKeyPressedWithInlineAutoco**mpleteTest.
> It fails on
> mac/win only.
>

For clarity, did you actually
force-test CtrlKeyPressedWithInlineAutocompleteTest and DesiredTLD on Linux?

By default they're disabled on Linux because they're thought to be flaky.
 Are you saying that nowadays they pass on Linux (and pass consistently)?

--mark


> After some investigation, I found out that this is happening
> because of different behavior of GetSelectionBounds() between Linux and
> Mac/Win.
> On Linux, selection is reverted (start > end) while in case of Mac/Win
> start <
> end.
>

> We discussed this very issue at the beginning of this thread and found out
> one
> potential bug in omnibox_view_views.cc.
>
> Stay tuned.
>

[Bart N., 2012-12-12 21:18:36]

> (b) For inlineable suggestions, GetSelectionBound returns selection for the
> completed text. This is actually a real bug, because we want to send cursor
> after user_text, not including inline_autocomplete_text_.
This is how to reproduce (b):
- Type a prefix for which there is an inlineable completions. At this point,
although the text is inlined in the box, the selection is empty
(start=end=cursor_position).
- Press "Ctrl" - the selection bounds change to include the inlined text.
- Releasing "Ctrl" leaves the selection unchanged.

One example query prefix is "go" (should be inlined to google.com).

[Bart N., 2012-12-12 21:21:37]

On 2012/12/12 21:17:23, Mark P wrote:
> On Wed, Dec 12, 2012 at 1:03 PM, <mailto:bartn@chromium.org> wrote:
> 
> > Quick update. After adding DCHECKs and removing normalization, there is
> > one test
> > failing:  OmniboxViewTest.**CtrlKeyPressedWithInlineAutoco**mpleteTest.
> > It fails on
> > mac/win only.
> >
> 
> For clarity, did you actually
> force-test CtrlKeyPressedWithInlineAutocompleteTest and DesiredTLD on Linux?
I removed "DISABLED_" prefix from CtrlKeyPressedWithInlineAutocompleteTest and
recompiled.
> 
> By default they're disabled on Linux because they're thought to be flaky.
>  Are you saying that nowadays they pass on Linux (and pass consistently)?
CtrlKeyPressedWithInlineAutocompleteTest passes consistently, at least on my
machine. Not sure about the other one. Perhaps worth revisiting this in a
separate CL (perhaps by adding MAYBE_, similar to other OSes).
> 
> --mark
> 
> 
> > After some investigation, I found out that this is happening
> > because of different behavior of GetSelectionBounds() between Linux and
> > Mac/Win.
> > On Linux, selection is reverted (start > end) while in case of Mac/Win
> > start <
> > end.
> >
> 
> > We discussed this very issue at the beginning of this thread and found out
> > one
> > potential bug in omnibox_view_views.cc.
> >
> > Stay tuned.
> >

[Bart N., 2012-12-12 21:25:48]

On 2012/12/12 21:18:36, Bart N. wrote:
> > (b) For inlineable suggestions, GetSelectionBound returns selection for the
> > completed text. This is actually a real bug, because we want to send cursor
> > after user_text, not including inline_autocomplete_text_.
> This is how to reproduce (b):
> - Type a prefix for which there is an inlineable completions. At this point,
> although the text is inlined in the box, the selection is empty
> (start=end=cursor_position).
> - Press "Ctrl" - the selection bounds change to include the inlined text.
> - Releasing "Ctrl" leaves the selection unchanged.
> 
> One example query prefix is "go" (should be inlined to http://google.com).
BTW, the reason why CtrlKeyPressedWithInlineAutocompleteTest passes on Linux is
trivial. Because of (a) which hides (b) - the selection is reverted and
coincidentally "selection end" happens to be the end of text, so DCHECK passes
:)
Consider this example:
go -> google.com (selection boundaries on Linux: start=6 end=2)

[Bart N., 2012-12-13 00:53:04]

Status Update:

The invariant put in DCHECK is not true in case where user has inline
autocomplete text and presses Ctrl key.

I chatted with Mark and we discovered at least one more corner case, not
currently handled. For instance, we may get inconsistent behavior when pressing
Ctrl key if user selects different parts of the inout text.

We haven't decided yet what would be the best fix and are considering a few
options (details omitted).

[Bart N., 2012-12-13 18:25:32]

> We haven't decided yet what would be the best fix and are considering a few
> options (details omitted).
Mark, I've implemented the safest approach based on our discussion and filed
http://crbug/165961 and http://crbug/165968 to track down the remaining issues.

Peter, since you are OOO, I'm going to go with a minor change which fixes the
failing unit test. Once you are back, let's agree on a better fix.

PTAL

[Bart N., 2012-12-13 18:26:20]

On 2012/12/13 18:25:32, Bart N. wrote:
> > We haven't decided yet what would be the best fix and are considering a few
> > options (details omitted).
> Mark, I've implemented the safest approach based on our discussion and filed
Clarification: based on the offline discussion with Mark.

[Mark P, 2012-12-14 01:25:34]

As I said earlier today, please feel free to land this whenever.

LGTM

[Bart N., 2012-12-14 02:29:34]

On 2012/12/14 01:25:34, Mark P wrote:
> As I said earlier today, please feel free to land this whenever.
> 
> LGTM
I've synced to head (revision 172738) and doubled checked that this revision did
not overwrite  my version in JSON (data version pre this change is still 48).

[commit-bot: I haz the power, 2012-12-14 02:31:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartn@chromium.org/11414303/57005

[Bart N., 2012-12-14 03:10:25]

On 2012/12/14 02:31:01, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/bartn%40chromium.org/11414303/57005

Looks like I missed
chrome/browser/autocomplete/contact_provider_chromeos_unittest.cc...

[commit-bot: I haz the power, 2012-12-14 03:24:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-14 03:25:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartn@chromium.org/11414303/66006

[commit-bot: I haz the power, 2012-12-14 09:57:18]

Change committed as 173131

[Peter Kasting, 2012-12-18 03:01:01]

You said "when editing selection is always empty".  But what if I actually
select something?  Isn't that a nonempty selection?  Sorry if I paged out too
much context of this issue since last week :(

https://chromiumcodereview.appspot.com/11414303/diff/66006/chrome/browser/sea...
File chrome/browser/search_engines/template_url.h (right):

https://chromiumcodereview.appspot.com/11414303/diff/66006/chrome/browser/sea...
chrome/browser/search_engines/template_url.h:76: // last character.
I thought "after the last character" was allowed now?

https://chromiumcodereview.appspot.com/11414303/diff/66006/chrome/browser/ui/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://chromiumcodereview.appspot.com/11414303/diff/66006/chrome/browser/ui/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:469: // position past the last
character from the user text.  Instead,
In theory, inline autocompletions are supposed to be reverse-selected, so you'd
still get the selection "end" at the end of the user's typing.  (That's true on
Windows for sure, I haven't tested the other platforms.)  It sounds like maybe
you found somewhere that isn't the case?

[Bart N., 2012-12-18 23:44:08]

On 2012/12/18 03:01:01, Peter Kasting wrote:
> You said "when editing selection is always empty".  But what if I actually
> select something?  Isn't that a nonempty selection?  Sorry if I paged out too
> much context of this issue since last week :(
Moving cursor and changing selection does not run AC. In order to trigger it,
you need to press a key, which removes selection. Do you know how to trigger AC
with selection other than highlighted inline autocompletion? (excluding pressing
Ctrl of course).

[Bart N., 2012-12-18 23:47:39]

The fixes are in https://codereview.chromium.org/11636005.

https://chromiumcodereview.appspot.com/11414303/diff/66006/chrome/browser/sea...
File chrome/browser/search_engines/template_url.h (right):

https://chromiumcodereview.appspot.com/11414303/diff/66006/chrome/browser/sea...
chrome/browser/search_engines/template_url.h:76: // last character.
On 2012/12/18 03:01:01, Peter Kasting wrote:
> I thought "after the last character" was allowed now?
You mean "wasn't", right? I forgot to fix it here. Done.

https://chromiumcodereview.appspot.com/11414303/diff/66006/chrome/browser/ui/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://chromiumcodereview.appspot.com/11414303/diff/66006/chrome/browser/ui/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:469: // position past the last
character from the user text.  Instead,
On 2012/12/18 03:01:01, Peter Kasting wrote:
> In theory, inline autocompletions are supposed to be reverse-selected, so
you'd
> still get the selection "end" at the end of the user's typing.  (That's true
on
> Windows for sure, I haven't tested the other platforms.)  It sounds like maybe
> you found somewhere that isn't the case?
Yes. See http://crbug.com/165961. I've updated the comment.

[Peter Kasting, 2012-12-18 23:50:18]

On 2012/12/18 23:44:08, Bart N. wrote:
> Moving cursor and changing selection does not run AC. In order to trigger it,
> you need to press a key, which removes selection. Do you know how to trigger
AC
> with selection other than highlighted inline autocompletion? (excluding
pressing
> Ctrl of course).

I don't think normally you can trigger it with a highlighted inline
autocompletion either -- as to trigger you need to hit delete or type a new
character, both of which ought to replace the inline autocompletion.

However, you can run with an arbitrary selection as follows:
* Don't have Instant Extended on.
* Ensure the dropdown is closed.
* Select some text in the omnibox.
* Hit the down arrow key to open the dropdown.

[Bart N., 2012-12-19 00:02:51]

On 2012/12/18 23:50:18, Peter Kasting wrote:
> On 2012/12/18 23:44:08, Bart N. wrote:
> > Moving cursor and changing selection does not run AC. In order to trigger
it,
> > you need to press a key, which removes selection. Do you know how to trigger
> AC
> > with selection other than highlighted inline autocompletion? (excluding
> pressing
> > Ctrl of course).
> 
> I don't think normally you can trigger it with a highlighted inline
> autocompletion either -- as to trigger you need to hit delete or type a new
I omitted the highlighted inline case since I worked around it already in this
CL (and opened 2 bugs). I was discussing other corner cases which might possibly
affect the behavior.
> character, both of which ought to replace the inline autocompletion.
> 
> However, you can run with an arbitrary selection as follows:
> * Don't have Instant Extended on.
> * Ensure the dropdown is closed.
Is it possible to close it once it's open? I'm having trouble executing this
step :)
> * Select some text in the omnibox.
> * Hit the down arrow key to open the dropdown.
In this case, you are absolutely right. The cursor position will be OS dependent
:(

[Peter Kasting, 2012-12-19 00:05:31]

On 2012/12/19 00:02:51, Bart N. wrote:
> I omitted the highlighted inline case since I worked around it already in this
> CL (and opened 2 bugs). I was discussing other corner cases which might
possibly
> affect the behavior.

OK.  I'm just still confused how you'd actually still see an inline
autocompletion at the time you called Start().

> > However, you can run with an arbitrary selection as follows:
> > * Don't have Instant Extended on.
> > * Ensure the dropdown is closed.
> Is it possible to close it once it's open? I'm having trouble executing this
> step :)

Hit escape, or switch tabs and switch back, or just don't type anything
different in the omnibox to begin with (you can just select some of the
permanent text).

[Bart N., 2012-12-19 00:20:36]

On 2012/12/19 00:05:31, Peter Kasting wrote:
> On 2012/12/19 00:02:51, Bart N. wrote:
> > I omitted the highlighted inline case since I worked around it already in
this
> > CL (and opened 2 bugs). I was discussing other corner cases which might
> possibly
> > affect the behavior.
> 
> OK.  I'm just still confused how you'd actually still see an inline
> autocompletion at the time you called Start().
You can see it if you hit "Ctrl", because it is not cleared in
OmniboxEditModel::OnControlKeyChanged.
> 
> > > However, you can run with an arbitrary selection as follows:
> > > * Don't have Instant Extended on.
> > > * Ensure the dropdown is closed.
> > Is it possible to close it once it's open? I'm having trouble executing this
> > step :)
> 
> Hit escape, or switch tabs and switch back, or just don't type anything
Hitting escape removes all text from Omnibox for me :( anyway, this is not
really important - I trust you.
> different in the omnibox to begin with (you can just select some of the
> permanent text).