Omnibox: Change |inline_autocomplete_offset| to |inline_autocompletion|

Omnibox: Change |inline_autocomplete_offset| to |inline_autocompletion|

This makes things easier to understand.  No more messy indexing into
|fill_into_edit|.  No more wondering whether
inline_autocomplete_offset == fill_into_edit.length()
is equivalent to
inline_autocomplete_offset == string16::npos
(Answer: currently equivalent.)

Also, it's probably easier in the future to make distinctions between
|fill_into_edit| and the inline completion.  For instance, we may want
one to have a trailing slash and the other not to under certain conditions.
The previous code restricted our ability to do this.

BUG=
TBR=estade
( for chrome/browser/resources/omnibox/omnibox.js )

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

[Mark P, 2013-07-16 16:45:06]

Peter, is this worth doing?

--mark

[Peter Kasting, 2013-07-16 18:09:05]

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:250: // if this match is
displayed inline (i.e., within the omnibox).
Nit: Don't refer to "blue", that's a Windows-ism.  Also "displayed inline" is
confusing.  "Inline autocompletion" is a fairly ubiquitous concept in this code,
so I don't think it's necessary to explain it; how about:

The inline autocompletion to display after the user's typing in the omnibox, if
this match becomes the default match.  This may be empty.

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_quick_provider.cc:282: if
(inline_autocomplete_offset > match.fill_into_edit.length())
Nit: How about just:

    if (inline_autocomplete_offset < match.fill_into_edit.length()) {
      match.inline_autocompletion =
          match.fill_into_edit.substr(inline_autocomplete_offset);
    }

This will require tweaking the comment ("|inline_autocomplete_offset| may be
beyond the end of the |fill_into_edit| if...").

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:353: // autocompletion)
here, it's surprising and annoying.
Nit: How about:

NOTE: Don't set match.inline_autocompletion to something non-empty here, ...

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/keyword_provider.cc (left):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/keyword_provider.cc:453: // we just don't bother to
do this, and leave inline autocompletion off.
We should perhaps preserve the comment here about why we don't set
|inline_autocompletion|?

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/search_provider_unittest.cc:902: { "a", "c", "b",
kNotApplicable }, "" },
Nit: If we're constructing a string from this instead of a char[] or similar, I
generally try to stay in the habit of using std::string() as the initializer
instead of "".

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_controller.cc:77:
current_match_.fill_into_edit = omnibox_edit_model_->user_text();
The one change that deleting this block makes is that
current_match_.fill_into_edit is no longer set to the user text.  I don't think
I understand precisely why this code was around before, so I'm not sure whether
that's an improvement or a regression.  Maybe Philippe knows.

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1117: // popup handling has
completely migrated to omnibox_controller.
This TODO should perhaps be preserved?

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

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.h:273: const string16 text,
Removing the '&' here seems like a mistake

[Mark P, 2013-07-16 18:53:23]

Apparently your implicit answer is, yes, this is worth doing.

--mark

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/autocomplete_match.h:250: // if this match is
displayed inline (i.e., within the omnibox).
On 2013/07/16 18:09:05, Peter Kasting wrote:
> Nit: Don't refer to "blue", that's a Windows-ism.  Also "displayed inline" is
> confusing.  "Inline autocompletion" is a fairly ubiquitous concept in this
code,
> so I don't think it's necessary to explain it; how about:
> 
> The inline autocompletion to display after the user's typing in the omnibox,
if
> this match becomes the default match.  This may be empty.

Okay.  Done.

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_quick_provider.cc:282: if
(inline_autocomplete_offset > match.fill_into_edit.length())
On 2013/07/16 18:09:05, Peter Kasting wrote:
> Nit: How about just:
> 
>     if (inline_autocomplete_offset < match.fill_into_edit.length()) {
>       match.inline_autocompletion =
>           match.fill_into_edit.substr(inline_autocomplete_offset);
>     }

Good idea.  Don't know how I missed seeing that simplification.

