Omnibox: Make the Controller Reorder Matches for Inlining

Omnibox: Make the Controller Reorder Matches for Inlining

Currently the omnibox assumes that all matches that score at least
1200 must be inlineable / able to be shown in the omnibox.  This
is a sizable constraint on every omnibox provider.

This change creates a field trial for a new behavior: have the omnibox
providers score matches however they want, then have the omnibox
itself reorder matches as necessary to put an inlineable result first.

This change does three main things:
* this change adds a field to AutocompleteMatch to indicate whether
a match is allowed to be a legal default match.  (there's a distinction
between inline_autocomplete_offset and allowed_to_be_default_match
-- see earlier discussion on this changelist; that's why this change
adds that new field.)
  - this change also has to set the field appropriately in every provider.
* this change makes AutocompleteResult do the reordering as necessary
  (when the field trial is enabled)
* when the field trial is enabled, this change bypasses the code in
Shortcuts, HistoryQuick, and SearchProvider that enforce the constraint
relevance >= 1200 implies inlineable, thereby allowing those
providers to deliver more accurate relevance scores.
* this change revises and adds tests as necessary.

In addition to the pile of tests, I have tested it interactively to
verify that it improves the ordering of matches in the
settings on my personal profile where I hoped it would.

BUG=252531
R=hfung@chromium.org, msw@chromium.org, pkasting@chromium.org

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

[Mark P, 2013-07-13 00:06:16]

Hi guys,

Here's a first basic draft of this proposal.  I'd like some early feedback
before I continue onto testing and whatnot.

thanks,
mark

[Peter Kasting, 2013-07-13 01:31:40]

There are two major potential issues with this.

First is that we may shift up an inlinable result when the old code was
intentionally designed not to do so.  For example, if the HUP sets the inline
autocomplete offset on most matches it creates, then cases where today the
search provider intentionally outscores the HUP would be changed into having the
HUP be on top.  Or if we wanted an exact URL match to outscore an autocompletion
(e.g. for intranet URLs), we'd need to ensure that the exact match has an offset
that's set to (length of the fill_into_edit), or the controller will erroneously
swap up the lower match.  But I think today we _don't_ set an offset on the
exact match in these cases, so that we don't show any trailing slash after a
hostname while the user types; so we're caught with bad behavior either
direction.

Second, and related, is that providers may not bother to set the inline
autocomplete offset on inlineable matches after the first, since they assume
there's no point to doing so, in cases where maybe we should allow the
controller to swap things up.

Both of these boil down to an indictment of the "inline autocomplete offset"
method of tracking all of this.  Perhaps instead we should have multiple fields,
one a bool indicating whether the match is "allowed to be a default match"
(which would be examined by your new code), and the other a string representing
the inline autocompletion, if any, to display if this match is made default.

I have no idea how everything I've just said will impact the future world with
Instant, where we'll likely want to make some matches "grey text autocomplete".

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_result.cc:115:
AutocompleteMatch::MoreRelevant(*it, *best_inlineable_match))) {
Seems like we don't need the MoreRelevant() check if we replace the
partial_sort() above with a complete sort.

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_result.h (right):

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_result.h:167: // True if the functions
here should reorder matches  as necessary
Nit: Extra space

https://codereview.chromium.org/18878007/diff/1/chrome/browser/omnibox/omnibo...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/18878007/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:276:
kReorderForInliningFieldTrialName) == "ReorderForInlining");
Nit: Indent 4, not 12

[Mark P, 2013-07-14 00:01:35]

You raise good points.

I was under the (apparently mistaken) impression that any match that would
be shown inline / be the default match would have
inline_autocomplete_offset set.  While this is true for any match that is
inline autocompleted (has text after the cursor), I realize now it's not
true for any of the what-you-typed matches.

You suggest adding a bool "allowed to be a default match".  That seems
reasonable to me, though a bit confusing to have both it and
inline_autocomplete_offset.  Are you sure the alternate idea of making
inline_autocomplete_offset be the fill_into_edit string length for the
what-you-typed results won't fly?  I see you're worried about the trailing
slash versus not.  Wouldn't we just be able to make sure the fill_into_edit
includes/doesn't include it as appropriate?  I like this latter idea.
 AutocompleteMatch has enough state variables as it is.

By the way, we're not consistent with trailing slashes right now.  If you
type a known intranet hostname (without the slash) into your browser, press
down and up, you get the omnibox to show the hostname with a slash.  If you
do this with a internet hostname, the slash doesn't appear.

--mark

On Fri, Jul 12, 2013 at 6:31 PM, <pkasting@chromium.org> wrote:

