[omnibox] Use suggestion instead of input for base text

[omnibox] Use suggestion instead of input for base text

Tail suggestions want to line up the suggested completion e.g. "e" or "f" after the input text that they are completing "a b c d":

a b c d
-------
a b c d e
a b c d f

but they replace the some of the input text with an ellipsis:

a b c d
  ... d e
  ... d f

To line up the text correctly, the render code needs to know the text that is missing e.g. "a b c" (since the ellipsis is much shorter.)

The code was passing a numerical index just past the "a b c", but was passing the input text, which might not be the prefix of the suggestion. (Suggestions can be stale, albeit temporarily.)

This change replaces sending the input text with the actual text that the index was calculated from (the suggestion) guaranteeing that the index is valid.

BUG=717152
Review-Url: https://codereview.chromium.org/2854423002
Cr-Commit-Position: refs/heads/master@{#470941}
Committed: https://chromium.googlesource.com/chromium/src/+/c6737bb033e394fa654ef8ac2faa12306aaf062d

[Kevin Bailey, 2017-05-03 15:10:29]

Description was changed from

==========
[omnibox] Use suggestion instead of input for base text

Tail suggestion results need to forward the removed text to the
render code so that it can calculate the horizontal offset vs
the text in the Omnibox (to make it line up vertically.) The
code was passing the text in the Omnibox instead, which, due to
issues of asynchronicity, may or may not be the correct text.
This change passes the text that the offset is being calculated
relative to instead.

BUG=717152
==========

to

==========
[omnibox] Use suggestion instead of input for base text

Tail suggestion results need to forward the removed text to the
render code so that it can calculate the horizontal offset vs
the text in the Omnibox (to make it line up vertically.) The
code was passing the text in the Omnibox instead, which, due to
issues of asynchronicity, may or may not be the correct text.
This change passes the text that the offset is being calculated
relative to instead.

BUG=717152
==========

[Kevin Bailey, 2017-05-03 15:10:29]

krb@chromium.org changed reviewers:
+ pkasting@chromium.org

[Kevin Bailey, 2017-05-03 16:10:49]

Appears to fix both backspacing quickly and select and replace DCHECK.

[Peter Kasting, 2017-05-04 23:30:23]

What do you mean by "reasons of asynchronicity"?  When would the input text ever
be out of sync with the matches generated for it?

[Peter Kasting, 2017-05-04 23:31:15]

Does this have to do with when we preserve old matches temporarily as we start
new input, so we're marking up a previous suggestion against a now-shorter
input?

[Kevin Bailey, 2017-05-05 00:22:34]

On 2017/05/04 23:31:15, Peter Kasting (catching up) wrote:
> Does this have to do with when we preserve old matches temporarily as we start
> new input, so we're marking up a previous suggestion against a now-shorter
> input?

That sounds better. When I said "async", I meant that the suggestions seemed
to be old compared to the input e.g. when backspacing quickly. Backspacing
slowly didn't cause the problem, and note that even backspacing quickly
was only off by one.

[Peter Kasting, 2017-05-05 00:28:02]

On 2017/05/05 00:22:34, Kevin Bailey wrote:
> On 2017/05/04 23:31:15, Peter Kasting (catching up) wrote:
> > Does this have to do with when we preserve old matches temporarily as we
start
> > new input, so we're marking up a previous suggestion against a now-shorter
> > input?
> 
> That sounds better. When I said "async", I meant that the suggestions seemed
> to be old compared to the input e.g. when backspacing quickly. Backspacing
> slowly didn't cause the problem, and note that even backspacing quickly
> was only off by one.

If it was the thing I suggested, I'd expect to see the problem when backspacing
slowly or quickly.

I worry that this sounds like "somehow these are out of sync, I'm not sure why"
and I want to deeply understand why, because I'm not sure either.

[Kevin Bailey, 2017-05-05 00:32:13]

On 2017/05/05 00:28:02, Peter Kasting (catching up) wrote:
> On 2017/05/05 00:22:34, Kevin Bailey wrote:
> > On 2017/05/04 23:31:15, Peter Kasting (catching up) wrote:
> > > Does this have to do with when we preserve old matches temporarily as we
> start
> > > new input, so we're marking up a previous suggestion against a now-shorter
> > > input?
> > 
> > That sounds better. When I said "async", I meant that the suggestions seemed
> > to be old compared to the input e.g. when backspacing quickly. Backspacing
> > slowly didn't cause the problem, and note that even backspacing quickly
> > was only off by one.
> 
> If it was the thing I suggested, I'd expect to see the problem when
backspacing
> slowly or quickly.
> 
> I worry that this sounds like "somehow these are out of sync, I'm not sure
why"
> and I want to deeply understand why, because I'm not sure either.

I think the problem is pretty obviously in the function that I changed.
It calculates an offset into string 1, and then passes the offset and
string 2 to another function.

[Peter Kasting, 2017-05-05 00:37:01]

On 2017/05/05 00:32:13, Kevin Bailey wrote:
> On 2017/05/05 00:28:02, Peter Kasting (catching up) wrote:
> > On 2017/05/05 00:22:34, Kevin Bailey wrote:
> > > On 2017/05/04 23:31:15, Peter Kasting (catching up) wrote:
> > > > Does this have to do with when we preserve old matches temporarily as we
> > start
> > > > new input, so we're marking up a previous suggestion against a
now-shorter
> > > > input?
> > > 
> > > That sounds better. When I said "async", I meant that the suggestions
seemed
> > > to be old compared to the input e.g. when backspacing quickly. Backspacing
> > > slowly didn't cause the problem, and note that even backspacing quickly
> > > was only off by one.
> > 
> > If it was the thing I suggested, I'd expect to see the problem when
> backspacing
> > slowly or quickly.
> > 
> > I worry that this sounds like "somehow these are out of sync, I'm not sure
> why"
> > and I want to deeply understand why, because I'm not sure either.
> 
> I think the problem is pretty obviously in the function that I changed.
> It calculates an offset into string 1, and then passes the offset and
> string 2 to another function.

Yeah, that certainly looks suspicious.  I'm still not 100% clear on what these
params all mean/do; it seems like maybe the "input text" one needs renaming
since you're making it not be the input text?  And it seems like the CL
description could use a clarification to describe the problem in more detail.

[Kevin Bailey, 2017-05-05 21:14:42]

Description was changed from

==========
[omnibox] Use suggestion instead of input for base text

Tail suggestion results need to forward the removed text to the
render code so that it can calculate the horizontal offset vs
the text in the Omnibox (to make it line up vertically.) The
code was passing the text in the Omnibox instead, which, due to
issues of asynchronicity, may or may not be the correct text.
This change passes the text that the offset is being calculated
relative to instead.

BUG=717152
==========

to

==========
[omnibox] Use suggestion instead of input for base text

Tail suggestions want to line up the suggested completion e.g. "e" or "f" after
the input text that they are completing "a b c d":

a b c d
-------
a b c d e
a b c d f

but they replace the some of the input text with an ellipsis:

a b c d
  ... d e
  ... d f

To line up the text correctly, the render code needs to know the text that is
missing e.g. "a b c" (since the ellipsis is much shorter.)

The code was passing a numerical index just past the "a b c", but was passing
the input text, which might not be the prefix of the suggestion. (Suggestions
can be stale, albeit temporarily.)

This change replaces sending the input text with the actual text that the index
was calculated from (the suggestion) guaranteeing that the index is valid.

BUG=717152
==========

[Kevin Bailey, 2017-05-06 19:56:21]

krb@chromium.org changed reviewers:
+ avi@chromium.org

[Kevin Bailey, 2017-05-06 19:56:45]

On 2017/05/05 00:37:01, Peter Kasting (catching up) wrote:
> On 2017/05/05 00:32:13, Kevin Bailey wrote:
> > On 2017/05/05 00:28:02, Peter Kasting (catching up) wrote:
> > > On 2017/05/05 00:22:34, Kevin Bailey wrote:
> > > > On 2017/05/04 23:31:15, Peter Kasting (catching up) wrote:
> > > > > Does this have to do with when we preserve old matches temporarily as
we
> > > start
> > > > > new input, so we're marking up a previous suggestion against a
> now-shorter
> > > > > input?
> > > > 
> > > > That sounds better. When I said "async", I meant that the suggestions
> seemed
> > > > to be old compared to the input e.g. when backspacing quickly.
Backspacing
> > > > slowly didn't cause the problem, and note that even backspacing quickly
> > > > was only off by one.
> > > 
> > > If it was the thing I suggested, I'd expect to see the problem when
> > backspacing
> > > slowly or quickly.
> > > 
> > > I worry that this sounds like "somehow these are out of sync, I'm not sure
> > why"
> > > and I want to deeply understand why, because I'm not sure either.
> > 
> > I think the problem is pretty obviously in the function that I changed.
> > It calculates an offset into string 1, and then passes the offset and
> > string 2 to another function.
> 
> Yeah, that certainly looks suspicious.  I'm still not 100% clear on what these
> params all mean/do; it seems like maybe the "input text" one needs renaming
> since you're making it not be the input text?  And it seems like the CL
> description could use a clarification to describe the problem in more detail.

Done and done.

[Peter Kasting, 2017-05-06 20:13:07]

So did you determine for sure that this happens due to preserving old matches
during the initial stages of a new autocomplete query?

LGTM if so, with naming question.

https://codereview.chromium.org/2854423002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/base_search_provider.cc (right):

https://codereview.chromium.org/2854423002/diff/20001/components/omnibox/brow...
components/omnibox/browser/base_search_provider.cc:256:
match.RecordAdditionalInfo(kACMatchPropertyContentsText,
Is this the right name for this?  It seems like suggestion() is distinct from
match_contents() (used above), though I'm not sure I understand precisely how.

[Avi (use Gerrit), 2017-05-06 21:53:22]

It's not clear why I was added as a reviewer. If for the cocoa side of things,
stamp lgtm as it's just a change to the constant.

[Kevin Bailey, 2017-05-06 21:55:01]

pkasting wrote:
> So did you determine for sure that this happens due to preserving old matches
> during the initial stages of a new autocomplete query?

The stack trace goes up to '::Start()' with 'input.from_omnibox_focus() ==
false', so 'ClearAllResults()' will not be called. I'm not sure how else they'd
be cleared in this stack trace.

If it helps, this is the direct result of selecting all and hitting 'a'.

https://codereview.chromium.org/2854423002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/base_search_provider.cc (right):

https://codereview.chromium.org/2854423002/diff/20001/components/omnibox/brow...
components/omnibox/browser/base_search_provider.cc:256:
match.RecordAdditionalInfo(kACMatchPropertyContentsText,
On 2017/05/06 20:13:07, Peter Kasting (catching up) wrote:
> Is this the right name for this?  It seems like suggestion() is distinct from
> match_contents() (used above), though I'm not sure I understand precisely how.

I could work 'suggestion' into the name if you like, but it seems like a subtle
difference to me.

[Kevin Bailey, 2017-05-06 21:56:52]

On 2017/05/06 21:53:22, Avi (ping after 24h) wrote:
> It's not clear why I was added as a reviewer. If for the cocoa side of things,
> stamp lgtm as it's just a change to the constant.

Hi Avi,

yes, I just needed an ok for the constant name change. I looked through the
OWNERS file, and looked through some recent changes to the Mac omnibox for
someone with currency.

cheers

[Peter Kasting, 2017-05-07 00:54:04]

Can we test this?  Maybe the process of writing the test will make it maximally
clear how this is occurring.  Also it'd be good to have a test anyway.

[Kevin Bailey, 2017-05-09 17:02:12]

On 2017/05/07 00:54:04, Peter Kasting wrote:
> Can we test this?  Maybe the process of writing the test will make it
maximally
> clear how this is occurring.  Also it'd be good to have a test anyway.

Added unit test

[Peter Kasting, 2017-05-10 20:38:12]

LGTM

https://codereview.chromium.org/2854423002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/base_search_provider.cc (right):

https://codereview.chromium.org/2854423002/diff/20001/components/omnibox/brow...
components/omnibox/browser/base_search_provider.cc:256:
match.RecordAdditionalInfo(kACMatchPropertyContentsText,
On 2017/05/06 21:55:01, Kevin Bailey wrote:
> On 2017/05/06 20:13:07, Peter Kasting (catching up) wrote:
> > Is this the right name for this?  It seems like suggestion() is distinct
from
> > match_contents() (used above), though I'm not sure I understand precisely
how.
> 
> I could work 'suggestion' into the name if you like, but it seems like a
subtle
> difference to me.

I think that's why I'm worried about this -- this property is not being set to
the contents, but to something else, and when I don't know what the difference
really is, I don't know how worried to be.

kACMatchPropertySuggestionText?

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/base_search_provider_unittest.cc (right):

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider_unittest.cc:185:
std::unique_ptr<TemplateURL> template_url(new TemplateURL(data));
Nit: Prefer MakeUnique to bare new:

  auto template_url = base::MakeUnique<TemplateURL>(data);

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider_unittest.cc:188:
base::ASCIIToUTF16("weather"), 7, "", GURL(), base::string16(),
Nit: Prefer std::string() to ""

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider_unittest.cc:210: EXPECT_EQ(1UL,
map.size());
Nit: Maybe this should be ASSERT, so we can blindly deref the first element
below and eliminate the loop around it?  That would also reduce the amount of
(probably not helpful) error spew in the case where there's more than one
element in the map.

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider_unittest.cc:214: int length;
Nit: Declare as size_t; then no EXPECT_GE is needed just below, and no
static_cast in the last line of the loop.

[Kevin Bailey, 2017-05-11 04:03:07]

The CQ bit was checked by krb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 04:04:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 04:57:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 04:57:01]

Dry run: This issue passed the CQ dry run.

[Kevin Bailey, 2017-05-11 13:27:38]

https://codereview.chromium.org/2854423002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/base_search_provider.cc (right):

https://codereview.chromium.org/2854423002/diff/20001/components/omnibox/brow...
components/omnibox/browser/base_search_provider.cc:256:
match.RecordAdditionalInfo(kACMatchPropertyContentsText,
On 2017/05/10 20:38:11, Peter Kasting wrote:
> On 2017/05/06 21:55:01, Kevin Bailey wrote:
> > On 2017/05/06 20:13:07, Peter Kasting (catching up) wrote:
> > > Is this the right name for this?  It seems like suggestion() is distinct
> from
> > > match_contents() (used above), though I'm not sure I understand precisely
> how.
> > 
> > I could work 'suggestion' into the name if you like, but it seems like a
> subtle
> > difference to me.
> 
> I think that's why I'm worried about this -- this property is not being set to
> the contents, but to something else, and when I don't know what the difference
> really is, I don't know how worried to be.
> 
> kACMatchPropertySuggestionText?

Done.

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
File components/omnibox/browser/base_search_provider_unittest.cc (right):

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider_unittest.cc:185:
std::unique_ptr<TemplateURL> template_url(new TemplateURL(data));
On 2017/05/10 20:38:11, Peter Kasting wrote:
> Nit: Prefer MakeUnique to bare new:
> 
>   auto template_url = base::MakeUnique<TemplateURL>(data);

Done. Fixed the other test too.

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider_unittest.cc:188:
base::ASCIIToUTF16("weather"), 7, "", GURL(), base::string16(),
On 2017/05/10 20:38:11, Peter Kasting wrote:
> Nit: Prefer std::string() to ""

Done.

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider_unittest.cc:210: EXPECT_EQ(1UL,
map.size());
On 2017/05/10 20:38:12, Peter Kasting wrote:
> Nit: Maybe this should be ASSERT, so we can blindly deref the first element
> below and eliminate the loop around it?  That would also reduce the amount of
> (probably not helpful) error spew in the case where there's more than one
> element in the map.

Done.

https://codereview.chromium.org/2854423002/diff/60001/components/omnibox/brow...
components/omnibox/browser/base_search_provider_unittest.cc:214: int length;
On 2017/05/10 20:38:12, Peter Kasting wrote:
> Nit: Declare as size_t; then no EXPECT_GE is needed just below, and no
> static_cast in the last line of the loop.

Done. With the high template usage, they could have just had StringTo().

[Kevin Bailey, 2017-05-11 13:27:47]

The CQ bit was checked by krb@chromium.org

[Kevin Bailey, 2017-05-11 13:27:48]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2854423002/#ps120001
(title: "Remove redundant class")

[commit-bot: I haz the power, 2017-05-11 13:28:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 13:34:48]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494509267657260,
"parent_rev": "2598ba647d0e3d19bd03092d92edeb2d7816566b", "commit_rev":
"c6737bb033e394fa654ef8ac2faa12306aaf062d"}

[commit-bot: I haz the power, 2017-05-11 13:35:03]

Description was changed from

==========
[omnibox] Use suggestion instead of input for base text

Tail suggestions want to line up the suggested completion e.g. "e" or "f" after
the input text that they are completing "a b c d":

a b c d
-------
a b c d e
a b c d f

but they replace the some of the input text with an ellipsis:

a b c d
  ... d e
  ... d f

To line up the text correctly, the render code needs to know the text that is
missing e.g. "a b c" (since the ellipsis is much shorter.)

The code was passing a numerical index just past the "a b c", but was passing
the input text, which might not be the prefix of the suggestion. (Suggestions
can be stale, albeit temporarily.)

This change replaces sending the input text with the actual text that the index
was calculated from (the suggestion) guaranteeing that the index is valid.

BUG=717152
==========

to

==========
[omnibox] Use suggestion instead of input for base text

Tail suggestions want to line up the suggested completion e.g. "e" or "f" after
the input text that they are completing "a b c d":

a b c d
-------
a b c d e
a b c d f

but they replace the some of the input text with an ellipsis:

a b c d
  ... d e
  ... d f

To line up the text correctly, the render code needs to know the text that is
missing e.g. "a b c" (since the ellipsis is much shorter.)

The code was passing a numerical index just past the "a b c", but was passing
the input text, which might not be the prefix of the suggestion. (Suggestions
can be stale, albeit temporarily.)

This change replaces sending the input text with the actual text that the index
was calculated from (the suggestion) guaranteeing that the index is valid.

BUG=717152

Review-Url: https://codereview.chromium.org/2854423002
Cr-Commit-Position: refs/heads/master@{#470941}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6737bb033e394fa654ef8ac2faa...
==========

[commit-bot: I haz the power, 2017-05-11 13:35:07]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/c6737bb033e394fa654ef8ac2faa...