> This will require tweaking the comment ("|inline_autocomplete_offset| may be
> beyond the end of the |fill_into_edit| if...").

Revised.

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:353: // autocompletion)
here, it's surprising and annoying.
On 2013/07/16 18:09:05, Peter Kasting wrote:
> Nit: How about:
> 
> NOTE: Don't set match.inline_autocompletion to something non-empty here, ...

Done.

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/keyword_provider.cc (left):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/keyword_provider.cc:453: // we just don't bother to
do this, and leave inline autocompletion off.
On 2013/07/16 18:09:05, Peter Kasting wrote:
> We should perhaps preserve the comment here about why we don't set
> |inline_autocompletion|?

Okay.

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/autocomplet...
chrome/browser/autocomplete/search_provider_unittest.cc:902: { "a", "c", "b",
kNotApplicable }, "" },
On 2013/07/16 18:09:05, Peter Kasting wrote:
> Nit: If we're constructing a string from this instead of a char[] or similar,
I
> generally try to stay in the habit of using std::string() as the initializer
> instead of "".

Done everywhere, though I think it looks a little silly going back and forth
between std::string() and "1" all the time.  "" vs "1" looks a lot saner.

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_controller.cc:77:
current_match_.fill_into_edit = omnibox_edit_model_->user_text();
On 2013/07/16 18:09:05, Peter Kasting wrote:
> The one change that deleting this block makes is that
> current_match_.fill_into_edit is no longer set to the user text.  I don't
think
> I understand precisely why this code was around before, so I'm not sure
whether
> that's an improvement or a regression.  Maybe Philippe knows.

I think I understand why this was here (and that it's okay to remove it), but
I'll add Philippe as a reviewer to make sure.

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1117: // popup handling has
completely migrated to omnibox_controller.
On 2013/07/16 18:09:05, Peter Kasting wrote:
> This TODO should perhaps be preserved?

Okay.  Done.

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

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.h:273: const string16 text,
On 2013/07/16 18:09:05, Peter Kasting wrote:
> Removing the '&' here seems like a mistake

This is critical.  If the & is there, it doesn't work.  It took me two hours to
debug this and figure out the & is the cause.

We pass match.inline_autocompletion into this function as |text|.  This function
resets match, which means it clears match.inline_autocompletion.  Hence,
everywhere later in the function that refers to |text| sees an empty string.

[Mark P, 2013-07-16 18:55:41]

Hi Philippe,

I'm working on a change that actually corresponds to a TODO for you in the code.
 Can you please take a look to see if it's like how you envisioned it.  Pay
particular attention to 
chrome/browser/ui/omnibox/omnibox_controller.cc
Am I right in removing all the code I did?  It's not entirely obvious if the
TODO means I could simplify the code as much as I did.

thanks,
mark

[Peter Kasting, 2013-07-16 18:59:55]

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

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.h:273: const string16 text,
On 2013/07/16 18:53:23, Mark P wrote:
> On 2013/07/16 18:09:05, Peter Kasting wrote:
> > Removing the '&' here seems like a mistake
> 
> This is critical.  If the & is there, it doesn't work.  It took me two hours
to
> debug this and figure out the & is the cause.
> 
> We pass match.inline_autocompletion into this function as |text|.  This
function
> resets match, which means it clears match.inline_autocompletion.  Hence,
> everywhere later in the function that refers to |text| sees an empty string.

Hmm.  In that case perhaps the affected caller(s) should copy the string before
calling the function, with a comment about why they're doing it.

I mean, there need to be comments regardless, but it's more efficient and
perhaps more clear to do this explicitly at the caller side in the affected
places rather than implicitly in the function call itself.

[Mark P, 2013-07-16 19:47:12]

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

https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.h:273: const string16 text,
On 2013/07/16 18:59:55, Peter Kasting wrote:
> On 2013/07/16 18:53:23, Mark P wrote:
> > On 2013/07/16 18:09:05, Peter Kasting wrote:
> > > Removing the '&' here seems like a mistake
> > 
> > This is critical.  If the & is there, it doesn't work.  It took me two hours
> to
> > debug this and figure out the & is the cause.
> > 
> > We pass match.inline_autocompletion into this function as |text|.  This
> function
> > resets match, which means it clears match.inline_autocompletion.  Hence,
> > everywhere later in the function that refers to |text| sees an empty string.
> 
> Hmm.  In that case perhaps the affected caller(s) should copy the string
before
> calling the function, with a comment about why they're doing it.

Are you sure you want me to do this?  It requires putting the ugly "make a copy"
code and comment at the location of three separate callers.

Wouldn't it be easier to add a comment here?
// We cannot take |text| or |keyword| as references because callers tends to get
these values from the current match and OnPopupDataChanged() resets the current
match.  If OnPopupDataChanged() took them as references, it would lose knowledge
of their original values.

> 
> I mean, there need to be comments regardless, but it's more efficient and
> perhaps more clear to do this explicitly at the caller side in the affected
> places rather than implicitly in the function call itself.

[Peter Kasting, 2013-07-16 19:49:58]

On 2013/07/16 19:47:12, Mark P wrote:
>
https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
> File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):
> 
>
https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
> chrome/browser/ui/omnibox/omnibox_edit_model.h:273: const string16 text,
> On 2013/07/16 18:59:55, Peter Kasting wrote:
> > On 2013/07/16 18:53:23, Mark P wrote:
> > > On 2013/07/16 18:09:05, Peter Kasting wrote:
> > > > Removing the '&' here seems like a mistake
> > > 
> > > This is critical.  If the & is there, it doesn't work.  It took me two
hours
> > to
> > > debug this and figure out the & is the cause.
> > > 
> > > We pass match.inline_autocompletion into this function as |text|.  This
> > function
> > > resets match, which means it clears match.inline_autocompletion.  Hence,
> > > everywhere later in the function that refers to |text| sees an empty
string.
> > 
> > Hmm.  In that case perhaps the affected caller(s) should copy the string
> before
> > calling the function, with a comment about why they're doing it.
> 
> Are you sure you want me to do this?  It requires putting the ugly "make a
copy"
> code and comment at the location of three separate callers.