> There are two major potential issues with this.
>
> First is that we may shift up an inlinable result when the old code was
> intentionally designed not to do so.  For example, if the HUP sets the
> inline
> autocomplete offset on most matches it creates, then cases where today the
> search provider intentionally outscores the HUP would be changed into
> having the
> HUP be on top.  Or if we wanted an exact URL match to outscore an
> autocompletion
> (e.g. for intranet URLs), we'd need to ensure that the exact match has an
> offset
> that's set to (length of the fill_into_edit), or the controller will
> erroneously
> swap up the lower match.  But I think today we _don't_ set an offset on the
> exact match in these cases, so that we don't show any trailing slash after
> a
> hostname while the user types; so we're caught with bad behavior either
> direction.
>
> Second, and related, is that providers may not bother to set the inline
> autocomplete offset on inlineable matches after the first, since they
> assume
> there's no point to doing so, in cases where maybe we should allow the
> controller to swap things up.
>
> Both of these boil down to an indictment of the "inline autocomplete
> offset"
> method of tracking all of this.  Perhaps instead we should have multiple
> fields,
> one a bool indicating whether the match is "allowed to be a default match"
> (which would be examined by your new code), and the other a string
> representing
> the inline autocompletion, if any, to display if this match is made
> default.
>
> I have no idea how everything I've just said will impact the future world
> with
> Instant, where we'll likely want to make some matches "grey text
> autocomplete".
>
>
> https://codereview.chromium.**org/18878007/diff/1/chrome/**
>
browser/autocomplete/**autocomplete_result.cc<https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/autocomplete_result.cc>
> File chrome/browser/autocomplete/**autocomplete_result.cc (right):
>
> https://codereview.chromium.**org/18878007/diff/1/chrome/**
>
browser/autocomplete/**autocomplete_result.cc#**newcode115<https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/autocomplete_result.cc#newcode115>
> chrome/browser/autocomplete/**autocomplete_result.cc:115:
> AutocompleteMatch::**MoreRelevant(*it, *best_inlineable_match))) {
> Seems like we don't need the MoreRelevant() check if we replace the
> partial_sort() above with a complete sort.
>
> https://codereview.chromium.**org/18878007/diff/1/chrome/**
>
browser/autocomplete/**autocomplete_result.h<https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/autocomplete_result.h>
> File chrome/browser/autocomplete/**autocomplete_result.h (right):
>
> https://codereview.chromium.**org/18878007/diff/1/chrome/**
>
browser/autocomplete/**autocomplete_result.h#**newcode167<https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/autocomplete_result.h#newcode167>
> chrome/browser/autocomplete/**autocomplete_result.h:167: // True if the
>
> functions here should reorder matches  as necessary
> Nit: Extra space
>
> https://codereview.chromium.**org/18878007/diff/1/chrome/**
>
browser/omnibox/omnibox_field_**trial.cc<https://codereview.chromium.org/18878007/diff/1/chrome/browser/omnibox/omnibox_field_trial.cc>
> File chrome/browser/omnibox/**omnibox_field_trial.cc (right):
>
> https://codereview.chromium.**org/18878007/diff/1/chrome/**
>
browser/omnibox/omnibox_field_**trial.cc#newcode276<https://codereview.chromium.org/18878007/diff/1/chrome/browser/omnibox/omnibox_field_trial.cc#newcode276>
> chrome/browser/omnibox/**omnibox_field_trial.cc:276:
> kReorderForInliningFieldTrialN**ame) == "ReorderForInlining");
> Nit: Indent 4, not 12
>
>
https://codereview.chromium.**org/18878007/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2013-07-14 05:05:51]

On 2013/07/14 00:01:35, Mark P wrote:
> You suggest adding a bool "allowed to be a default match".  That seems
> reasonable to me, though a bit confusing to have both it and
> inline_autocomplete_offset.

I'm not sure you understood me.  My suggestion was to kill
|inline_autocomplete_offset|.  It's always been a poor solution anyway, and for
some time I've wanted to replace it with a string.  The suggestion here is to
replace with a string and a bool.

> Are you sure the alternate idea of making
> inline_autocomplete_offset be the fill_into_edit string length for the
> what-you-typed results won't fly?

It might but that seems confusing (and potentially error-prone) to me.  It's
easier to understand if the code isn't doing something so subtle.

> I see you're worried about the trailing
> slash versus not.  Wouldn't we just be able to make sure the fill_into_edit
> includes/doesn't include it as appropriate?

I'm not convinced we could.  I worry about subtleties like intranet URLs where
slash-stripping can change behavior.  I don't think I have a well-formed example
of something bad here.  I just worry that Here There Be Dragons.

> By the way, we're not consistent with trailing slashes right now.  If you
> type a known intranet hostname (without the slash) into your browser, press
> down and up, you get the omnibox to show the hostname with a slash.  If you
> do this with a internet hostname, the slash doesn't appear.

That is intentional, and is basically what I was alluding to above.  It has to
do with how, even if you've never visited "foo.com", we'll parse it as
URL-by-default, but we won't with "foo".  So when we're trying to treat them
both as URLs, we can always safely strip the slash on the former, but not
necessarily on the latter.  This is confusing because omnibox behavior depends
on history and not just parsing -- which is why you said "known intranet host"
-- but we're trying to ensure things like, if you edit the URL, or you paste it
into other Chrome windows or something, or if history gets purged while you're
typing, the behavior doesn't change.

It's a mess.

[Mark P, 2013-07-15 18:24:35]

On Sat, Jul 13, 2013 at 10:05 PM, <pkasting@chromium.org> wrote:

> On 2013/07/14 00:01:35, Mark P wrote:
>
>> You suggest adding a bool "allowed to be a default match".  That seems
>> reasonable to me, though a bit confusing to have both it and
>> inline_autocomplete_offset.
>>
>
> I'm not sure you understood me.  My suggestion was to kill
> |inline_autocomplete_offset|.  It's always been a poor solution anyway,
> and for
> some time I've wanted to replace it with a string.  The suggestion here is
> to
> replace with a string and a bool.


Ah, yes, I did misunderstand you.

So you suggest replacing |inline_autocomplete_offset| with a string
|inline_autocompletion|?  If we're in that world, then all this changelist
has to do is add a bool |allowed_to_be_default_match|.  (I assume you're
not too worried about the increased space requirement.)  This sounds like a
straightforward world.

I looked through the code to see what it would take to change
|inline_autocomplete_offset| to |inline_autocompletion|.  It looks easy
with one exception.  It's mostly easy because in fact almost all the uses
of |inline_autocomplete_offset| are used to chop the end off
|fill_into_edit| (starting at |inline_autocomplete_offset|) -- this result
is exactly what |inline_autocompletion| would already be.

The only exception is this block of code that handles what text gets put in
the omnibox after the user has arrowed down to select a match then presses
escape.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Do you have a suggestion for how to fix this code to use
|inline_autocompletion|?  I haven't checked but do you think |user_text_|
will be the original text the user entered (i.e., a good thing to restore
the omnibox to)?

--mark


>
>  Are you sure the alternate idea of making
>> inline_autocomplete_offset be the fill_into_edit string length for the
>> what-you-typed results won't fly?
>>
>
> It might but that seems confusing (and potentially error-prone) to me.
>  It's
> easier to understand if the code isn't doing something so subtle.
>
>
>  I see you're worried about the trailing
>> slash versus not.  Wouldn't we just be able to make sure the
>> fill_into_edit
>> includes/doesn't include it as appropriate?
>>
>
> I'm not convinced we could.  I worry about subtleties like intranet URLs
> where
> slash-stripping can change behavior.  I don't think I have a well-formed
> example
> of something bad here.  I just worry that Here There Be Dragons.
>
>
>  By the way, we're not consistent with trailing slashes right now.  If you
>> type a known intranet hostname (without the slash) into your browser,
>> press
>> down and up, you get the omnibox to show the hostname with a slash.  If
>> you
>> do this with a internet hostname, the slash doesn't appear.
>>
>
> That is intentional, and is basically what I was alluding to above.  It
> has to
> do with how, even if you've never visited "foo.com", we'll parse it as
> URL-by-default, but we won't with "foo".  So when we're trying to treat
> them
> both as URLs, we can always safely strip the slash on the former, but not
> necessarily on the latter.  This is confusing because omnibox behavior
> depends
> on history and not just parsing -- which is why you said "known intranet
> host"
> -- but we're trying to ensure things like, if you edit the URL, or you
> paste it
> into other Chrome windows or something, or if history gets purged while
> you're
> typing, the behavior doesn't change.
>
> It's a mess.
>
>
https://codereview.chromium.**org/18878007/<https://codereview.chromium.org/1...
>

[Peter Kasting, 2013-07-15 18:29:31]

On 2013/07/15 18:24:35, Mark P wrote:
> The only exception is this block of code that handles what text gets put in
> the omnibox after the user has arrowed down to select a match then presses
> escape.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> Do you have a suggestion for how to fix this code to use
> |inline_autocompletion|?  I haven't checked but do you think |user_text_|
> will be the original text the user entered (i.e., a good thing to restore
> the omnibox to)?

I'm confused about why you're confused.  Did you highlight the wrong block of
code?  This doesn't seem to be about resetting the text after hitting escape.

If this is really the code you meant, it looks to me as if it's straightforward.
 In a world where you have the inline autocompletion string already, this block
disappears and you simply pass that string to OnPopupDataChanged() directly.

[Mark P, 2013-07-15 19:34:55]

On Mon, Jul 15, 2013 at 11:29 AM, <pkasting@chromium.org> wrote:

> On 2013/07/15 18:24:35, Mark P wrote:
>
>> The only exception is this block of code that handles what text gets put
>> in
>> the omnibox after the user has arrowed down to select a match then presses
>> escape.
>>
>
> https://code.google.com/p/**chromium/codesearch#chromium/**
>
src/chrome/browser/ui/omnibox/**omnibox_edit_model.cc&l=1114-**1120<https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/omnibox/omnibox_edit_model.cc&l=1114-1120>
>
>  Do you have a suggestion for how to fix this code to use
>> |inline_autocompletion|?  I haven't checked but do you think |user_text_|
>> will be the original text the user entered (i.e., a good thing to restore
>> the omnibox to)?
>>
>
> I'm confused about why you're confused.  Did you highlight the wrong block
> of
> code?


I must have.  The code I sent you looks straightforward.

I can't find the code I thought was problematic.

Maybe I was confused.  Maybe I shouldn't get up too easy and start replying
to e-mail.

New changelist coming, adding |inline_autocompletion|.

--mark



> This doesn't seem to be about resetting the text after hitting escape.
>
> If this is really the code you meant, it looks to me as if it's
> straightforward.
>  In a world where you have the inline autocompletion string already, this
> block
> disappears and you simply pass that string to OnPopupDataChanged()
> directly.
>
>
https://codereview.chromium.**org/18878007/<https://codereview.chromium.org/1...
>

[Mark P, 2013-07-17 15:45:18]

Hi Peter,

I revised the strategy in this changelist, adding an
|allowed_to_be_default_match| field as we discussed.  What do you think of this
new approach?

Note that I haven't tested this or anything yet.

--mark

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_result.cc:115:
AutocompleteMatch::MoreRelevant(*it, *best_inlineable_match))) {
On 2013/07/13 01:31:40, Peter Kasting wrote:
> Seems like we don't need the MoreRelevant() check if we replace the
> partial_sort() above with a complete sort.

I thought about this.

For long inputs, the set of matches will be small (say, 5) so the difference
between full sort and partial sort is zero.  Under this condition, we probably
end up with a few (say, 4) MoreRelevant calls.  This is fast so this condition
doesn't matter.

For short inputs, we have larger results set, more on the order of 20.  Sorting
the extra 14 elements (after the first 6) will take something like 50 calls to
MoreRelevant.  This is substantially more than the at-most 20 that will happen
with this extra loop to find the most relevant inlineable result.

I think it's better leave it the way it is.

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_result.h (right):

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_result.h:167: // True if the functions
here should reorder matches  as necessary
On 2013/07/13 01:31:40, Peter Kasting wrote:
> Nit: Extra space

Done.

https://codereview.chromium.org/18878007/diff/1/chrome/browser/omnibox/omnibo...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/18878007/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:276:
kReorderForInliningFieldTrialName) == "ReorderForInlining");
On 2013/07/13 01:31:40, Peter Kasting wrote:
> Nit: Indent 4, not 12

Done.

[Peter Kasting, 2013-07-17 18:29:48]

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_result.cc:115:
AutocompleteMatch::MoreRelevant(*it, *best_inlineable_match))) {
On 2013/07/17 15:45:18, Mark P wrote:
> For short inputs, we have larger results set, more on the order of 20. 
Sorting
> the extra 14 elements (after the first 6) will take something like 50 calls to
> MoreRelevant.  This is substantially more than the at-most 20 that will happen
> with this extra loop to find the most relevant inlineable result.
> 
> I think it's better leave it the way it is.

50 calls to MoreRelevant seems immaterial.  Each one is a string equality check
and a numeric compare.  Doing 50 of those shouldn't take measurable time, even
if we have to run SortAndCull a dozen times for each keystroke, especially if
the alternative is that we're already doing 20.  And it would make the code less
confusing.

If you were really worried, you could locally put a small timing block around
this and then type a few short inputs into your omnibox to check how long this
takes.  I bet it will be tremendously short.

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.h:255: // match.
Nit: I might add something like:

Providers should set this to true only if the user's input, plus any inline
autocompletion on this match, would lead the user to expect a navigation to this
match's destination URL.  For example, with input "foo", a search for "bar" or
navigation to "bar.com" should not set this flag; a navigation to "foo.com"
should only set this flag if ".com" will be inline autocompleted; and a
navigation to "foo/" (an intranet host) or search for "foo" could (and probably
should) set this flag.

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:108: if ((begin() != end())
&& !begin()->allowed_to_be_default_match &&
Nit: First condition should be "!empty()"

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:120:
DCHECK(best_legal_default_match != end());
Our existing code probably violates this, so be careful.

For example, if I type a '?' alone, I probably only get back the "placeholder
result" from the search provider -- which isn't currently "allowed to be
default".  (The fix there is presumably to set the "allowed to be default" flag
on that match.)

There may be other places where people weren't setting any inline autocomplete
offset but we might need to allow something to be default.  Auditing every
construction of AutocompleteMatch carefully is probably the only way to find
them :(

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:128:
default_match_->allowed_to_be_default_match);
Nit: Indent 4, not even

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1134: if
((!omnibox_will_reorder_for_legal_default_match_ &&
Nit: Simpler:

    if (omnibox_will_reorder_for_legal_default_match_ ?
        LacksDefaultMatch() : IsTopMatchNotAllowedToBeDefaultMatch())

[Mark P, 2013-07-17 20:39:18]

I'll take your lack of major complaints to be support for this general approach.

--mark

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_result.cc:115:
AutocompleteMatch::MoreRelevant(*it, *best_inlineable_match))) {
On 2013/07/17 18:29:48, Peter Kasting wrote:
> On 2013/07/17 15:45:18, Mark P wrote:
> > For short inputs, we have larger results set, more on the order of 20. 
> Sorting
> > the extra 14 elements (after the first 6) will take something like 50 calls
to
> > MoreRelevant.  This is substantially more than the at-most 20 that will
happen
> > with this extra loop to find the most relevant inlineable result.
> > 
> > I think it's better leave it the way it is.
> 
> 50 calls to MoreRelevant seems immaterial.  Each one is a string equality
check
> and a numeric compare.  Doing 50 of those shouldn't take measurable time, even
> if we have to run SortAndCull a dozen times for each keystroke, especially if
> the alternative is that we're already doing 20.  And it would make the code
less
> confusing.
> 
> If you were really worried, you could locally put a small timing block around
> this and then type a few short inputs into your omnibox to check how long this
> takes.  I bet it will be tremendously short.

Okay, I trust your judgement.

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.h:255: // match.
On 2013/07/17 18:29:48, Peter Kasting wrote:
> Nit: I might add something like:
> 
> Providers should set this to true only if the user's input, plus any inline
> autocompletion on this match, would lead the user to expect a navigation to
this
> match's destination URL.  For example, with input "foo", a search for "bar" or
> navigation to "bar.com" should not set this flag; a navigation to "foo.com"
> should only set this flag if ".com" will be inline autocompleted; and a
> navigation to "foo/" (an intranet host) or search for "foo" could (and
probably
> should) set this flag.

That's a good explanation.  Inserted with minor editing.

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:108: if ((begin() != end())
&& !begin()->allowed_to_be_default_match &&
On 2013/07/17 18:29:48, Peter Kasting wrote:
> Nit: First condition should be "!empty()"

Done.  Note that given the code below, I can remove the whole first line of this
if clause if we want.  It depends on whether you think it's clearer or faster
with these parts of the test or without.

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:120:
DCHECK(best_legal_default_match != end());
On 2013/07/17 18:29:48, Peter Kasting wrote:
> Our existing code probably violates this, so be careful.

Yes, I know.  I added the DCHECK in hope that it would help me iron out all the
kinks as in the placeholder one you found below.

> For example, if I type a '?' alone, I probably only get back the "placeholder
> result" from the search provider -- which isn't currently "allowed to be
> default".  (The fix there is presumably to set the "allowed to be default"
flag
> on that match.)

Did that.

> There may be other places where people weren't setting any inline autocomplete
> offset but we might need to allow something to be default.  Auditing every
> construction of AutocompleteMatch carefully is probably the only way to find
> them :(

I audited every provider and within them searched for "fill_into_edit".  I
thought that would find me every match.  Apparently it made me miss the
placeholder one (no fill_into_edit oddly).  I just did more searches and I think
that's the only one I missed.

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:128:
default_match_->allowed_to_be_default_match);
On 2013/07/17 18:29:48, Peter Kasting wrote:
> Nit: Indent 4, not even

Done.

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1134: if
((!omnibox_will_reorder_for_legal_default_match_ &&
On 2013/07/17 18:29:48, Peter Kasting wrote:
> Nit: Simpler:
> 
>     if (omnibox_will_reorder_for_legal_default_match_ ?
>         LacksDefaultMatch() : IsTopMatchNotAllowedToBeDefaultMatch())

Done.

[Peter Kasting, 2013-07-17 22:12:30]

LGTM

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:108: if ((begin() != end())
&& !begin()->allowed_to_be_default_match &&
On 2013/07/17 20:39:18, Mark P wrote:
> On 2013/07/17 18:29:48, Peter Kasting wrote:
> > Nit: First condition should be "!empty()"
> 
> Done.  Note that given the code below, I can remove the whole first line of
this
> if clause if we want.  It depends on whether you think it's clearer or faster
> with these parts of the test or without.

I would leave these here.  I think it's slightly clearer with them in place.

I would use "begin() + 1" as the initial value in the loop in that case, since
these tests guarantee that's both safe and correct.

https://codereview.chromium.org/18878007/diff/51001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/51001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1142: // * no matches are allowed
to be the default match.  SearchProvider is
Tiny nit: I'd capitalize the first word of each bullet

[Mark P, 2013-07-17 23:50:45]

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/35001/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:108: if ((begin() != end())
&& !begin()->allowed_to_be_default_match &&
On 2013/07/17 22:12:30, Peter Kasting wrote:
> On 2013/07/17 20:39:18, Mark P wrote:
> > On 2013/07/17 18:29:48, Peter Kasting wrote:
> > > Nit: First condition should be "!empty()"
> > 
> > Done.  Note that given the code below, I can remove the whole first line of
> this
> > if clause if we want.  It depends on whether you think it's clearer or
faster
> > with these parts of the test or without.
> 
> I would leave these here.  I think it's slightly clearer with them in place.
> 
> I would use "begin() + 1" as the initial value in the loop in that case, since
> these tests guarantee that's both safe and correct.

Done.

https://codereview.chromium.org/18878007/diff/51001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/51001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1142: // * no matches are allowed
to be the default match.  SearchProvider is
On 2013/07/17 22:12:30, Peter Kasting wrote:
> Tiny nit: I'd capitalize the first word of each bullet

Done.

[msw, 2013-07-18 06:23:57]

This will definitely need some unit tests.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.cc:114:
allowed_to_be_default_match = match.allowed_to_be_default_match;
Does any code modify a copied match in some way that should alter this value?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:108: // Top match is not
allowed to be the default match.  Find the most
I find it odd that this simply checks the flag to determine if a match is truly
allowed to be the default, rather than enforcing something, like exact input or
inlinable fill_into_edit. I like the idea of making Autocomplete
Result|Controller enforce the sensibility of the default match. Unfortunately,
this solution falls short of what would hope for, by leaving the validity of a
legal default match up to individual providers. So the logic to determine inline
autocompletion behavior and the capacity of a match to be the default is still
scattered. Also, a simple error in setting the flag elsewhere could yield
undesirable behavior without any safeguard here to check that a default match is
truly allowed. What do you think?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:112:
std::rotate(matches_.begin(), it, it + 1);
Why rotate instead of just moving the new default match to the top?
Shouldn't this try to be the least disruptive it can be?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:121:
default_match_->allowed_to_be_default_match);
Won't this fail if the user is not in the experiment and the providers were
disobedient?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/extension_app_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/extension_app_provider.cc:83:
match.allowed_to_be_default_match = true;
Why is this always true?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_quick_provider.cc:226:
(!PreventInlineAutocomplete(autocomplete_input_) ||
Did you mean to negate this boolean? It looks unintentional.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_url_provider.cc:352:
match.allowed_to_be_default_match = true;
How does this relate to the seemingly contradictory comment below?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_url_provider.cc:1064:
match.allowed_to_be_default_match = true;
Should this only be true is inline_autocomplete_offset != string16::npos?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1075: bool
SearchProvider::IsTopMatchNotInlinable() const {
This function is almost what I'd expect AutocompleteResult to check to ensure
that matches marked as allowed really are allowed.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1080: bool
SearchProvider::IsTopMatchNotAllowedToBeDefaultMatch() const {
Can you combine these two functions? It'd make more sense if the logic to check
only the top match versus checking anywhere within the set of matches were more
tightly related to the omnibox_will_reorder_for_legal_default_match_ flag that
controls this nuanced detail.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1147: //   false, the first part
of the test above will suffice.)
I don't follow "the first part of the test above will suffice", can you be more
explicit? Or move this logic into the combined LacksDefaultMatch() :
IsTopMatchNotAllowedToBeDefaultMatch() I suggested above, and I think it'll be a
lot more self-evident from the code.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/shortcuts_provider.cc:180: // may allow it to become
the highest ranked matches (= default match)
nit: s/matches/match, also s/=/i.e. the/

[Peter Kasting, 2013-07-18 17:35:28]

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.cc:114:
allowed_to_be_default_match = match.allowed_to_be_default_match;
On 2013/07/18 06:23:57, msw wrote:
> Does any code modify a copied match in some way that should alter this value?

If there is some, I don't know what we should do other than have that code
modify this value directly.  The only change we could make here -- namely, not
copying over this value automatically -- is definitely wrong and will break lots
of code, e.g. everything that puts matches in a vector.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:108: // Top match is not
allowed to be the default match.  Find the most
On 2013/07/18 06:23:57, msw wrote:
> I find it odd that this simply checks the flag to determine if a match is
truly
> allowed to be the default, rather than enforcing something, like exact input
or
> inlinable fill_into_edit.

It's basically impossible to enforce this correctly because this code would have
to understand every way we can do fuzzy matching.

For example, if the user inputs "http:///foo.co", we can inline-autocomplete
with a fill_into_edit of "http://foo.com".  It's pretty much impossible at this
point to realize that that removed slash in the middle is "acceptable" for
inlining purposes.

So I don't think we can or should try to do anything smart here.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:112:
std::rotate(matches_.begin(), it, it + 1);
On 2013/07/18 06:23:57, msw wrote:
> Why rotate instead of just moving the new default match to the top?

This rotate command _does_ just move the default match to the top.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:121:
default_match_->allowed_to_be_default_match);
On 2013/07/18 06:23:57, msw wrote:
> Won't this fail if the user is not in the experiment and the providers were
> disobedient?

Yes.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_quick_provider.cc:226:
(!PreventInlineAutocomplete(autocomplete_input_) ||
On 2013/07/18 06:23:57, msw wrote:
> Did you mean to negate this boolean? It looks unintentional.

Hmm, good catch, I agree.  I looked at this code when I reviewed but I think I
made a mistake in reading it.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_url_provider.cc:352:
match.allowed_to_be_default_match = true;
On 2013/07/18 06:23:57, msw wrote:
> How does this relate to the seemingly contradictory comment below?

We do want the match to be default, we just don't want random trailing slashes
to appear as the user is finishing typing URLs.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_url_provider.cc:1064:
match.allowed_to_be_default_match = true;
On 2013/07/18 06:23:57, msw wrote:
> Should this only be true is inline_autocomplete_offset != string16::npos?

I don't think so but I'm not actually sure.

[msw, 2013-07-18 17:46:53]

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.cc:114:
allowed_to_be_default_match = match.allowed_to_be_default_match;
On 2013/07/18 17:35:28, Peter Kasting wrote:
> On 2013/07/18 06:23:57, msw wrote:
> > Does any code modify a copied match in some way that should alter this
value?
> 
> If there is some, I don't know what we should do other than have that code
> modify this value directly.  The only change we could make here -- namely, not
> copying over this value automatically -- is definitely wrong and will break
lots
> of code, e.g. everything that puts matches in a vector.

Yes, I would suggest modifying the code that does the copying, if needed. I
don't think this code should be changed, but I think we should audit places that
copy (and modify) matches to ensure they're not doing something wrong with this
new behavior.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:108: // Top match is not
allowed to be the default match.  Find the most
On 2013/07/18 17:35:28, Peter Kasting wrote:
> On 2013/07/18 06:23:57, msw wrote:
> > I find it odd that this simply checks the flag to determine if a match is
> truly
> > allowed to be the default, rather than enforcing something, like exact input
> or
> > inlinable fill_into_edit.
> 
> It's basically impossible to enforce this correctly because this code would
have
> to understand every way we can do fuzzy matching.
> 
> For example, if the user inputs "http:///foo.co", we can inline-autocomplete
> with a fill_into_edit of "http://foo.com".  It's pretty much impossible at
this
> point to realize that that removed slash in the middle is "acceptable" for
> inlining purposes.
> 
> So I don't think we can or should try to do anything smart here.

I disagree, I suspect the problem is hard, but should be addressed.
URL prefix matching should be able to handle an extra slash.
Can you think of other cases that really would make it impossible?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:112:
std::rotate(matches_.begin(), it, it + 1);
On 2013/07/18 17:35:28, Peter Kasting wrote:
> On 2013/07/18 06:23:57, msw wrote:
> > Why rotate instead of just moving the new default match to the top?
> 
> This rotate command _does_ just move the default match to the top.

afaict this will modify the matches preceding |it| as well:
http://www.cplusplus.com/reference/algorithm/rotate/
Looks like {a, b, c*, d} would become {c*, a, b, d}, right?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_url_provider.cc:352:
match.allowed_to_be_default_match = true;
On 2013/07/18 17:35:28, Peter Kasting wrote:
> On 2013/07/18 06:23:57, msw wrote:
> > How does this relate to the seemingly contradictory comment below?
> 
> We do want the match to be default, we just don't want random trailing slashes
> to appear as the user is finishing typing URLs.

Ah, thanks for clarifying.

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

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:108: // Top match is not
allowed to be the default match.  Find the most
On 2013/07/18 17:46:53, msw wrote:
> URL prefix matching should be able to handle an extra slash.

I don't know what this means.

> Can you think of other cases that really would make it impossible?

In general we can do arbitrary transforms and/or spelling corrections.  Jut on
URLs we can certainly do a variety of fixup/escape/unescape changes.

I think it is completely appropriate to trust providers to do the right thing
here.  Even if it were possible to check things at this point, we'd need to do
it by duplicating all the logic in the providers that does this stuff.

It's not the controller's job to try and double-check the providers any more
than it's the edit model's job to double-check the controller (or the location
bar view's job, or whatever).

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:112:
std::rotate(matches_.begin(), it, it + 1);
On 2013/07/18 17:46:53, msw wrote:
> On 2013/07/18 17:35:28, Peter Kasting wrote:
> > On 2013/07/18 06:23:57, msw wrote:
> > > Why rotate instead of just moving the new default match to the top?
> > 
> > This rotate command _does_ just move the default match to the top.
> 
> afaict this will modify the matches preceding |it| as well:
> http://www.cplusplus.com/reference/algorithm/rotate/
> Looks like {a, b, c*, d} would become {c*, a, b, d}, right?

Yes, which is precisely the definition of "move c to the top".

[msw, 2013-07-18 18:12:11]

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:108: // Top match is not
allowed to be the default match.  Find the most
On 2013/07/18 17:51:54, Peter Kasting wrote:
> On 2013/07/18 17:46:53, msw wrote:
> > URL prefix matching should be able to handle an extra slash.
> 
> I don't know what this means.

We check for and trim schemes and www. prefixes elsewhere, and I suspect we
could do so in a robust enough manner to ignore an extra slash, etc. But your
point below makes this mostly moot.

> 
> > Can you think of other cases that really would make it impossible?
> 
> In general we can do arbitrary transforms and/or spelling corrections.  Jut on
> URLs we can certainly do a variety of fixup/escape/unescape changes.
> 
> I think it is completely appropriate to trust providers to do the right thing
> here.  Even if it were possible to check things at this point, we'd need to do
> it by duplicating all the logic in the providers that does this stuff.
> 
> It's not the controller's job to try and double-check the providers any more
> than it's the edit model's job to double-check the controller (or the location
> bar view's job, or whatever).

That's a good point; provider complexities should not be duplicated here. More
straightforward inlining would be worth checking, but presuming that we have a
good deal of substitution that would seem arbitrary to the controller, I suppose
we must trust the providers.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:112:
std::rotate(matches_.begin(), it, it + 1);
On 2013/07/18 17:51:54, Peter Kasting wrote:
> On 2013/07/18 17:46:53, msw wrote:
> > On 2013/07/18 17:35:28, Peter Kasting wrote:
> > > On 2013/07/18 06:23:57, msw wrote:
> > > > Why rotate instead of just moving the new default match to the top?
> > > 
> > > This rotate command _does_ just move the default match to the top.
> > 
> > afaict this will modify the matches preceding |it| as well:
> > http://www.cplusplus.com/reference/algorithm/rotate/
> > Looks like {a, b, c*, d} would become {c*, a, b, d}, right?
> 
> Yes, which is precisely the definition of "move c to the top".

Hahaha, *face-palm*. I'll admit at first I was thinking it'd be rotating the
whole list, which would be wrong, but then I saw the |it + 1| end arg for the
rotate and still thought it'd be wrong. But how the hell did I write an example
that shows its correctness and still miss the point? I'll chalk that up to
watching the Kissinger Fireside Chat while reviewing.

[Mark P, 2013-07-21 20:31:04]

> This will definitely need some unit tests.

You're certainly right.  It also needs regular testing too.  :-)

As the changelist description says, I haven't tested this at all.  I sent this
changelist, which mind you I think is complete for the functional code, for
review mainly to make sure I could get agreement that this is a plausible
approach.  It seems like we have that agreement.

Below I responded to the comments that still are relevant after your discussions
with Peter in the this code review comment thread.

--mark

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.cc:114:
allowed_to_be_default_match = match.allowed_to_be_default_match;
On 2013/07/18 17:46:53, msw wrote:
> On 2013/07/18 17:35:28, Peter Kasting wrote:
> > On 2013/07/18 06:23:57, msw wrote:
> > > Does any code modify a copied match in some way that should alter this
> value?
> > 
> > If there is some, I don't know what we should do other than have that code
> > modify this value directly.  The only change we could make here -- namely,
not
> > copying over this value automatically -- is definitely wrong and will break
> lots
> > of code, e.g. everything that puts matches in a vector.
> 
> Yes, I would suggest modifying the code that does the copying, if needed. I
> don't think this code should be changed, but I think we should audit places
that
> copy (and modify) matches to ensure they're not doing something wrong with
this
> new behavior.

When constructing this changelist, I examined every instance of
inline_autocomplete_offset.  This normally controls whether a match should be
allowed to be default; if this value changes, that means a whether it can be
default usually changes.  I didn't see any instances where it looks like
revising the value after the AutocompleteMatch was creates it is the right thing
to do.  I also examined every place where an AutocompleteMatch is created.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_result.cc:121:
default_match_->allowed_to_be_default_match);
On 2013/07/18 17:35:28, Peter Kasting wrote:
> On 2013/07/18 06:23:57, msw wrote:
> > Won't this fail if the user is not in the experiment and the providers were
> > disobedient?
> 
> Yes.

Precisely: I want this to fail if the user is not the experiment and the
providers were disobedient because it will help me figure out if I missed a
condition in which something should be allowed to be the default match but I
didn't mark it as such.  Or it'll let me learn if we have a bug in some provider
where something is getting to be the default match but we don't think it should
be.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/extension_app_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/extension_app_provider.cc:83:
match.allowed_to_be_default_match = true;
On 2013/07/18 06:23:57, msw wrote:
> Why is this always true?

Until very recently (when we disabled extensions/apps in the omnibox), they were
allows to be the default match.  For instance, in,  I think, say, M-26, I could
type YouTube or SshInATab and see the YouTube or SshInATab app listed first with
an app icon.  In other words, we've previously allowed apps to be the default
match based on relevance.  This changelist is not trying to change this.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_quick_provider.cc:226:
(!PreventInlineAutocomplete(autocomplete_input_) ||
On 2013/07/18 17:35:28, Peter Kasting wrote:
> On 2013/07/18 06:23:57, msw wrote:
> > Did you mean to negate this boolean? It looks unintentional.
> 
> Hmm, good catch, I agree.  I looked at this code when I reviewed but I think I
> made a mistake in reading it.

Yes, good catch.  That was a bug.  Fixed.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_url_provider.cc:1064:
match.allowed_to_be_default_match = true;
On 2013/07/18 17:35:28, Peter Kasting wrote:
> On 2013/07/18 06:23:57, msw wrote:
> > Should this only be true is inline_autocomplete_offset != string16::npos?
> 
> I don't think so but I'm not actually sure.

I think the current code is right:

HistoryURL provider always makes prefix matches only.  Isn't everything it
returns (both URLs with completions and what-you-typed results, the latter of
which can have inline_autocomplete_offset == string16::npos), allowed to be the
default match? (unless we're preventing inline autocompletion)  That's why I
think we should basically always set allowed_to_be_default_match to true.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (left):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1075: bool
SearchProvider::IsTopMatchNotInlinable() const {
On 2013/07/18 06:23:57, msw wrote:
> This function is almost what I'd expect AutocompleteResult to check to ensure
> that matches marked as allowed really are allowed.

Comment moot given the discussion you and Peter had in autocomplete_result.cc

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1080: bool
SearchProvider::IsTopMatchNotAllowedToBeDefaultMatch() const {
On 2013/07/18 06:23:57, msw wrote:
> Can you combine these two functions? It'd make more sense if the logic to
check
> only the top match versus checking anywhere within the set of matches were
more
> tightly related to the omnibox_will_reorder_for_legal_default_match_ flag that
> controls this nuanced detail.

I see your desire, but I'd like to keep them separate.  Combining them would
make something check the first if reorder is false and the latter if reorder is
true, but I like having them separate so I can write DCHECKs below that allow me
to assert the later regardless of the former.  Or maybe I could easily collect
statistics on how often the former fails when reorder is true.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1147: //   false, the first part
of the test above will suffice.)
On 2013/07/18 06:23:57, msw wrote:
> I don't follow "the first part of the test above will suffice", can you be
more
> explicit? Or move this logic into the combined LacksDefaultMatch() :
> IsTopMatchNotAllowedToBeDefaultMatch() I suggested above, and I think it'll be
a
> lot more self-evident from the code.

I rewrote this comment.  It was confusing because the original comment's
structure was explaining some code that was structured differently in the past
that I revised (to use the ? : construct) per Peter's suggestion.  I didn't
notice the comment became confusing as a result.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/shortcuts_provider.cc:180: // may allow it to become
the highest ranked matches (= default match)
On 2013/07/18 06:23:57, msw wrote:
> nit: s/matches/match, also s/=/i.e. the/

Done.

[Mark P, 2013-07-22 23:38:51]

FYI,
I've begun interactively testing this change.  I found a bug that some keyword
provider results are allowed to be the default match.  (My previous version of
this changelist assumed they couldn't be.)  I fixed it; see
keyword_provider.{cc,h}.

--mark

[msw, 2013-07-23 21:55:32]

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/autocomplete_match.cc:114:
allowed_to_be_default_match = match.allowed_to_be_default_match;
On 2013/07/21 20:31:05, Mark P wrote:
> On 2013/07/18 17:46:53, msw wrote:
> > On 2013/07/18 17:35:28, Peter Kasting wrote:
> > > On 2013/07/18 06:23:57, msw wrote:
> > > > Does any code modify a copied match in some way that should alter this
> > value?
> > > 
> > > If there is some, I don't know what we should do other than have that code
> > > modify this value directly.  The only change we could make here -- namely,
> not
> > > copying over this value automatically -- is definitely wrong and will
break
> > lots
> > > of code, e.g. everything that puts matches in a vector.
> > 
> > Yes, I would suggest modifying the code that does the copying, if needed. I
> > don't think this code should be changed, but I think we should audit places
> that
> > copy (and modify) matches to ensure they're not doing something wrong with
> this
> > new behavior.
> 
> When constructing this changelist, I examined every instance of
> inline_autocomplete_offset.  This normally controls whether a match should be
> allowed to be default; if this value changes, that means a whether it can be
> default usually changes.  I didn't see any instances where it looks like
> revising the value after the AutocompleteMatch was creates it is the right
thing
> to do.  I also examined every place where an AutocompleteMatch is created.

Thanks for doing that audit.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/extension_app_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/extension_app_provider.cc:83:
match.allowed_to_be_default_match = true;
On 2013/07/21 20:31:05, Mark P wrote:
> On 2013/07/18 06:23:57, msw wrote:
> > Why is this always true?
> 
> Until very recently (when we disabled extensions/apps in the omnibox), they
were
> allows to be the default match.  For instance, in,  I think, say, M-26, I
could
> type YouTube or SshInATab and see the YouTube or SshInATab app listed first
with
> an app icon.  In other words, we've previously allowed apps to be the default
> match based on relevance.  This changelist is not trying to change this.

I suppose this change itself is okay then, should an author verify?
So, is this provider deprecated and should it be deleted?

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_url_provider.cc:1064:
match.allowed_to_be_default_match = true;
On 2013/07/21 20:31:05, Mark P wrote:
> On 2013/07/18 17:35:28, Peter Kasting wrote:
> > On 2013/07/18 06:23:57, msw wrote:
> > > Should this only be true is inline_autocomplete_offset != string16::npos?
> > 
> > I don't think so but I'm not actually sure.
> 
> I think the current code is right:
> 
> HistoryURL provider always makes prefix matches only.  Isn't everything it
> returns (both URLs with completions and what-you-typed results, the latter of
> which can have inline_autocomplete_offset == string16::npos), allowed to be
the
> default match? (unless we're preventing inline autocompletion)  That's why I
> think we should basically always set allowed_to_be_default_match to true.

It looks like you took my suggestion in r212693 anyway...

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1080: bool
SearchProvider::IsTopMatchNotAllowedToBeDefaultMatch() const {
On 2013/07/21 20:31:05, Mark P wrote:
> On 2013/07/18 06:23:57, msw wrote:
> > Can you combine these two functions? It'd make more sense if the logic to
> check
> > only the top match versus checking anywhere within the set of matches were
> more
> > tightly related to the omnibox_will_reorder_for_legal_default_match_ flag
that
> > controls this nuanced detail.
> 
> I see your desire, but I'd like to keep them separate.  Combining them would
> make something check the first if reorder is false and the latter if reorder
is
> true, but I like having them separate so I can write DCHECKs below that allow
me
> to assert the later regardless of the former.  Or maybe I could easily collect
> statistics on how often the former fails when reorder is true.

A combined function would check that either (1) or (2) is true:
(1) The top match may be default. <OR> (2) Some match may be default.
(1) implies (2), so if (1) or (2) is true, then (2) must be true.
DCHECK(Combined) will automatically "assert the later regardless of the former".
Combine them :)

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:220: keyword.length(),
SplitReplacementStringFromInput(text, true), true, 0);
It's not obvious why this file has true or false sprinkled in to make some
matches able to be default and some not; can you clarify?

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:316: // Otherwise, we're dealing
with the "default search" result which has no
nit: please update this comment to something like:
// Otherwise, |match| is a verbatim (SEARCH_WHAT_YOU_TYPED) match.

[Mark P, 2013-07-26 16:48:13]

Hi Mike,

Thanks for your feedback.  Here's a new patchset for you to review at your
leisure.

--mark

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/extension_app_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/extension_app_provider.cc:83:
match.allowed_to_be_default_match = true;
On 2013/07/23 21:55:32, msw wrote:
> On 2013/07/21 20:31:05, Mark P wrote:
> > On 2013/07/18 06:23:57, msw wrote:
> > > Why is this always true?
> > 
> > Until very recently (when we disabled extensions/apps in the omnibox), they
> were
> > allows to be the default match.  For instance, in,  I think, say, M-26, I
> could
> > type YouTube or SshInATab and see the YouTube or SshInATab app listed first
> with
> > an app icon.  In other words, we've previously allowed apps to be the
default
> > match based on relevance.  This changelist is not trying to change this.
> 
> I suppose this change itself is okay then, should an author verify?
> So, is this provider deprecated and should it be deleted?

I'll add an appropriate reviewer and ask these questions when this changelist is
closer to final form.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/history_url_provider.cc:1064:
match.allowed_to_be_default_match = true;
On 2013/07/23 21:55:32, msw wrote:
> On 2013/07/21 20:31:05, Mark P wrote:
> > On 2013/07/18 17:35:28, Peter Kasting wrote:
> > > On 2013/07/18 06:23:57, msw wrote:
> > > > Should this only be true is inline_autocomplete_offset !=
string16::npos?
> > > 
> > > I don't think so but I'm not actually sure.
> > 
> > I think the current code is right:
> > 
> > HistoryURL provider always makes prefix matches only.  Isn't everything it
> > returns (both URLs with completions and what-you-typed results, the latter
of
> > which can have inline_autocomplete_offset == string16::npos), allowed to be
> the
> > default match? (unless we're preventing inline autocompletion)  That's why I
> > think we should basically always set allowed_to_be_default_match to true.
> 
> It looks like you took my suggestion in r212693 anyway...

Thought more; revised this code.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1080: bool
SearchProvider::IsTopMatchNotAllowedToBeDefaultMatch() const {
On 2013/07/23 21:55:32, msw wrote:
> On 2013/07/21 20:31:05, Mark P wrote:
> > On 2013/07/18 06:23:57, msw wrote:
> > > Can you combine these two functions? It'd make more sense if the logic to
> > check
> > > only the top match versus checking anywhere within the set of matches were
> > more
> > > tightly related to the omnibox_will_reorder_for_legal_default_match_ flag
> that
> > > controls this nuanced detail.
> > 
> > I see your desire, but I'd like to keep them separate.  Combining them would
> > make something check the first if reorder is false and the latter if reorder
> is
> > true, but I like having them separate so I can write DCHECKs below that
allow
> me
> > to assert the later regardless of the former.  Or maybe I could easily
collect
> > statistics on how often the former fails when reorder is true.
> 
> A combined function would check that either (1) or (2) is true:
> (1) The top match may be default. <OR> (2) Some match may be default.
> (1) implies (2), so if (1) or (2) is true, then (2) must be true.
> DCHECK(Combined) will automatically "assert the later regardless of the
former".
> Combine them :)

Please suggest a good combined name.  I can't think of one, which is why my
default is separate-makes-more-sense.

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:220: keyword.length(),
SplitReplacementStringFromInput(text, true), true, 0);
On 2013/07/23 21:55:33, msw wrote:
> It's not obvious why this file has true or false sprinkled in to make some
> matches able to be default and some not; can you clarify?

Sure.  I'll clarify in this comment; you can decide if we need to add comments
in the code.

(1)
KeywordProvider::CreateAutocompleteMatch()
does
// Creates a fully marked-up AutocompleteMatch for a specific keyword.
according to the .h
This is only called by the omnibox_controller.
Suggesting a keyword should be inlineable.

(2) on line 322
is where we create an exact match for a specific keyword but with no remaining
text.
This should be inlineable too.  It's an exact keyword suggestion.

(3) on line 368
is where we create inexact matches for keywords.  (The user types the beginning
of a keyword.)
These could reasonably be considered inlineable but they are never scores higher
than 700 according to CalculateRelevance() so in practice were never inlineable.
 I'm marking them as such (not inlineable).  I agree the one is confusing; I
added a comment there.

(4) on line 578
See comment by 569-574.  These are suggestions from extensions.  They're given
scores less than the extension's exact what-you-typed match so these are never
expected/allowed to be the default match.

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:316: // Otherwise, we're dealing
with the "default search" result which has no
On 2013/07/23 21:55:33, msw wrote:
> nit: please update this comment to something like:
> // Otherwise, |match| is a verbatim (SEARCH_WHAT_YOU_TYPED) match.

Did something like that with some editing.

[msw, 2013-07-26 20:04:49]

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/extension_app_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/extension_app_provider.cc:83:
match.allowed_to_be_default_match = true;
On 2013/07/26 16:48:13, Mark P wrote:
> On 2013/07/23 21:55:32, msw wrote:
> > On 2013/07/21 20:31:05, Mark P wrote:
> > > On 2013/07/18 06:23:57, msw wrote:
> > > > Why is this always true?
> > > 
> > > Until very recently (when we disabled extensions/apps in the omnibox),
they
> > were
> > > allows to be the default match.  For instance, in,  I think, say, M-26, I
> > could
> > > type YouTube or SshInATab and see the YouTube or SshInATab app listed
first
> > with
> > > an app icon.  In other words, we've previously allowed apps to be the
> default
> > > match based on relevance.  This changelist is not trying to change this.
> > 
> > I suppose this change itself is okay then, should an author verify?
> > So, is this provider deprecated and should it be deleted?
> 
> I'll add an appropriate reviewer and ask these questions when this changelist
is
> closer to final form.

I think now would be okay. Your general approach seems reasonable to me and I'm
running out of meaningful comments/Qs; but perhaps Peter has some deeper
concerns or perhaps you'll have a revelation from the more extensive playtesting
and automated tests you're planning. In any case, if this provider should be
deleted, then the question is moot and that change can be handled tangentially.

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1080: bool
SearchProvider::IsTopMatchNotAllowedToBeDefaultMatch() const {
On 2013/07/26 16:48:13, Mark P wrote:
> On 2013/07/23 21:55:32, msw wrote:
> > On 2013/07/21 20:31:05, Mark P wrote:
> > > On 2013/07/18 06:23:57, msw wrote:
> > > > Can you combine these two functions? It'd make more sense if the logic
to
> > > check
> > > > only the top match versus checking anywhere within the set of matches
were
> > > more
> > > > tightly related to the omnibox_will_reorder_for_legal_default_match_
flag
> > that
> > > > controls this nuanced detail.
> > > 
> > > I see your desire, but I'd like to keep them separate.  Combining them
would
> > > make something check the first if reorder is false and the latter if
reorder
> > is
> > > true, but I like having them separate so I can write DCHECKs below that
> allow
> > me
> > > to assert the later regardless of the former.  Or maybe I could easily
> collect
> > > statistics on how often the former fails when reorder is true.
> > 
> > A combined function would check that either (1) or (2) is true:
> > (1) The top match may be default. <OR> (2) Some match may be default.
> > (1) implies (2), so if (1) or (2) is true, then (2) must be true.
> > DCHECK(Combined) will automatically "assert the later regardless of the
> former".
> > Combine them :)
> 
> Please suggest a good combined name.  I can't think of one, which is why my
> default is separate-makes-more-sense.

LacksValidDefaultMatch or HasValidDefaultMatch seems good.

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:220: keyword.length(),
SplitReplacementStringFromInput(text, true), true, 0);
On 2013/07/26 16:48:13, Mark P wrote:
> On 2013/07/23 21:55:33, msw wrote:
> > It's not obvious why this file has true or false sprinkled in to make some
> > matches able to be default and some not; can you clarify?
> 
> Sure.  I'll clarify in this comment; you can decide if we need to add comments
> in the code.
> 
> (1)
> KeywordProvider::CreateAutocompleteMatch()
> does
> // Creates a fully marked-up AutocompleteMatch for a specific keyword.
> according to the .h
> This is only called by the omnibox_controller.
> Suggesting a keyword should be inlineable.
> 
> (2) on line 322
> is where we create an exact match for a specific keyword but with no remaining
> text.
> This should be inlineable too.  It's an exact keyword suggestion.
> 
> (3) on line 368
> is where we create inexact matches for keywords.  (The user types the
beginning
> of a keyword.)
> These could reasonably be considered inlineable but they are never scores
higher
> than 700 according to CalculateRelevance() so in practice were never
inlineable.
>  I'm marking them as such (not inlineable).  I agree the one is confusing; I
> added a comment there.
> 
> (4) on line 578
> See comment by 569-574.  These are suggestions from extensions.  They're given
> scores less than the extension's exact what-you-typed match so these are never
> expected/allowed to be the default match.

I think your comments are insightful enough that they belong in the code, the
behavior isn't entirely obvious without them, but then again, I haven't worked
with KeywordProvider much.

[Mark P, 2013-07-26 23:55:23]

ptal

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/52016/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1080: bool
SearchProvider::IsTopMatchNotAllowedToBeDefaultMatch() const {
On 2013/07/26 20:04:49, msw wrote:
> On 2013/07/26 16:48:13, Mark P wrote:
> > On 2013/07/23 21:55:32, msw wrote:
> > > On 2013/07/21 20:31:05, Mark P wrote:
> > > > On 2013/07/18 06:23:57, msw wrote:
> > > > > Can you combine these two functions? It'd make more sense if the logic
> to
> > > > check
> > > > > only the top match versus checking anywhere within the set of matches
> were
> > > > more
> > > > > tightly related to the omnibox_will_reorder_for_legal_default_match_
> flag
> > > that
> > > > > controls this nuanced detail.
> > > > 
> > > > I see your desire, but I'd like to keep them separate.  Combining them
> would
> > > > make something check the first if reorder is false and the latter if
> reorder
> > > is
> > > > true, but I like having them separate so I can write DCHECKs below that
> > allow
> > > me
> > > > to assert the later regardless of the former.  Or maybe I could easily
> > collect
> > > > statistics on how often the former fails when reorder is true.
> > > 
> > > A combined function would check that either (1) or (2) is true:
> > > (1) The top match may be default. <OR> (2) Some match may be default.
> > > (1) implies (2), so if (1) or (2) is true, then (2) must be true.
> > > DCHECK(Combined) will automatically "assert the later regardless of the
> > former".
> > > Combine them :)
> > 
> > Please suggest a good combined name.  I can't think of one, which is why my
> > default is separate-makes-more-sense.
> 
> LacksValidDefaultMatch or HasValidDefaultMatch seems good.

Used the first to parallel the other constraint testing functions.

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/75001/chrome/browser/autocomple...
chrome/browser/autocomplete/keyword_provider.cc:220: keyword.length(),
SplitReplacementStringFromInput(text, true), true, 0);
On 2013/07/26 20:04:49, msw wrote:
> On 2013/07/26 16:48:13, Mark P wrote:
> > On 2013/07/23 21:55:33, msw wrote:
> > > It's not obvious why this file has true or false sprinkled in to make some
> > > matches able to be default and some not; can you clarify?
> > 
> > Sure.  I'll clarify in this comment; you can decide if we need to add
comments
> > in the code.
> > 
> > (1)
> > KeywordProvider::CreateAutocompleteMatch()
> > does
> > // Creates a fully marked-up AutocompleteMatch for a specific keyword.
> > according to the .h
> > This is only called by the omnibox_controller.
> > Suggesting a keyword should be inlineable.
> > 
> > (2) on line 322
> > is where we create an exact match for a specific keyword but with no
remaining
> > text.
> > This should be inlineable too.  It's an exact keyword suggestion.
> > 
> > (3) on line 368
> > is where we create inexact matches for keywords.  (The user types the
> beginning
> > of a keyword.)
> > These could reasonably be considered inlineable but they are never scores
> higher
> > than 700 according to CalculateRelevance() so in practice were never
> inlineable.
> >  I'm marking them as such (not inlineable).  I agree the one is confusing; I
> > added a comment there.
> > 
> > (4) on line 578
> > See comment by 569-574.  These are suggestions from extensions.  They're
given
> > scores less than the extension's exact what-you-typed match so these are
never
> > expected/allowed to be the default match.
> 
> I think your comments are insightful enough that they belong in the code, the
> behavior isn't entirely obvious without them, but then again, I haven't worked
> with KeywordProvider much.

Added comments in those other three areas as requested.

[Mark P, 2013-08-05 23:39:29]

Hi reviewers,

I think it's time to start pushing this boat out to sea.

Vitaly - I touched extesion_app_provider.cc as part of this change.  Is that
provider still enabled somewhere (either Chrome or ChromeOS)?  If so, should
apps be allowed to be the default match?

Peter & Mike - Since the last time you two reviewed this change,
I've updated and added unit tests.  I think the test coverage is
pretty extensive.  There haven't been any substantive changes
to the non-test files.  In fact, the only functional change I can
find (aside from switching the field trial to the new bundled
field trial setup) is a minor change to history_url_provider
about when to mark things as a legal default match.  I found
this when testing.
  - there's a lot of review (mostly tests); if you're intimidated
by the size of this patchset, I suggest you divide up the
review.

Harry - You don't need to review this code.  This just an FYI.

thanks,
mark

P.S.  I'm hoping to get this submitted by the end of the week.

https://codereview.chromium.org/18878007/diff/150001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/150001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1865:
TEST_F(SearchProviderTest, LocalAndRemoteRelevances) {
FYI: I didn't bother adding allowed_to_be_default_match checks to this
particular test because I think the two huge TEST_F()s immediately above pretty
well cover this space.

[H Fung, 2013-08-06 22:03:17]

I didn't look at all the code, but had a few comments.

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:103:
std::sort(matches_.begin(), matches_.end(), &AutocompleteMatch::MoreRelevant);
Nit: Since you use begin() and end() below, maybe be consistent here?
(I also don't see how the existing sorting function worked since it only sorted
the first num_matches after being sorted based on destination url)

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:119: DCHECK((default_match_
== end()) ||
I'm not sure, so feel free to ignore, but maybe empty() might be clearer than
(default_match_ == end())

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result_unittest.cc:98: if
(field_trial_list_)
Nit: the if is not needed?

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:371: //TODO: uncomment
me!  chrome_variations::testing::ClearAllVariationParams();
Don't forget to uncomment/remove?

[Mark P, 2013-08-06 22:18:06]

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:103:
std::sort(matches_.begin(), matches_.end(), &AutocompleteMatch::MoreRelevant);
On 2013/08/06 22:03:17, H Fung wrote:
> Nit: Since you use begin() and end() below, maybe be consistent here?

Switched below to matches_ to be consistent with the rest of this function that
I didn't trust.

> (I also don't see how the existing sorting function worked since it only
sorted
> the first num_matches after being sorted based on destination url)

Look again at the original sorting function AutocompleteMatch::MoreRelevant.  It
sorts by relevance, breaking ties by destination URL.

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:119: DCHECK((default_match_
== end()) ||
On 2013/08/06 22:03:17, H Fung wrote:
> I'm not sure, so feel free to ignore, but maybe empty() might be clearer than
> (default_match_ == end())

I can't do
default_match_.empty()
because default_match_ is an iterator, not the underlying vector.

I could do matches_.empty() but I think using default_match_ makes things
clearer that this is an assortment about, well, default_match_.

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result_unittest.cc:98: if
(field_trial_list_)
On 2013/08/06 22:03:17, H Fung wrote:
> Nit: the if is not needed?

I will ignore this comment because
https://codereview.chromium.org/22031002/
will land before this change, and that change has a better structure for this
field trial state stuff.

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:371: //TODO: uncomment
me!  chrome_variations::testing::ClearAllVariationParams();
On 2013/08/06 22:03:17, H Fung wrote:
> Don't forget to uncomment/remove?

Good point.  I cannot uncomment this until
https://codereview.chromium.org/22031002/
lands.

I added something to the changelist description to remind me.

[H Fung, 2013-08-06 22:53:00]

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:103:
std::sort(matches_.begin(), matches_.end(), &AutocompleteMatch::MoreRelevant);
On 2013/08/06 22:18:06, Mark P wrote:
> On 2013/08/06 22:03:17, H Fung wrote:
> > Nit: Since you use begin() and end() below, maybe be consistent here?
> 
> Switched below to matches_ to be consistent with the rest of this function
that
> I didn't trust.
> 
> > (I also don't see how the existing sorting function worked since it only
> sorted
> > the first num_matches after being sorted based on destination url)
> 
> Look again at the original sorting function AutocompleteMatch::MoreRelevant. 
It
> sorts by relevance, breaking ties by destination URL.

I meant that AutocompleteMatch::DestinationSortFunc used to sort |matches_|
based on destination URL.  After that, only the first num_matches matches get
partially sorted using AutocompleteMatch::MoreRelevant.  So I'm not sure how a
high-scoring match with a URL like zillow would get sorted properly in this
function.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:107: if (!empty() &&
!begin()->allowed_to_be_default_match &&
Nit: matches_.begin() here too?

[Peter Kasting, 2013-08-06 22:56:16]

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:107: if (!empty() &&
!begin()->allowed_to_be_default_match &&
Nit: Fix this use of begin() too?

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:111: // relevance legal match
and shift it to the front.
Nit: relevance -> relevant

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result_unittest.cc:56: delete
field_trial_list_;
Nit: Use scoped_ptr<> (see other CL you have in flight)

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:225:
autocomplete_input_.current_page_classification()) &&
Nit: No parens around unary operator application

This whole expression might read a little more clearly at this point if it is
negated and the resulting values exchanged.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:278: if
(!history_match.can_inline ||
Nit: Shorter:

  match.allowed_to_be_default_match = history_match.can_inline &&
      !PreventInlineAutocomplete(autocomplete_input_);
  if (match.allowed_to_be_default_match) {
    ...

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:1069:
match.allowed_to_be_default_match = !params->prevent_inline_autocomplete ?
Nit: Simpler:

  match.allowed_to_be_default_match = !params->prevent_inline_autocomplete ||
      ((inline_autocomplete_offset != string16::npos) &&
       (inline_autocomplete_offset >= match.fill_into_edit.length()));

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:135: // A pair of
with the URL and whether that URL is allowed to be
Nit: Grammar

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:137: typedef
std::pair<std::string, bool> ExpectedUrlAndLegalDefaultMatch;
Tiny nit: Chrome code does both, but personally I prefer "URL" to "Url" in type
names.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:379: // this implicit
consequence of the relevance scoring code.
How could these be allowed to be default?  Wouldn't we need to inline
autocomplete them to do so?  And then we still wouldn't have a navigable match
because there would be no search terms.  Right?  Shouldn't all that be the
rationale for why these shouldn't be default?

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:591: // never become the default
match.  Mark them as such.
It seems like the real reason we want these to not be default is because they're
async.  Scoring less than the first match was the original way of ensuring that,
so it's more parallel to (rather than causative of) marking "can't be default"
here.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider_unittest.cc:84: const string16
description =
Nit: Maybe instead of manually adding "<< description" below you should just use
SCOPED_TRACE with this string here?

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1071: bool
SearchProvider::LacksValidDefaultMatch(
Nit: I think this function's body would read more clearly if you reversed it to
be "HasValidDefaultMatch()".  I found myself repeatedly confused about how we
were returning a bool indicating the lack of something.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1077: // to be the default match.
Nit: This comment is somewhat confusing.  How about:

  One of the SearchProvider matches may need to be the overall default.  If
AutocompleteResult is allowed to reorder matches, this means we simply need at
least one match in the list to be |allowed_to_be_default|.  If no reordering is
possible, however, then our first match needs to set this flag.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1149: // match, and it relies on
SearchProvider to always return one.)
Nit: Given the comments in LacksValidDefaultMatch(), most or all of this comment
can go away.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1155: !IsTopMatchScoreTooLow());
Nit: Indent 4, not even

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1480: (inline_autocomplete_offset
!= string16::npos);
Eh?  The conditional above has already guaranteed this holds.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.h:383: bool
omnibox_will_reorder_for_legal_default_match) const;
Nit: It's slightly confusing to refer to "omnibox" here since this code is
technically part of the omnibox already.

Maybe |allow_default_anywhere_in_list|?  I suppose you could do
|autocomplete_result_can_reorder_default_matches| or something but that seems
kind of verbose.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:171: delete
field_trial_list_;
Nit: Can you convert this to a scoped_ptr<> also?

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:258: // matches that
always allowed to be the default match.
Nit: that -> that are?

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:421: // The what you
typed match should too of course.
Nit: should too of -> should be too, of

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:177: // Reset relevance scores
to guarantee no result is given a score that may
Nit: result -> match

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:180: // problem.  In the
process of resetting scores, guarantee that all scores
Nit: You may want to say why this is a problem (or in what circumstances it's a
problem?)

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:183:
input.current_page_classification())) {
Nit: Indent 4, not 8

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider_unittest.cc:259: // they score.
Nit: Should this have a TODO to change this to allow ShortcutsProvider to
inline?

[Mark P, 2013-08-07 00:44:31]

Peter,

Thanks for the fairly comprehensive (and speedy) feedback.  Here's the next
iteration of the change.

Harry - I replied to your comments too here.

thanks,
mark

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/166001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:103:
std::sort(matches_.begin(), matches_.end(), &AutocompleteMatch::MoreRelevant);
On 2013/08/06 22:53:00, H Fung wrote:
> On 2013/08/06 22:18:06, Mark P wrote:
> > On 2013/08/06 22:03:17, H Fung wrote:
> > > Nit: Since you use begin() and end() below, maybe be consistent here?
> > 
> > Switched below to matches_ to be consistent with the rest of this function
> that
> > I didn't trust.
> > 
> > > (I also don't see how the existing sorting function worked since it only
> > sorted
> > > the first num_matches after being sorted based on destination url)
> > 
> > Look again at the original sorting function AutocompleteMatch::MoreRelevant.

> It
> > sorts by relevance, breaking ties by destination URL.
> 
> I meant that AutocompleteMatch::DestinationSortFunc used to sort |matches_|
> based on destination URL.  After that, only the first num_matches matches get
> partially sorted using AutocompleteMatch::MoreRelevant.  So I'm not sure how a
> high-scoring match with a URL like zillow would get sorted properly in this
> function.

I think you don't understand what partial_sort does.
http://www.cplusplus.com/reference/algorithm/partial_sort/

It sorts the array enough so that highest X matches are put at the beginning, in
order.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:107: if (!empty() &&
!begin()->allowed_to_be_default_match &&
On 2013/08/06 22:53:00, H Fung wrote:
> Nit: matches_.begin() here too?

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:107: if (!empty() &&
!begin()->allowed_to_be_default_match &&
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Fix this use of begin() too?

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:111: // relevance legal match
and shift it to the front.
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: relevance -> relevant

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result_unittest.cc:56: delete
field_trial_list_;
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Use scoped_ptr<> (see other CL you have in flight)

I assume the other changelist will land first so didn't bother to update this
changelist with the improvements made on that one.  (I'd figure it out when
resolving.)  But given that both you and Harry complained, I copied the changes
that are landing for the field trial stuff on that changelist to this one
(basically in effect resolving before that change lands).

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:225:
autocomplete_input_.current_page_classification()) &&
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: No parens around unary operator application

It's not around the unary operator; its around the && pair in the X in the X ? Y
: Z test.

> This whole expression might read a little more clearly at this point if it is
> negated and the resulting values exchanged.

Agreed.  Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_quick_provider.cc:278: if
(!history_match.can_inline ||
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Shorter:
> 
>   match.allowed_to_be_default_match = history_match.can_inline &&
>       !PreventInlineAutocomplete(autocomplete_input_);
>   if (match.allowed_to_be_default_match) {
>     ...

Yup, that's shorter and no less clear.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:1069:
match.allowed_to_be_default_match = !params->prevent_inline_autocomplete ?
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Simpler:
> 
>   match.allowed_to_be_default_match = !params->prevent_inline_autocomplete ||
>       ((inline_autocomplete_offset != string16::npos) &&
>        (inline_autocomplete_offset >= match.fill_into_edit.length()));

Nice.  Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:135: // A pair of
with the URL and whether that URL is allowed to be
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Grammar

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:137: typedef
std::pair<std::string, bool> ExpectedUrlAndLegalDefaultMatch;
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Tiny nit: Chrome code does both, but personally I prefer "URL" to "Url" in
type
> names.

In this context, I think URL looks better.  I prefer Url when it's at the end of
something; if it's in the middle of a camel case I think it can look odd.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:379: // this implicit
consequence of the relevance scoring code.
On 2013/08/06 22:56:16, Peter Kasting wrote:
> How could these be allowed to be default?  Wouldn't we need to inline
> autocomplete them to do so?  And then we still wouldn't have a navigable match
> because there would be no search terms.  Right?  Shouldn't all that be the
> rationale for why these shouldn't be default?

Good points.  (I misunderstand something about how destinations worked when you
typed an exact URL.)

Removed the comment.  I think the code is clear without a comment.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:591: // never become the default
match.  Mark them as such.
On 2013/08/06 22:56:16, Peter Kasting wrote:
> It seems like the real reason we want these to not be default is because
they're
> async.  Scoring less than the first match was the original way of ensuring
that,
> so it's more parallel to (rather than causative of) marking "can't be default"
> here.

Rewrote comment with that in mind.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider_unittest.cc:84: const string16
description =
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Maybe instead of manually adding "<< description" below you should just
use
> SCOPED_TRACE with this string here?

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1071: bool
SearchProvider::LacksValidDefaultMatch(
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: I think this function's body would read more clearly if you reversed it
to
> be "HasValidDefaultMatch()".  I found myself repeatedly confused about how we
> were returning a bool indicating the lack of something.

I chose Lacks...() for consistency with the other constraint functions.  In all
those cases, returning true means the constraint is violated.  Frankly, I think
some of the other names are confusing because of the way they're negated, but
that's outside the scope of this change (which is big enough already).

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1077: // to be the default match.
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: This comment is somewhat confusing.  How about:
> 
>   One of the SearchProvider matches may need to be the overall default.  If
> AutocompleteResult is allowed to reorder matches, this means we simply need at
> least one match in the list to be |allowed_to_be_default|.  If no reordering
is
> possible, however, then our first match needs to set this flag.

Yes, that is clearer.  Used that comment.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1149: // match, and it relies on
SearchProvider to always return one.)
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Given the comments in LacksValidDefaultMatch(), most or all of this
comment
> can go away.

I know it's mostly redundant but I think it's useful here to make reading down
the list of constraints easier.  Again, it's mostly for consistency with
already-existing code.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1155: !IsTopMatchScoreTooLow());
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Indent 4, not even

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1480: (inline_autocomplete_offset
!= string16::npos);
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Eh?  The conditional above has already guaranteed this holds.

Replaced with "true" :-).

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.h:383: bool
omnibox_will_reorder_for_legal_default_match) const;
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: It's slightly confusing to refer to "omnibox" here since this code is
> technically part of the omnibox already.
> 
> Maybe |allow_default_anywhere_in_list|?  I suppose you could do
> |autocomplete_result_can_reorder_default_matches| or something but that seems
> kind of verbose.

Changed to autocomplete_result_will_reorder_for_default_match.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:171: delete
field_trial_list_;
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Can you convert this to a scoped_ptr<> also?

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:258: // matches that
always allowed to be the default match.
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: that -> that are?

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:421: // The what you
typed match should too of course.
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: should too of -> should be too, of

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:177: // Reset relevance scores
to guarantee no result is given a score that may
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: result -> match

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:180: // problem.  In the
process of resetting scores, guarantee that all scores
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: You may want to say why this is a problem (or in what circumstances it's
a
> problem?)

Added a comment.  Not sure if it help or hurts.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:183:
input.current_page_classification())) {
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Indent 4, not 8

Done.

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider_unittest.cc:259: // they score.
On 2013/08/06 22:56:16, Peter Kasting wrote:
> Nit: Should this have a TODO to change this to allow ShortcutsProvider to
> inline?

No.  TODOs in code should be trivial.  I'm not even sure we want to allow them
to ever inline; this can cause a positive feedback loop that might be hard to
overcome depending on the details of the scoring algorithm.  We should create a
bug if you want to discuss it; it any case the issue is bigger than a TODO
within the code.

[Peter Kasting, 2013-08-07 00:49:53]

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1071: bool
SearchProvider::LacksValidDefaultMatch(
On 2013/08/07 00:44:31, Mark P wrote:
> On 2013/08/06 22:56:16, Peter Kasting wrote:
> > Nit: I think this function's body would read more clearly if you reversed it
> to
> > be "HasValidDefaultMatch()".  I found myself repeatedly confused about how
we
> > were returning a bool indicating the lack of something.
> 
> I chose Lacks...() for consistency with the other constraint functions.  In
all
> those cases, returning true means the constraint is violated.  Frankly, I
think
> some of the other names are confusing because of the way they're negated, but
> that's outside the scope of this change (which is big enough already).

I don't see any particular benefit to being consistent in that sense.

I'd make this function Has...(), then have a followup change that reverses
whichever other functions would make more sense reversed.  But if you wanted to
do it all at once that'd be OK too; I just don't really want to leave this
function this way long term because my brain hurts.

[Mark P, 2013-08-07 00:58:51]

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:1071: bool
SearchProvider::LacksValidDefaultMatch(
On 2013/08/07 00:49:53, Peter Kasting wrote:
> On 2013/08/07 00:44:31, Mark P wrote:
> > On 2013/08/06 22:56:16, Peter Kasting wrote:
> > > Nit: I think this function's body would read more clearly if you reversed
it
> > to
> > > be "HasValidDefaultMatch()".  I found myself repeatedly confused about how
> we
> > > were returning a bool indicating the lack of something.
> > 
> > I chose Lacks...() for consistency with the other constraint functions.  In
> all
> > those cases, returning true means the constraint is violated.  Frankly, I
> think
> > some of the other names are confusing because of the way they're negated,
but
> > that's outside the scope of this change (which is big enough already).
> 
> I don't see any particular benefit to being consistent in that sense.
> 
> I'd make this function Has...(),

Done.

> then have a followup change that reverses
> whichever other functions would make more sense reversed.  But if you wanted
to
> do it all at once that'd be OK too; I just don't really want to leave this
> function this way long term because my brain hurts.

[msw, 2013-08-07 20:09:20]

I'm beginning to lose touch with these changes, so feel free to ignore me.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (left):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:512: // Get the
controller's internal members in the correct state.
why is this no longer needed?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:349:
match.relevance = 1000;
where does 1000 come from? why is it needed?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:123: DCHECK((default_match_
== end()) ||
nit: maybe make this matches_.end() like your change on line 127?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result_unittest.cc:318: // Must be the
same as kBundledExperimentFieldTrialName
Is this duplication of constants common/necessary? This file's field trial code
is pretty cryptic and beyond me. Couldn't AutocompleteResult have a simple
boolean member |reorder_for_legal_default_match| or similar, generally set by
some field trial code, but also modifiable for testing without hacking up some
fake field trial settings?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:1069:
match.allowed_to_be_default_match = !params->prevent_inline_autocomplete ||
Shouldn't this match never be default if prevent inline autocomplete is true?
It looks like this allows default inline autocomplete when prevent is true.
But I might be reading this wrong...

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:299:
"http://slashdot.org/favorite_page.html", false),
I have no clue why these aren't legal defaults.
Suggesting "http://slashdot.org/" for "slash" seems ok to me...
Ignore me, I guess... I'm quite confused and won't review this file.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:584: // Because these matches
are sync, we make them score less than the
I don't follow this statement at all... is the first match not returned
synchronously? why do lower scores make the other matches not legal defaults?
Aren't they actually legal, but simply unlikely to be promoted to the default
match so long as the first match is legal? Ignore me, I clearly don't understand
this...

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider_unittest.cc:85:
EXPECT_EQ(keyword_cases[i].num_results, matches.size());
nit: you could make this ASSERT_EQ, then remove the corresponding if.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1126:
EXPECT_EQ(kNotApplicable, cases[i].matches[j].contents) <<
Can you just EXPECT_EQ(kEmptyMatch, cases[i].matches[j]);?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1217: { { "b", false },
{ "a", true }, kEmptyMatch, kEmptyMatch },
This is the first difference I notice from the test above. Does it make sense to
have all the cases duplicated across both tests, since the field trial will
presumably end at some point and then we can choose a single behavior and remove
the duplicated test? Or would it make more sense to split up the tests so the
common pieces are run in both configs, and then the differences can be more
explicitly demonstrated in separate tests or similar?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1221: { { "a", true}, {
"b", false }, kEmptyMatch, kEmptyMatch },
nit: space after true

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1323: // Must be the
same as kBundledExperimentFieldTrialName
Ditto to comment in AutocompleteResultTest.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1363:
EXPECT_EQ(kNotApplicable, cases[i].matches[j].contents) <<
Can you just EXPECT_EQ(kEmptyMatch, cases[i].matches[j]);?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1392: { { "a", true,
true },
nit: consider using space indentation to make the struct fields line up across
lines within the same test case. Ditto for other cases throughout this test.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1424: kEmptyMatch,
nit: consider packing multiple empties onto the same line. (ditto elsewhere)

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1847:
EXPECT_EQ(kNotApplicable, cases[i].matches[j].contents) <<
Can you just EXPECT_EQ(kEmptyMatch, cases[i].matches[j]);?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1988: kEmptyMatch } },
nit: wrap empties here, like in the cases above and below.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:2053:
EXPECT_EQ(kNotApplicable, cases[i].output[j].match_contents);
Can you just EXPECT_EQ(kEmptyMatch, cases[i].output[j]);?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/omnibox/o...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/omnibox/o...
chrome/browser/omnibox/omnibox_field_trial.h:132:
AutocompleteInput::PageClassification current_page_classification);
Why does the page classification matter?

[Mark P, 2013-08-07 22:13:05]

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (left):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:512: // Get the
controller's internal members in the correct state.
On 2013/08/07 20:09:21, msw wrote:
> why is this no longer needed?

It apparently was never needed (or at least not needed for a while).  I needed
to remove it here because this initial RunQuery() call results in a DCHECK
because the input doesn't trigger a keyword mode and this test never sets up a
default search provider, meaning there's no matches (and therefore no default
match).

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:349:
match.relevance = 1000;
On 2013/08/07 20:09:21, msw wrote:
> where does 1000 come from? why is it needed?

It needs to be set to something because nowadays AutocompleteResult filters all
matches with 0 relevance.  1000 is arbitrary.  Added a comment.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:123: DCHECK((default_match_
== end()) ||
On 2013/08/07 20:09:21, msw wrote:
> nit: maybe make this matches_.end() like your change on line 127?

Done.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result_unittest.cc:318: // Must be the
same as kBundledExperimentFieldTrialName
On 2013/08/07 20:09:21, msw wrote:
> Is this duplication of constants common/necessary?

Alexei asked a similar question on a different code review.  Here was
my answer:
> I'd rather not have someone reading the .h have to think about this
>implementation detail, even if it's in a private section and listed as a
friend.
He said "Fair enough, SGTM."

I'm willing to back off if pushed, but that was my reasoning.

> This file's field trial code
> is pretty cryptic and beyond me. Couldn't AutocompleteResult have a simple
> boolean member |reorder_for_legal_default_match| or similar, generally set by
> some field trial code, but also modifiable for testing without hacking up some
> fake field trial settings?

|reorder_for_legal_default_match| changes depending on the input so having
it as a modifiable boolean member seems difficult because the tests will have
to inject the right setting at the right time.  I guess we could have a function
in this class that overrides its value (permanently) for testing.

I actually like this rule-setting code because it tests exactly how the code
is used: we have to put rules in exactly this cryptic form in the field trial
config file.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:1069:
match.allowed_to_be_default_match = !params->prevent_inline_autocomplete ||
On 2013/08/07 20:09:21, msw wrote:
> Shouldn't this match never be default if prevent inline autocomplete is true?
> It looks like this allows default inline autocomplete when prevent is true.
> But I might be reading this wrong...

The latter part of the test effectively says "is there any inline completion". 
If no, then the match is allowed.  Added comment.

I thought writing the test this way would be better than putting something in
the if statement above and then having an else to handle other cases.  (i.e.,
put the allowed_to_be_default_match setting code all in one place)

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:299:
"http://slashdot.org/favorite_page.html", false),
On 2013/08/07 20:09:21, msw wrote:
> I have no clue why these aren't legal defaults.
> Suggesting "http://slashdot.org/" for "slash" seems ok to me...
> Ignore me, I guess... I'm quite confused and won't review this file.

This whole function uses RunTest() calls that have prevent_inline_autocompletion
= true.  That's why these extensions aren't allowed.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:584: // Because these matches
are sync, we make them score less than the
On 2013/08/07 20:09:21, msw wrote:
> I don't follow this statement at all... is the first match not returned
> synchronously? why do lower scores make the other matches not legal defaults?
> Aren't they actually legal, but simply unlikely to be promoted to the default
> match so long as the first match is legal? Ignore me, I clearly don't
understand
> this...

Oops, I think all that confusion came from a typo: sync vs. async.

I think it's clear this comment is actively harmful.  I rewrote it to be closer
to a
version I originally had before Peter asked me to think about expanding it.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider_unittest.cc:85:
EXPECT_EQ(keyword_cases[i].num_results, matches.size());
On 2013/08/07 20:09:21, msw wrote:
> nit: you could make this ASSERT_EQ, then remove the corresponding if.

Done.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1126:
EXPECT_EQ(kNotApplicable, cases[i].matches[j].contents) <<
On 2013/08/07 20:09:21, msw wrote:
> Can you just EXPECT_EQ(kEmptyMatch, cases[i].matches[j]);?

I can't use EXPECT_EQ because the compiler complains.  (The EXPECT_EQ doesn't
have a template that supports arbitrary structs I think.)

Ditto for EXPECT_TRUE ( == ).  [1]

Left as is.  Besides, using .contents makes the failure error message display
the particular contents of the missing entry, which is handy (though I guess I
could just change the << message to include it if I managed to make your
suggestion work).

[1]
../../chrome/browser/autocomplete/search_provider_unittest.cc: In member
function ‘virtual void
SearchProviderTest_DefaultFetcherSuggestRelevance_Test::TestBody()’:
../../chrome/browser/autocomplete/search_provider_unittest.cc:1126:7: error: no
match for ‘operator==’ in
‘cases[i].SearchProviderTest_DefaultFetcherSuggestRelevance_Test::TestBody()::<anonymous
struct>::matches[j] == kEmptyMatch’
../../chrome/browser/autocomplete/search_provider_unittest.cc:1126:7: note:
candidates are:
../../base/memory/scoped_ptr.h:564:6: note: template<class T, class D> bool
operator==(T*, const scoped_ptr<T, D>&)
../../base/memory/scoped_ptr.h:692:6: note: template<class C, class FP> bool
operator==(C*, const scoped_ptr_malloc<C, FP>&)
../../base/memory/linked_ptr.h:164:6: note: template<class T> bool
operator==(T*, const linked_ptr<T>&)

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1217: { { "b", false },
{ "a", true }, kEmptyMatch, kEmptyMatch },
On 2013/08/07 20:09:21, msw wrote:
> This is the first difference I notice from the test above. Does it make sense
to
> have all the cases duplicated across both tests, since the field trial will
> presumably end at some point and then we can choose a single behavior and
remove
> the duplicated test?

I like this approach, duplicating as necessary and then deleting when one
behavior is discarded.

If this were code in the binary itself, I might feel differently, but I don't
mind duplication much in test files.

> Or would it make more sense to split up the tests so the
> common pieces are run in both configs, and then the differences can be more
> explicitly demonstrated in separate tests or similar?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1221: { { "a", true}, {
"b", false }, kEmptyMatch, kEmptyMatch },
On 2013/08/07 20:09:21, msw wrote:
> nit: space after true

Done.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1323: // Must be the
same as kBundledExperimentFieldTrialName
On 2013/08/07 20:09:21, msw wrote:
> Ditto to comment in AutocompleteResultTest.

Noted.  See discussion there.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1363:
EXPECT_EQ(kNotApplicable, cases[i].matches[j].contents) <<
On 2013/08/07 20:09:21, msw wrote:
> Can you just EXPECT_EQ(kEmptyMatch, cases[i].matches[j]);?

See discussion above.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1392: { { "a", true,
true },
On 2013/08/07 20:09:21, msw wrote:
> nit: consider using space indentation to make the struct fields line up across
> lines within the same test case. Ditto for other cases throughout this test.

Will do a later patchset.  Uploading the current patch set so you can read my
replies to / questions about your comments.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1424: kEmptyMatch,
On 2013/08/07 20:09:21, msw wrote:
> nit: consider packing multiple empties onto the same line. (ditto elsewhere)

I went with this approach throughout this file:
If there's only one "real" (non-empty matches), put that on a line, then have
another line with all empty matches.  If there are multiple "real" matches, then
every match (including empty ones) gets its own line.

I tried to be consistent but I may have messed up a few places.  There was a lot
of stuff to mind-numbing revisions here.

Would you prefer put all empty matches on one line (if possible), wrapping as
necessary?

I assume you don't want empties tacked on to the end of a "real" match line even
if there is space.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1847:
EXPECT_EQ(kNotApplicable, cases[i].matches[j].contents) <<
On 2013/08/07 20:09:21, msw wrote:
> Can you just EXPECT_EQ(kEmptyMatch, cases[i].matches[j]);?

See discussion above.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:1988: kEmptyMatch } },
On 2013/08/07 20:09:21, msw wrote:
> nit: wrap empties here, like in the cases above and below.

Done.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:2053:
EXPECT_EQ(kNotApplicable, cases[i].output[j].match_contents);
On 2013/08/07 20:09:21, msw wrote:
> Can you just EXPECT_EQ(kEmptyMatch, cases[i].output[j]);?

See discussion above.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/omnibox/o...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/omnibox/o...
chrome/browser/omnibox/omnibox_field_trial.h:132:
AutocompleteInput::PageClassification current_page_classification);
On 2013/08/07 20:09:21, msw wrote:
> Why does the page classification matter?

We may only want to reorder in certain contexts.  I'm not initially imagining
using context, but I can imagine using it.  For instance, reordering has a
tendency to show URLs more aggressively, something that may not appropriate on a
search results page doing search term replacement (where users are more likely
to want to refine to another query).

[Peter Kasting, 2013-08-07 22:20:01]

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result_unittest.cc:318: // Must be the
same as kBundledExperimentFieldTrialName
On 2013/08/07 22:13:05, Mark P wrote:
> On 2013/08/07 20:09:21, msw wrote:
> > Is this duplication of constants common/necessary?
> 
> Alexei asked a similar question on a different code review.  Here was
> my answer:
> > I'd rather not have someone reading the .h have to think about this
> >implementation detail, even if it's in a private section and listed as a
> friend.

FWIW, I also asked about this kind of thing.

I don't think it's problematic to declare these in a .h and atop them put a
comment like "// Exposed publicly for the sake of unittests."  That makes it
clear that they're not particularly critical to understanding the implementation
unless you want to know how tests interact with the code.  I think this would be
an overall improvement.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:584: // Because these matches
are sync, we make them score less than the
On 2013/08/07 22:13:05, Mark P wrote:
> On 2013/08/07 20:09:21, msw wrote:
> > I don't follow this statement at all... is the first match not returned
> > synchronously? why do lower scores make the other matches not legal
defaults?
> > Aren't they actually legal, but simply unlikely to be promoted to the
default
> > match so long as the first match is legal? Ignore me, I clearly don't
> understand
> > this...
> 
> Oops, I think all that confusion came from a typo: sync vs. async.
> 
> I think it's clear this comment is actively harmful.  I rewrote it to be
closer
> to a
> version I originally had before Peter asked me to think about expanding it.

Your newest version seems noticeably worse.  I disagree with your rewriting. 
How about something like this:

"Because these matches are async, we should never let them become the default
match, lest we introduce race conditions in the omnibox' user interaction."

[Mark P, 2013-08-08 15:22:16]

New snapshot up.  Highlights are:

* exposed the necessary consts from omnibox_field_trial.cc in
omnibox_field_trial.h for use in unit tests to remove duplication.  (After
realizing that multiple people initially thought this was the right approach, I
decided to go with it.  And it does look alright.)

* made a lot of the test data align vertically in search_provider_unittest.cc,
as requested by Mike and promised by me.

* combined multiple kEmptyMatch in search_provider_unittest.cc into one line.  I
initially pushed back on this request from Mike but as I cleaned up
search_provider_unittest.cc I realized this suggestion of his was good. 
Apologies!  I should simply do more of what people suggest in code reviews
rather than listing the (weak) reasons for doing what I did.

Everyone, please take another look.

Mike, for reference, the only two comments in search_provider_unittest.cc that I
didn't end up doing (initially or later) was the EXPECT_EQ(kEmptyMatch...) and
the question about refactoring common parts of the suggested relevance scores
tests.  So those are two comments you may want to reply to.  There might be
other comments to reply to in other files; I didn't re-audit those file when
sending this message.

thanks,
mark

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:584: // Because these matches
are sync, we make them score less than the
On 2013/08/07 22:20:01, Peter Kasting wrote:
> On 2013/08/07 22:13:05, Mark P wrote:
> > On 2013/08/07 20:09:21, msw wrote:
> > > I don't follow this statement at all... is the first match not returned
> > > synchronously? why do lower scores make the other matches not legal
> defaults?
> > > Aren't they actually legal, but simply unlikely to be promoted to the
> default
> > > match so long as the first match is legal? Ignore me, I clearly don't
> > understand
> > > this...
> > 
> > Oops, I think all that confusion came from a typo: sync vs. async.
> > 
> > I think it's clear this comment is actively harmful.  I rewrote it to be
> closer
> > to a
> > version I originally had before Peter asked me to think about expanding it.
> 
> Your newest version seems noticeably worse.  I disagree with your rewriting. 
> How about something like this:
> 
> "Because these matches are async, we should never let them become the default
> match, lest we introduce race conditions in the omnibox' user interaction."

Yeah, that sounds good.  Done.

[H Fung, 2013-08-08 20:10:36]

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:107: if (!empty() &&
!matches_.begin()->allowed_to_be_default_match &&
!matches_.empty() also, to be consistent?

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result_unittest.cc:41:
field_trial_list_.reset();
not needed?

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:220: // match to be the default
match.
I'm not sure if it's worth changing the function name because of this (to
something like CreateWhatYouTypedMatch)

[Mark P, 2013-08-08 22:06:10]

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:107: if (!empty() &&
!matches_.begin()->allowed_to_be_default_match &&
On 2013/08/08 20:10:36, H Fung wrote:
> !matches_.empty() also, to be consistent?

Done.

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result_unittest.cc:41:
field_trial_list_.reset();
On 2013/08/08 20:10:36, H Fung wrote:
> not needed?

Yes, needed.  See comment directly above the line. :-)

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:220: // match to be the default
match.
On 2013/08/08 20:10:36, H Fung wrote:
> I'm not sure if it's worth changing the function name because of this (to
> something like CreateWhatYouTypedMatch)

A reasonable idea.  I think it makes things clearer.  Done.

[Mark P, 2013-08-09 14:37:14]

Rebased this now that the demote-by-type change has landed
https://codereview.chromium.org/22031002/ 

I believe after this rebasing the change is in final form--there's nothing else
that has to be done before it can be submitted.

--mark

[Peter Kasting, 2013-08-09 21:59:34]

LGTM

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:137: typedef
std::pair<std::string, bool> ExpectedUrlAndLegalDefaultMatch;
On 2013/08/07 00:44:31, Mark P wrote:
> On 2013/08/06 22:56:16, Peter Kasting wrote:
> > Tiny nit: Chrome code does both, but personally I prefer "URL" to "Url" in
> type
> > names.
> 
> In this context, I think URL looks better.  I prefer Url when it's at the end
of
> something; if it's in the middle of a camel case I think it can look odd.

Did you reverse URL and Url in your reply, or just fail to update this type
name?

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:181: // and, even when they
are, the KeywordProvider does not bother to set
Nit: Was KeywordProvider intended to be ShortcutsProvider here?

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:349:
match.relevance = 1000;  // arbitrary non-zero value
Nit: Capitalize, punctuate

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:107: if (!empty() &&
!matches_.begin()->allowed_to_be_default_match &&
On 2013/08/08 22:06:10, Mark P wrote:
> On 2013/08/08 20:10:36, H Fung wrote:
> > !matches_.empty() also, to be consistent?
> 
> Done.

Doesn't look changed?

Honestly, I would also be OK with switching everything in this function to using
the member accessors instead of writing "matches_.X" everywhere (it'd be
shorter), just as long as things are consistent.

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:220: // match to be the default
match.
On 2013/08/08 22:06:10, Mark P wrote:
> On 2013/08/08 20:10:36, H Fung wrote:
> > I'm not sure if it's worth changing the function name because of this (to
> > something like CreateWhatYouTypedMatch)
> 
> A reasonable idea.  I think it makes things clearer.  Done.

Does not seem changed?

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/omnibox/o...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/omnibox/o...
chrome/browser/omnibox/omnibox_field_trial.cc:239: // static (just like
everything else in this file)
Nit: I wouldn't just write this here, since no one else does.  If you want to
put it on everything in the file, or (less good) on top of the file, fine,
otherwise I'd just leave it off entirely.

[Mark P, 2013-08-09 22:24:10]

Thanks for the extensive review Peter.

Harry and Mike, I'd love to hear additional comments or a message such as "I've
looked at it to the extent I want to and at least the part I looked at looks
fine to me."  If I don't hear anything, I'll assume you're happy.  (I trust that
Peter gave this a complete review, which is why I don't wait for you guys.)

I'm hoping to submit this over the weekend when the tree and commit queue are
quiet.

--mark

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/176001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:137: typedef
std::pair<std::string, bool> ExpectedUrlAndLegalDefaultMatch;
On 2013/08/09 21:59:34, Peter Kasting wrote:
> On 2013/08/07 00:44:31, Mark P wrote:
> > On 2013/08/06 22:56:16, Peter Kasting wrote:
> > > Tiny nit: Chrome code does both, but personally I prefer "URL" to "Url" in
> > type
> > > names.
> > 
> > In this context, I think URL looks better.  I prefer Url when it's at the
end
> of
> > something; if it's in the middle of a camel case I think it can look odd.
> 
> Did you reverse URL and Url in your reply, or just fail to update this type
> name?

I reversed it in my reply.  I find strings with multiple consequective capital
letters harder to parse.  I keep reading this initially as "land" for some
reason: ExpectedURLAndLegalDefaultMatch

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:181: // and, even when they
are, the KeywordProvider does not bother to set
On 2013/08/09 21:59:34, Peter Kasting wrote:
> Nit: Was KeywordProvider intended to be ShortcutsProvider here?

Yes.  Fixed.

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_provider_unittest.cc:349:
match.relevance = 1000;  // arbitrary non-zero value
On 2013/08/09 21:59:34, Peter Kasting wrote:
> Nit: Capitalize, punctuate

Done.

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:107: if (!empty() &&
!matches_.begin()->allowed_to_be_default_match &&
On 2013/08/09 21:59:34, Peter Kasting wrote:
> On 2013/08/08 22:06:10, Mark P wrote:
> > On 2013/08/08 20:10:36, H Fung wrote:
> > > !matches_.empty() also, to be consistent?
> > 
> > Done.
> 
> Doesn't look changed?

This change disappeared when rebasing.  I noticed it later and uploaded a new
patch, but you reviewed a slightly earlier patch.

> Honestly, I would also be OK with switching everything in this function to
using
> the member accessors instead of writing "matches_.X" everywhere (it'd be
> shorter), just as long as things are consistent.

I changed things to drop matches_. where applicable.  I found it looked funny to
me, making it harder to read  things like std::sort(begin(), end(), ...) to
infer what was actually being sorted.  I decided to leave the matches_ as is in
this function.

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:220: // match to be the default
match.
On 2013/08/09 21:59:34, Peter Kasting wrote:
> On 2013/08/08 22:06:10, Mark P wrote:
> > On 2013/08/08 20:10:36, H Fung wrote:
> > > I'm not sure if it's worth changing the function name because of this (to
> > > something like CreateWhatYouTypedMatch)
> > 
> > A reasonable idea.  I think it makes things clearer.  Done.
> 
> Does not seem changed?

Already did; you didn't review the most recent patch.

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/omnibox/o...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/18878007/diff/211001/chrome/browser/omnibox/o...
chrome/browser/omnibox/omnibox_field_trial.cc:239: // static (just like
everything else in this file)
On 2013/08/09 21:59:34, Peter Kasting wrote:
> Nit: I wouldn't just write this here, since no one else does.  If you want to
> put it on everything in the file, or (less good) on top of the file, fine,
> otherwise I'd just leave it off entirely.

Okay, dropped.

[H Fung, 2013-08-09 22:27:34]

lgtm

I have no more comments, thanks.

[msw, 2013-08-10 17:32:17]

LGTM with nits and minor/test comments. I'll take a look at any changes and
comment if needed for followup, but this is good enough to land that you don't
have to wait for another round of review.

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/omnibox/o...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/omnibox/o...
chrome/browser/omnibox/omnibox_field_trial.h:132:
AutocompleteInput::PageClassification current_page_classification);
On 2013/08/07 22:13:05, Mark P wrote:
> On 2013/08/07 20:09:21, msw wrote:
> > Why does the page classification matter?
> 
> We may only want to reorder in certain contexts.  I'm not initially imagining
> using context, but I can imagine using it.  For instance, reordering has a
> tendency to show URLs more aggressively, something that may not appropriate on
a
> search results page doing search term replacement (where users are more likely
> to want to refine to another query).

I'd remove the arg for simplicity if it doesn't have an imminent planned use.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:182: DCHECK((default_match_
== matches_.end()) ||
nit: Use matches_.empty(), not default_match_ == matches_.end(); line 186 too.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:1070: // empty?".
I guess with |inline_autocomplete_offset != string16::npos| it's really saying
"does fill-into-edit match input without any inline autocompletion", aka "it's
verbatim", right?

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:297: const
ExpectedUrlAndLegalDefaultMatch expected_nonsynth[] = {
Use curly-brace initialization throughout these tests, as dropping the highly
repetitive, long "ExpectedUrlAndLegalDefaultMatch(" explicit ctor invocations
should make everything look a lot cleaner; you'll probably need to make
ExpectedUrlAndLegalDefaultMatch a struct (not a pair). Also shortening the name
to something like "UrlAndLegalDefault" with a struct/pair comment more
explicitly stating the meaning and usage would make fewer lines have to wrap.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:218: // A what-you-typed match
is allowed to be the default match.
nit: verbatim.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.h (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.h:87: AutocompleteMatch
CreateWhatYouTypedMatch(const string16& text,
nit: I'll always favor Verbatim over WhatYouTyped, but I guess both are used.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider_unittest.cc:208: kEmptyMatch,
nit: I think these empties could be combined too (and lines 212-213)

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:2169: true },
optional nit: if you reduce the indentation between the input and the match URL,
you could prevent some wrapping here and below. It's nice that they all line up,
but reducing the indent uniformly across for certain blocks of cases, like this
"// Inline matches with valid..." block of 6 cases seems fine. I'll leave it up
to you; truly optional.

[Mark P, 2013-08-11 03:22:47]

I took most of your suggestions.  Lots of tests look prettier and shorter now,
with cleaner lines.

--mark

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/omnibox/o...
File chrome/browser/omnibox/omnibox_field_trial.h (right):

https://codereview.chromium.org/18878007/diff/195001/chrome/browser/omnibox/o...
chrome/browser/omnibox/omnibox_field_trial.h:132:
AutocompleteInput::PageClassification current_page_classification);
On 2013/08/10 17:32:18, msw wrote:
> On 2013/08/07 22:13:05, Mark P wrote:
> > On 2013/08/07 20:09:21, msw wrote:
> > > Why does the page classification matter?
> > 
> > We may only want to reorder in certain contexts.  I'm not initially
imagining
> > using context, but I can imagine using it.  For instance, reordering has a
> > tendency to show URLs more aggressively, something that may not appropriate
on
> a
> > search results page doing search term replacement (where users are more
likely
> > to want to refine to another query).
> 
> I'd remove the arg for simplicity if it doesn't have an imminent planned use.

I'm afraid removing the arg, though it would make this API simpler, would either
require more code or a more complex configuration language.  Note below that
GetValueForRuleInContext(), which is the basic function to lookup config
information about what experiments are active, takes |page_classification|.  If
you look at the comment by its implementation, you'll note that that's an
expected part of the configuration language.  So, if I want to remove the
parameter from ReorderForLegalDefaultMatch(), I have two options:
* have the implementation of this function call GetValueForRuleInContext() with
a dummy page classification, and expect people who write configs to know what to
use as the dummy value for the reorder rule.
* have the implementation of this function do it own lookup of the rule (with no
page classification context) and therefore have people who write a config have
to remember two different formats for the config, one format for most rules and
one format for the reorder rule.

I don't like either of these options much.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_result.cc:182: DCHECK((default_match_
== matches_.end()) ||
On 2013/08/10 17:32:18, msw wrote:
> nit: Use matches_.empty(), not default_match_ == matches_.end(); line 186 too.

I'd prefer to leave this as it is because it's meant to be an assertion about
the validity of default_match_.  (I can imagine that default_match_ could end up
being an invalid iterator, and I don't want to have this CHECK pass if it's
invalid, which could happen if I change the == end() to !empty().  Obviously it
can't happen with line 181 immediately before this check, but still...)

I can move the DCHECK to line 188 for it to be more obviously a post-condition
for this function.  It would also look less silly there.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:1070: // empty?".
On 2013/08/10 17:32:18, msw wrote:
> I guess with |inline_autocomplete_offset != string16::npos| it's really saying
> "does fill-into-edit match input without any inline autocompletion", aka "it's
> verbatim", right?

Yes, added more to the comment to make it obvious.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider_unittest.cc:297: const
ExpectedUrlAndLegalDefaultMatch expected_nonsynth[] = {
On 2013/08/10 17:32:18, msw wrote:
> Use curly-brace initialization throughout these tests, as dropping the highly
> repetitive, long "ExpectedUrlAndLegalDefaultMatch(" explicit ctor invocations
> should make everything look a lot cleaner; you'll probably need to make
> ExpectedUrlAndLegalDefaultMatch a struct (not a pair). Also shortening the
name
> to something like "UrlAndLegalDefault" with a struct/pair comment more
> explicitly stating the meaning and usage would make fewer lines have to wrap.

Yes, it looks much shorter this way.  Done.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.cc:218: // A what-you-typed match
is allowed to be the default match.
On 2013/08/10 17:32:18, msw wrote:
> nit: verbatim.

Done.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider.h (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider.h:87: AutocompleteMatch
CreateWhatYouTypedMatch(const string16& text,
On 2013/08/10 17:32:18, msw wrote:
> nit: I'll always favor Verbatim over WhatYouTyped, but I guess both are used.

Done.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/keyword_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/keyword_provider_unittest.cc:208: kEmptyMatch,
On 2013/08/10 17:32:18, msw wrote:
> nit: I think these empties could be combined too (and lines 212-213)

Done.

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/18878007/diff/260001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider_unittest.cc:2169: true },
On 2013/08/10 17:32:18, msw wrote:
> optional nit: if you reduce the indentation between the input and the match
URL,
> you could prevent some wrapping here and below. It's nice that they all line
up,
> but reducing the indent uniformly across for certain blocks of cases, like
this
> "// Inline matches with valid..." block of 6 cases seems fine. I'll leave it
up
> to you; truly optional.

Done here and below.

[Mark P, 2013-08-12 13:41:45]

Committed patchset #55 manually as r216986 (presubmit successful).