Why 3?  I thought there was one place for sure and MAYBE a second one (but I was
not sure there).

The assumption was that most callers don't need to do the copy.

[beaudoin, 2013-07-16 19:53:12]

On 2013/07/16 19:49:58, Peter Kasting wrote:
> On 2013/07/16 19:47:12, Mark P wrote:
> >
>
https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
> > File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):
> > 
> >
>
https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/...
> > chrome/browser/ui/omnibox/omnibox_edit_model.h:273: const string16 text,
> > On 2013/07/16 18:59:55, Peter Kasting wrote:
> > > On 2013/07/16 18:53:23, Mark P wrote:
> > > > On 2013/07/16 18:09:05, Peter Kasting wrote:
> > > > > Removing the '&' here seems like a mistake
> > > > 
> > > > This is critical.  If the & is there, it doesn't work.  It took me two
> hours
> > > to
> > > > debug this and figure out the & is the cause.
> > > > 
> > > > We pass match.inline_autocompletion into this function as |text|.  This
> > > function
> > > > resets match, which means it clears match.inline_autocompletion.  Hence,
> > > > everywhere later in the function that refers to |text| sees an empty
> string.
> > > 
> > > Hmm.  In that case perhaps the affected caller(s) should copy the string
> > before
> > > calling the function, with a comment about why they're doing it.
> > 
> > Are you sure you want me to do this?  It requires putting the ugly "make a
> copy"
> > code and comment at the location of three separate callers.
> 
> Why 3?  I thought there was one place for sure and MAYBE a second one (but I
was
> not sure there).
> 
> The assumption was that most callers don't need to do the copy.

LGTM. Thanks for handling that Mark, feels much better this way.

[Peter Kasting, 2013-07-16 19:57:27]

On 2013/07/16 19:53:12, beaudoin wrote:
> LGTM. Thanks for handling that Mark, feels much better this way.

So why did we need to set the |fill_into_edit| to the current omnibox text in
the first place, if deleting that functionality doesn't cause problems?

[beaudoin, 2013-07-16 20:07:28]

You are right Peter, missed this.

https://codereview.chromium.org/19197005/diff/26002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/19197005/diff/26002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:77:
current_match_.fill_into_edit = omnibox_edit_model_->user_text();
In fact you need to reset the fill_into_edit with the omnibox user_text still,
because there is a possibility that the *match prefix differs from the omnibox
content (ie, swapping spaces for %20 IIRC). This is probably the only thing you
need to keep doing here, all the rest was to mangle the original prefix and the
inline autocompletion.

[Mark P, 2013-07-16 20:11:10]

On Tue, Jul 16, 2013 at 12:49 PM, <pkasting@chromium.org> wrote:

> On 2013/07/16 19:47:12, Mark P wrote:
>
> https://codereview.chromium.**org/19197005/diff/9001/chrome/**
>
browser/ui/omnibox/omnibox_**edit_model.h<https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/omnibox_edit_model.h>
>
>> File chrome/browser/ui/omnibox/**omnibox_edit_model.h (right):
>>
>
>
> https://codereview.chromium.**org/19197005/diff/9001/chrome/**
>
browser/ui/omnibox/omnibox_**edit_model.h#newcode273<https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/omnibox_edit_model.h#newcode273>
>
>> chrome/browser/ui/omnibox/**omnibox_edit_model.h:273: const string16
>> text,
>> On 2013/07/16 18:59:55, Peter Kasting wrote:
>> > On 2013/07/16 18:53:23, Mark P wrote:
>> > > On 2013/07/16 18:09:05, Peter Kasting wrote:
>> > > > Removing the '&' here seems like a mistake
>> > >
>> > > This is critical.  If the & is there, it doesn't work.  It took me two
>>
> hours
>
>> > to
>> > > debug this and figure out the & is the cause.
>> > >
>> > > We pass match.inline_autocompletion into this function as |text|.
>>  This
>> > function
>> > > resets match, which means it clears match.inline_autocompletion.
>>  Hence,
>> > > everywhere later in the function that refers to |text| sees an empty
>>
> string.
>
>> >
>> > Hmm.  In that case perhaps the affected caller(s) should copy the string
>> before
>> > calling the function, with a comment about why they're doing it.
>>
>
>  Are you sure you want me to do this?  It requires putting the ugly "make a
>>
> copy"
>
>> code and comment at the location of three separate callers.
>>
>
> Why 3?  I thought there was one place for sure and MAYBE a second one (but
> I was
> not sure there).
>
> The assumption was that most callers don't need to do the copy.
>

Peter,

Here are the callers.  The first three of them (after my changelist) use
variables within |match| that get passed as references.  Only the fourth is
correct as-is.  The third I don't change but think the current code is
likely a bug.

https://code.google.com/p/chromium/codesearch#search/&q=OnPopupDataChanged%5C...


>
https://codereview.chromium.**org/19197005/<https://codereview.chromium.org/1...
>

[Mark P, 2013-07-16 20:11:50]

https://codereview.chromium.org/19197005/diff/26002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (left):

https://codereview.chromium.org/19197005/diff/26002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:77:
current_match_.fill_into_edit = omnibox_edit_model_->user_text();
On 2013/07/16 20:07:28, beaudoin wrote:
> In fact you need to reset the fill_into_edit with the omnibox user_text still,
> because there is a possibility that the *match prefix differs from the omnibox
> content (ie, swapping spaces for %20 IIRC). This is probably the only thing
you
> need to keep doing here, all the rest was to mangle the original prefix and
the
> inline autocompletion.

Done.

[Peter Kasting, 2013-07-16 21:06:57]

On 2013/07/16 20:11:10, Mark P wrote:
> On Tue, Jul 16, 2013 at 12:49 PM, <mailto:pkasting@chromium.org> wrote:
> 
> > On 2013/07/16 19:47:12, Mark P wrote:
> >
> > https://codereview.chromium.**org/19197005/diff/9001/chrome/**
> >
>
browser/ui/omnibox/omnibox_**edit_model.h<https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/omnibox_edit_model.h>
> >
> >> File chrome/browser/ui/omnibox/**omnibox_edit_model.h (right):
> >>
> >
> >
> > https://codereview.chromium.**org/19197005/diff/9001/chrome/**
> >
>
browser/ui/omnibox/omnibox_**edit_model.h#newcode273<https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/omnibox_edit_model.h#newcode273>
> >
> >> chrome/browser/ui/omnibox/**omnibox_edit_model.h:273: const string16
> >> text,
> >> On 2013/07/16 18:59:55, Peter Kasting wrote:
> >> > On 2013/07/16 18:53:23, Mark P wrote:
> >> > > On 2013/07/16 18:09:05, Peter Kasting wrote:
> >> > > > Removing the '&' here seems like a mistake
> >> > >
> >> > > This is critical.  If the & is there, it doesn't work.  It took me two
> >>
> > hours
> >
> >> > to
> >> > > debug this and figure out the & is the cause.
> >> > >
> >> > > We pass match.inline_autocompletion into this function as |text|.
> >>  This
> >> > function
> >> > > resets match, which means it clears match.inline_autocompletion.
> >>  Hence,
> >> > > everywhere later in the function that refers to |text| sees an empty
> >>
> > string.
> >
> >> >
> >> > Hmm.  In that case perhaps the affected caller(s) should copy the string
> >> before
> >> > calling the function, with a comment about why they're doing it.
> >>
> >
> >  Are you sure you want me to do this?  It requires putting the ugly "make a
> >>
> > copy"
> >
> >> code and comment at the location of three separate callers.
> >>
> >
> > Why 3?  I thought there was one place for sure and MAYBE a second one (but
> > I was
> > not sure there).
> >
> > The assumption was that most callers don't need to do the copy.
> >
> 
> Peter,
> 
> Here are the callers.  The first three of them (after my changelist) use
> variables within |match| that get passed as references.  Only the fourth is
> correct as-is.  The third I don't change but think the current code is
> likely a bug.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=OnPopupDataChanged%25...

I think only the first of these four is affected.

The issue is OnPopupDataChanged() calling
OmniboxController::InvalidateCurrentMatch(), right?  Only the first caller is
actually referencing fields on OmniboxController::current_match_.  The second
and third callers are referencing fields on one of the matches on
AutocompleteController::result_, which isn't affected by the aforementioned
function call.

[Mark P, 2013-07-16 21:39:52]

On Tue, Jul 16, 2013 at 2:06 PM, <pkasting@chromium.org> wrote:

> On 2013/07/16 20:11:10, Mark P wrote:
>
>  On Tue, Jul 16, 2013 at 12:49 PM, <mailto:pkasting@chromium.org> wrote:
>>
>
>  > On 2013/07/16 19:47:12, Mark P wrote:
>> >
>> > https://codereview.chromium.****org/19197005/diff/9001/chrome/****
>> >
>>
>
> browser/ui/omnibox/omnibox_****edit_model.h<https://**
> codereview.chromium.org/**19197005/diff/9001/chrome/**
>
browser/ui/omnibox/omnibox_**edit_model.h<https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/omnibox_edit_model.h>
> >
>
>> >
>> >> File chrome/browser/ui/omnibox/****omnibox_edit_model.h (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.****org/19197005/diff/9001/chrome/****
>> >
>>
>
> browser/ui/omnibox/omnibox_****edit_model.h#newcode273<https:**
> //codereview.chromium.org/**19197005/diff/9001/chrome/**
>
browser/ui/omnibox/omnibox_**edit_model.h#newcode273<https://codereview.chromium.org/19197005/diff/9001/chrome/browser/ui/omnibox/omnibox_edit_model.h#newcode273>
> >
>
>> >
>> >> chrome/browser/ui/omnibox/****omnibox_edit_model.h:273: const string16
>>
>> >> text,
>> >> On 2013/07/16 18:59:55, Peter Kasting wrote:
>> >> > On 2013/07/16 18:53:23, Mark P wrote:
>> >> > > On 2013/07/16 18:09:05, Peter Kasting wrote:
>> >> > > > Removing the '&' here seems like a mistake
>> >> > >
>> >> > > This is critical.  If the & is there, it doesn't work.  It took me
>> two
>> >>
>> > hours
>> >
>> >> > to
>> >> > > debug this and figure out the & is the cause.
>> >> > >
>> >> > > We pass match.inline_autocompletion into this function as |text|.
>> >>  This
>> >> > function
>> >> > > resets match, which means it clears match.inline_autocompletion.
>> >>  Hence,
>> >> > > everywhere later in the function that refers to |text| sees an
>> empty
>> >>
>> > string.
>> >
>> >> >
>> >> > Hmm.  In that case perhaps the affected caller(s) should copy the
>> string
>> >> before
>> >> > calling the function, with a comment about why they're doing it.
>> >>
>> >
>> >  Are you sure you want me to do this?  It requires putting the ugly
>> "make a
>> >>
>> > copy"
>> >
>> >> code and comment at the location of three separate callers.
>> >>
>> >
>> > Why 3?  I thought there was one place for sure and MAYBE a second one
>> (but
>> > I was
>> > not sure there).
>> >
>> > The assumption was that most callers don't need to do the copy.
>> >
>>
>
>  Peter,
>>
>
>  Here are the callers.  The first three of them (after my changelist) use
>> variables within |match| that get passed as references.  Only the fourth
>> is
>> correct as-is.  The third I don't change but think the current code is
>> likely a bug.
>>
>
>
> https://code.google.com/p/**chromium/codesearch#search/&q=**
> OnPopupDataChanged%255C%28%**2520-file:test%2520file:cc%24&**
>
sq=package:chromium&type=cs<https://code.google.com/p/chromium/codesearch#search/&q=OnPopupDataChanged%255C%28%2520-file:test%2520file:cc%24&sq=package:chromium&type=cs>
>
> I think only the first of these four is affected.
>
> The issue is OnPopupDataChanged() calling
> OmniboxController::**InvalidateCurrentMatch(), right?  Only the first
> caller is
> actually referencing fields on OmniboxController::current_**match_.  The
> second
> and third callers are referencing fields on one of the matches on
> AutocompleteController::**result_, which isn't affected by the
> aforementioned
> function call.
>

Good point.  Done.

Please take another look.

thanks,
mark


>
>
https://codereview.chromium.**org/19197005/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2013-07-16 21:48:50]

Question for Philippe below.

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:75: // we need to replace it.
I'm still confused by all this (sorry).  Why is it a bad thing if the
fill_into_edit doesn't quite match the user's typing?  If the only thing we use
the fill_into_edit for is showing something when the user arrows down and back
(or similar), there's no reason it has to exactly match the user's typing.

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1115: // the
inline_autocompletion originally was.
Nit: Typo + confusing, how about:

OnPopupDataChanged() resets OmniboxController's |current_match_| early on. 
Therefore, copy match.inline_autocompletion to a temp to preserve its value
across the entire call.

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1116: const string16
inline_autocompletion = match.inline_autocompletion;
Tiny nit: I slightly prefer constructor-style init for non-POD types.

[Mark P, 2013-07-16 23:34:08]

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1115: // the
inline_autocompletion originally was.
On 2013/07/16 21:48:50, Peter Kasting wrote:
> Nit: Typo + confusing, how about:
> 
> OnPopupDataChanged() resets OmniboxController's |current_match_| early on. 
> Therefore, copy match.inline_autocompletion to a temp to preserve its value
> across the entire call.

Done.

(You're a good editor!)

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1116: const string16
inline_autocompletion = match.inline_autocompletion;
On 2013/07/16 21:48:50, Peter Kasting wrote:
> Tiny nit: I slightly prefer constructor-style init for non-POD types.

Done.

[Mark P, 2013-07-17 20:48:08]

Philippe,

Can you please respond to Peter's  comment about why there's a need to keep line
setting fill_into_edit given the new approach.

For context, I tracked down the changelist in which you added this line.
https://chromiumcodereview.appspot.com/14698028
Click on the "Expand All Messages" link and then search the page for
"current_match_.fill_into_edit" to see the discussion you and Peter had about it
at the time.

I'm uncomfortable submitting this until this issue is settled.

thanks,
mark

[beaudoin1, 2013-07-17 21:33:52]

Mark, i will look at it tonight.
On Jul 17, 2013 4:48 PM, <mpearson@chromium.org> wrote:

>
> Philippe,
>
> Can you please respond to Peter's  comment about why there's a need to
> keep line
> setting fill_into_edit given the new approach.
>
> For context, I tracked down the changelist in which you added this line.
>
https://chromiumcodereview.**appspot.com/14698028<https://chromiumcodereview....
> Click on the "Expand All Messages" link and then search the page for
> "current_match_.fill_into_**edit" to see the discussion you and Peter had
> about it
> at the time.
>
> I'm uncomfortable submitting this until this issue is settled.
>
> thanks,
> mark
>
>
>
https://codereview.chromium.**org/19197005/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2013-07-17 22:07:10]

I wonder if we needed this because we were going to send the fill_into_edit to
some instant-related consumer?

It certainly seems like we shouldn't need it now, but yeah, good call on finding
that conversation :)

[Mark P, 2013-07-17 22:56:40]

Okay, removed that line and its associated comment.

Peter, are you ready for me to submit this now?

--mark

On Wed, Jul 17, 2013 at 3:07 PM, <pkasting@chromium.org> wrote:

> I wonder if we needed this because we were going to send the
> fill_into_edit to
> some instant-related consumer?
>
> It certainly seems like we shouldn't need it now, but yeah, good call on
> finding
> that conversation :)
>
>
https://codereview.chromium.**org/19197005/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2013-07-17 23:05:49]

On 2013/07/17 22:56:40, Mark P wrote:
> Okay, removed that line and its associated comment.

Uh, you should still wait for Philippe's analysis.

LGTM from me.

[beaudoin, 2013-07-18 17:50:18]

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_controller.cc (right):

https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_controller.cc:75: // we need to replace it.
On 2013/07/16 21:48:50, Peter Kasting wrote:
> I'm still confused by all this (sorry).  Why is it a bad thing if the
> fill_into_edit doesn't quite match the user's typing?  If the only thing we
use
> the fill_into_edit for is showing something when the user arrows down and back
> (or similar), there's no reason it has to exactly match the user's typing.

omnibox_edit_model.cc uses current_match() quite a bit so I have not completed
an extensive analysis but, IIRC, the problem was that it looked weird if you
arrowed down through the suggestions and hit ESC or arrowed back up. It was a
bit hard to trigger since the prefix almost always match. Maybe it can be
simulated by deliberately messing up with current_match_.fill_into_edit here and
see if any visible adverse effect happen as the user edit, arrows, hits esc,
etc.

[Peter Kasting, 2013-07-18 17:54:44]

On 2013/07/18 17:50:18, beaudoin wrote:
>
https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
> File chrome/browser/ui/omnibox/omnibox_controller.cc (right):
> 
>
https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
> chrome/browser/ui/omnibox/omnibox_controller.cc:75: // we need to replace it.
> On 2013/07/16 21:48:50, Peter Kasting wrote:
> > I'm still confused by all this (sorry).  Why is it a bad thing if the
> > fill_into_edit doesn't quite match the user's typing?  If the only thing we
> use
> > the fill_into_edit for is showing something when the user arrows down and
back
> > (or similar), there's no reason it has to exactly match the user's typing.
> 
> omnibox_edit_model.cc uses current_match() quite a bit so I have not completed
> an extensive analysis but, IIRC, the problem was that it looked weird if you
> arrowed down through the suggestions and hit ESC or arrowed back up. It was a
> bit hard to trigger since the prefix almost always match. Maybe it can be
> simulated by deliberately messing up with current_match_.fill_into_edit here
and
> see if any visible adverse effect happen as the user edit, arrows, hits esc,
> etc.

If you arrow down and then back up, it is expected (and desirable) that you may
see a different string than your typing.

If you arrow down and then hit escape, we shouldn't be showing the
fill_into_edit, so you should see your exact typing again.

[beaudoin, 2013-07-18 18:15:37]

On 2013/07/18 17:54:44, Peter Kasting wrote:
> On 2013/07/18 17:50:18, beaudoin wrote:
> >
>
https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
> > File chrome/browser/ui/omnibox/omnibox_controller.cc (right):
> > 
> >
>
https://codereview.chromium.org/19197005/diff/37001/chrome/browser/ui/omnibox...
> > chrome/browser/ui/omnibox/omnibox_controller.cc:75: // we need to replace
it.
> > On 2013/07/16 21:48:50, Peter Kasting wrote:
> > > I'm still confused by all this (sorry).  Why is it a bad thing if the
> > > fill_into_edit doesn't quite match the user's typing?  If the only thing
we
> > use
> > > the fill_into_edit for is showing something when the user arrows down and
> back
> > > (or similar), there's no reason it has to exactly match the user's typing.
> > 
> > omnibox_edit_model.cc uses current_match() quite a bit so I have not
completed
> > an extensive analysis but, IIRC, the problem was that it looked weird if you
> > arrowed down through the suggestions and hit ESC or arrowed back up. It was
a
> > bit hard to trigger since the prefix almost always match. Maybe it can be
> > simulated by deliberately messing up with current_match_.fill_into_edit here
> and
> > see if any visible adverse effect happen as the user edit, arrows, hits esc,
> > etc.
> 
> If you arrow down and then back up, it is expected (and desirable) that you
may
> see a different string than your typing.
> 
> If you arrow down and then hit escape, we shouldn't be showing the
> fill_into_edit, so you should see your exact typing again.

I am pretty sure we're showing fill_into_edit on an ESC. I manually tracked the
calls up to OmniboxPopupModel::SetSelectedLine that sets the line back to 0, I
have not found any other place where the typed text would be saved.

The idea of current_match_ was precisely to keep the full state of what was
being shown in the omnibox instead of tracking it with booleans and temporary
strings in various other places. This was the design Sreeram and I came to for a
number of reasons, mostly because the instant state management was out of hands.
We should probably bring him back into the discussion if you want to dig deeper
in this. Happy to discuss over VC if you think it's worthwhile.

For reference, this is the CL where current_match_ went in:
  https://codereview.chromium.org/14698028/

[Peter Kasting, 2013-07-18 18:22:44]

On 2013/07/18 18:15:37, beaudoin wrote:
> I am pretty sure we're showing fill_into_edit on an ESC.

Definitely not.  Check it with any URL that has a www. on the front by typing
the first character past the www. part.  E.g.:

* You visit www.foo.com frequently
* Type 'f' to get inline-autocompleted to "foo.com".  Noe selected match in
dropdown says "www.foo.com"
* Arrow down, then hit esc.  Verify you're still seeing "foo.com" and not
"www.foo.com"
* Arrow down, then arrow up.  Now you see "www.foo.com", demonstrating that the
fill_into_edit isn't the user typing, and isn't what's displayed after hitting
esc.

> The idea of current_match_ was precisely to keep the full state of what was
> being shown in the omnibox instead of tracking it with booleans and temporary
> strings in various other places. This was the design Sreeram and I came to for
a
> number of reasons, mostly because the instant state management was out of
hands.
> We should probably bring him back into the discussion if you want to dig
deeper
> in this. Happy to discuss over VC if you think it's worthwhile.

The complete omnibox state is significantly more complex than just the
currently-selected match.  You need all the fields in the currently-selected
match plus many more fields in order to describe it correctly.  Trying to hijack
the fill_into_edit field on current_match to mean something other than what
fill_into_edit classically means (namely, the text we display ONLY when arrowing
to this result) is neither sufficient to capture the omnibox state nor correct.

[beaudoin, 2013-07-18 19:15:27]

On 2013/07/18 18:22:44, Peter Kasting wrote:
> On 2013/07/18 18:15:37, beaudoin wrote:
> > I am pretty sure we're showing fill_into_edit on an ESC.
> 
> Definitely not.  Check it with any URL that has a www. on the front by typing
> the first character past the www. part.
> [...]

You are right. Thanks for that quick test, my experience matches yours. Still
can't track the entire logic in the code but I'm not surprised. :)

> The complete omnibox state is significantly more complex than just the
> currently-selected match.  You need all the fields in the currently-selected
> match plus many more fields in order to describe it correctly.  Trying to
hijack
> the fill_into_edit field on current_match to mean something other than what
> fill_into_edit classically means (namely, the text we display ONLY when
arrowing
> to this result) is neither sufficient to capture the omnibox state nor
correct.

I probably did not express myself correctly (partly because I don't understand
everything involved). current_match_ was supposed to make it re-route instant so
it did not hijack the search provider. For that purpose, most instances of
GetInfoForCurrentText have been replaced by CurrentMatch, which relies on the
current_match_ here rather than looking at result(). I definitely am not the
reference on Sreeram's long term plan so I should probably not muddle the water
more than I already have.

Regarding the current issue: I remember a unit test failing before adding this
fill_into_edit mangling here, but it may have been related to instant and it may
be gone now. I think the test I suggested above (playing with fill_into_edit a
little around here) be used to help identify problem cases where the omnibox
content unexpectedly changes. If you don't notice anything and all tests pass I
suggest you drop it.

If you drop it, you can also drop user_text() in omnibox_edit_model_.

[Mark P, 2013-07-18 19:31:23]

On Thu, Jul 18, 2013 at 12:15 PM, <beaudoin@chromium.org> wrote:

> If you drop it, you can also drop user_text() in omnibox_edit_model_.
>

Done.

--mark



>
>
>
https://codereview.chromium.**org/19197005/<https://codereview.chromium.org/1...
>

[Mark P, 2013-07-19 00:14:56]

Given the results of this discussion and the fact that the try failures seem
unconnected to this change, I'm going to check the CQ box and see if it goes
through.

--mark

[commit-bot: I haz the power, 2013-07-19 00:15:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/19197005/65001

[commit-bot: I haz the power, 2013-07-19 00:26:17]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-19 13:22:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/19197005/65001

[commit-bot: I haz the power, 2013-07-19 23:40:27]

Change committed as 212693