Omnibox: Fix URL-What-You-Typed Allowed-To-Be-Default-Match Issues

Omnibox: Fix URL-What-You-Typed Allowed-To-Be-Default-Match Issues

The two linked bugs both describe situations where the user types
a URL but that URL does not appear first because the top-scoring
provider that returned the URL set it incorrectly as not allowed to
be the default match.

This changes fixes those issues by making providers (namely
HistoryQuick, Search, and Shortcuts) mark a match as
allowed_to_be_default_match if it is UWYT match for this input, or (even
if not identical) it would be dedupped against the UWYT match.
The changes to HistoryQuick and Search fix the attached
bugs.  Shortcuts does the identical part of the test, not the de-dupping part
of the test.  The Shortcuts change to handle dedupping is not to fix a
known bug; rather it's simply to make Shortcuts behave as it should
and in parallel with the other providers.

I don't think changes in other providers beyond these three are
necessary.

This change necessitated some changes to AutocompleteMatch to make a
statically-available version of ComputeStrippedDestinationURL() so we
can strip URLs (such as the UWYT URL) that isn't going to be a
AutocompleteMatch in the relevant provider.

Also removes the IsInlineable() function in BaseSearchProvider::Result,
which is only used in one place and not actually necessary in that place.
(It's computing the same thing that code there is already computing.)

BUG=383065, 388397
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282669

[Mark P, 2014-06-27 21:32:11]

Hi Peter,

Here's the solution we discussed.  Please take a look.  If you like it, I'll add
tests where possible.

thanks,
mark

[Peter Kasting, 2014-06-30 23:02:58]

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.cc:350: const GURL& url, Profile*
profile, const base::string16& keyword) {
Nit: One arg per line

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.cc:434:
allow_fallback_to_destination_host ? destination_url.host()
Nit: Break after '?' instead of before ':'

(aka I suggest not running clang-format on omnibox changes)

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.h:171: // Returns |url| altered
by stripping off "www.", converting https protocol
Nit: https protocol to http -> "https" to "http"

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.h:181: Profile* profile,
Pass a TemplateURLService* here instead (see
https://codereview.chromium.org/354773002/ for the parallel change that is
making ComputeStrippedDestinationURL() take a TemplateURLService*; you may want
to wait for that to land first).

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.h:226: static TemplateURL*
GetTemplateURLWithKeyword(
Nit: Put this with the other static functions.

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (left):

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:420:
URLPrefix::BestURLPrefix(base::UTF8ToUTF16(url_.spec()), input) != NULL;
Can we remove any #includes due to deleting this code (or the code above)?

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

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:254:
url_fixer::FixupURL(base::UTF16ToUTF8(match.fill_into_edit),
Why are we computing the stripped fixed-up fill_into_edit, instead of just
looking at |match|'s stripped destination URL as the other providers you've
changed do?

[Mark P, 2014-06-30 23:27:37]

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.cc:350: const GURL& url, Profile*
profile, const base::string16& keyword) {
On 2014/06/30 23:02:58, Peter Kasting wrote:
> Nit: One arg per line

Done.

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.cc:434:
allow_fallback_to_destination_host ? destination_url.host()
On 2014/06/30 23:02:58, Peter Kasting wrote:
> Nit: Break after '?' instead of before ':'
> 
> (aka I suggest not running clang-format on omnibox changes)

Done.

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.h:171: // Returns |url| altered
by stripping off "www.", converting https protocol
On 2014/06/30 23:02:58, Peter Kasting wrote:
> Nit: https protocol to http -> "https" to "http"

Done.

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.h:181: Profile* profile,
On 2014/06/30 23:02:58, Peter Kasting wrote:
> Pass a TemplateURLService* here instead (see
> https://codereview.chromium.org/354773002/ for the parallel change that is
> making ComputeStrippedDestinationURL() take a TemplateURLService*; you may
want
> to wait for that to land first).

I will wait for that to land and then make the corresponding change here. 
Please tell me when that happens.

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.h:226: static TemplateURL*
GetTemplateURLWithKeyword(
On 2014/06/30 23:02:58, Peter Kasting wrote:
> Nit: Put this with the other static functions.

Done.

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (left):

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:420:
URLPrefix::BestURLPrefix(base::UTF8ToUTF16(url_.spec()), input) != NULL;
On 2014/06/30 23:02:58, Peter Kasting wrote:
> Can we remove any #includes due to deleting this code (or the code above)?

I just checked.  The answer is no.

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

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:254:
url_fixer::FixupURL(base::UTF16ToUTF8(match.fill_into_edit),
On 2014/06/30 23:02:58, Peter Kasting wrote:
> Why are we computing the stripped fixed-up fill_into_edit, instead of just
> looking at |match|'s stripped destination URL as the other providers you've
> changed do?

Because when I originally wrote this code, you insisted that we set
allowed_to_be_default_match using the URL indicated by fill_into_edit and not
match's destination URL.  When I wrote this revision, I kept that property.

If you wish to take back that assertion, I can comply.

[Peter Kasting, 2014-06-30 23:31:25]

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

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:254:
url_fixer::FixupURL(base::UTF16ToUTF8(match.fill_into_edit),
On 2014/06/30 23:27:36, Mark P wrote:
> On 2014/06/30 23:02:58, Peter Kasting wrote:
> > Why are we computing the stripped fixed-up fill_into_edit, instead of just
> > looking at |match|'s stripped destination URL as the other providers you've
> > changed do?
> 
> Because when I originally wrote this code, you insisted that we set
> allowed_to_be_default_match using the URL indicated by fill_into_edit and not
> match's destination URL.  When I wrote this revision, I kept that property.
> 
> If you wish to take back that assertion, I can comply.

I think it doesn't make sense to be inconsistent.  Some providers use the
destination URL, this one uses the fill_into_edit.  When we talked about this
last week, my memory is that we eventually agreed that the destination URL was
the right thing to use.  If so, we should use it here too.

This may make the code here similar enough to the other two places that you can
hoist all three into AutocompleteMatch, though I'm not sure.

[Mark P, 2014-07-01 17:45:33]

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

https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/shortcuts_provider.cc:254:
url_fixer::FixupURL(base::UTF16ToUTF8(match.fill_into_edit),
On 2014/06/30 23:31:25, Peter Kasting wrote:
> On 2014/06/30 23:27:36, Mark P wrote:
> > On 2014/06/30 23:02:58, Peter Kasting wrote:
> > > Why are we computing the stripped fixed-up fill_into_edit, instead of just
> > > looking at |match|'s stripped destination URL as the other providers
you've
> > > changed do?
> > 
> > Because when I originally wrote this code, you insisted that we set
> > allowed_to_be_default_match using the URL indicated by fill_into_edit and
not
> > match's destination URL.  When I wrote this revision, I kept that property.
> > 
> > If you wish to take back that assertion, I can comply.
> 
> This may make the code here similar enough to the other two places that you
can
> hoist all three into AutocompleteMatch, though I'm not sure.

Yes, the code looks very similar.  I can refactor it into a helper function in
AutocompleteMatch if you want (EnsureUWYTIsAllowedToBeDefault()).  I thought for
a bit about whether I can put this code somewhere so that it gets called
automatically on all matches from all providers, which would be nice, but I
think this code needs to be called within the providers so providers can react
to the results of it.  e.g., SearchProvider will know about the correct
allowed_to_be_default_match state when checking constraints.

Sounds good?

[Peter Kasting, 2014-07-01 18:01:14]

On 2014/07/01 17:45:33, Mark P wrote:
>
https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
> File chrome/browser/autocomplete/shortcuts_provider.cc (right):
> 
>
https://codereview.chromium.org/353223002/diff/20001/chrome/browser/autocompl...
> chrome/browser/autocomplete/shortcuts_provider.cc:254:
> url_fixer::FixupURL(base::UTF16ToUTF8(match.fill_into_edit),
> On 2014/06/30 23:31:25, Peter Kasting wrote:
> > On 2014/06/30 23:27:36, Mark P wrote:
> > > On 2014/06/30 23:02:58, Peter Kasting wrote:
> > > > Why are we computing the stripped fixed-up fill_into_edit, instead of
just
> > > > looking at |match|'s stripped destination URL as the other providers
> you've
> > > > changed do?
> > > 
> > > Because when I originally wrote this code, you insisted that we set
> > > allowed_to_be_default_match using the URL indicated by fill_into_edit and
> not
> > > match's destination URL.  When I wrote this revision, I kept that
property.
> > > 
> > > If you wish to take back that assertion, I can comply.
> > 
> > This may make the code here similar enough to the other two places that you
> can
> > hoist all three into AutocompleteMatch, though I'm not sure.
> 
> Yes, the code looks very similar.  I can refactor it into a helper function in
> AutocompleteMatch if you want (EnsureUWYTIsAllowedToBeDefault()).  I thought
for
> a bit about whether I can put this code somewhere so that it gets called
> automatically on all matches from all providers, which would be nice, but I
> think this code needs to be called within the providers so providers can react
> to the results of it.  e.g., SearchProvider will know about the correct
> allowed_to_be_default_match state when checking constraints.
> 
> Sounds good?

Sounds OK to me.

[Mark P, 2014-07-07 23:25:59]

Peter,

I refactored this to use a common function that lives in AutocompleteMatch as we
discussed.  I also added tests.

Please take a look.

thanks,
mark

[Peter Kasting, 2014-07-09 00:46:43]

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:352: TemplateURL* template_url
= keyword.empty() ? NULL :
Nit: Break after '?' instead of ':'

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:354: if (template_url == NULL
&& !host.empty()) {
Nit: No {}

Simpler:

  return (template_url || host.empty()) ?
      template_url : template_url_service->GetTemplateURLForHost(host);

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.h:188: // other than the search
terms themselves that would otherwise prevent from
Nit: from -> doing

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.h:201: // the AutocompleteInput
interpreted as a URL (i.e., canonicalized_url()).
Nit: canonicalized_url() -> AutocompleteInput::canonicalized_url()

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/search_provider_unittest.cc:2211: true },
Hmm.  This seems like a bad outcome.  I think if the user explicitly types a
scheme, we should only allow to be default a match that matches that scheme.

I wonder if there are already deduping bugs as a result of this -- like, if the
HUP returns both http://foo.com (because you typed it) and https://foo.com
(because you visited it), maybe one gets deduped to the other.  Or maybe we're
relying today on "no provider will suggest the latter when you type the former",
in which case we should just ensure this test tests what providers could
actually do.

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_provider.cc:146: const base::string16
fixed_up_input(FixupUserInput(input).second);
Tiny nit: Really we should perhaps be computing these just below max_relevance
(since they're used after it is) and there shouldn't be any blank lines in that
region (to indicate that all three variables are part of the same block as the
loop).

[Mark P, 2014-07-09 19:54:17]

New snapup, and replies (including one long one) to your comments.

--mark

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:352: TemplateURL* template_url
= keyword.empty() ? NULL :
On 2014/07/09 00:46:43, Peter Kasting wrote:
> Nit: Break after '?' instead of ':'

Done.

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:354: if (template_url == NULL
&& !host.empty()) {
On 2014/07/09 00:46:43, Peter Kasting wrote:
> Nit: No {}
> 
> Simpler:
> 
>   return (template_url || host.empty()) ?
>       template_url : template_url_service->GetTemplateURLForHost(host);

Okay.  Done.

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.h:188: // other than the search
terms themselves that would otherwise prevent from
On 2014/07/09 00:46:43, Peter Kasting wrote:
> Nit: from -> doing

Done.

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.h:201: // the AutocompleteInput
interpreted as a URL (i.e., canonicalized_url()).
On 2014/07/09 00:46:43, Peter Kasting wrote:
> Nit: canonicalized_url() -> AutocompleteInput::canonicalized_url()

Done.

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/search_provider_unittest.cc:2211: true },
On 2014/07/09 00:46:43, Peter Kasting wrote:
> Hmm.  This seems like a bad outcome.  I think if the user explicitly types a
> scheme, we should only allow to be default a match that matches that scheme.

I agree.  Yet [see below]

> I wonder if there are already deduping bugs as a result of this -- like, if
the
> HUP returns both http://foo.com (because you typed it) and https://foo.com
> (because you visited it), maybe one gets deduped to the other.  Or maybe we're
> relying today on "no provider will suggest the latter when you type the
former",
> in which case we should just ensure this test tests what providers could
> actually do.

You're right: we're already doing dedupping like this.  I think what's saving us
is that generally if the user types http://whatever, no provider is likely to
suggest https://whatever, and vice versa.

Do you want me to remove the tests in this unit test that involve scheme
changes?

Also, if you want, I could add something to the autocomplete_result.cc code:
      if (AutocompleteMatch::IsSearchType(default_match_->type)) {
        DCHECK_NE(metrics::OmniboxInputType::URL, input.type()) << debug_info;
      } else {
        DCHECK_NE(metrics::OmniboxInputType::FORCED_QUERY, input.type())
            << debug_info;
      }
I could add a block within the else clause that checks
  if ((input.type() == URL) && !LowerCaseEqualsASCII(input.scheme(),
url::kHttpScheme)) {
    DCHECK_EQ(input.canonicalized_url().scheme(),
default_match_->destination_url.scheme());
  }
i.e., if the user explicitly typed a non-http scheme, then make sure the
destination_url keeps the same scheme.
(input.scheme() may be http if the user typed it or if the user didn't type a
scheme at all.  FixupURL and Parse infer the http scheme on inputs where no
exist.  So non-http means the user must've explicitly typed a scheme.)
(It would be harder to simply say "if user explicitly typed a scheme then make
sure the destination URL scheme matches."  Also, I don't think the http->https
transition is a problem, which is probably the most likely transition that comes
from http.)
Frankly, I don't think this test will catch much if anything but I can add it if
you want.

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/shortcuts_provider.cc (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/shortcuts_provider.cc:146: const base::string16
fixed_up_input(FixupUserInput(input).second);
On 2014/07/09 00:46:43, Peter Kasting wrote:
> Tiny nit: Really we should perhaps be computing these just below max_relevance
> (since they're used after it is) and there shouldn't be any blank lines in
that
> region (to indicate that all three variables are part of the same block as the
> loop).

Done.

[Peter Kasting, 2014-07-09 21:05:03]

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/search_provider_unittest.cc:2211: true },
On 2014/07/09 19:54:17, Mark P wrote:
> Do you want me to remove the tests in this unit test that involve scheme
> changes?

Yeah, I think we should.

> Also, if you want, I could add something to the autocomplete_result.cc code:
>       if (AutocompleteMatch::IsSearchType(default_match_->type)) {
>         DCHECK_NE(metrics::OmniboxInputType::URL, input.type()) << debug_info;
>       } else {
>         DCHECK_NE(metrics::OmniboxInputType::FORCED_QUERY, input.type())
>             << debug_info;
>       }
> I could add a block within the else clause that checks
>   if ((input.type() == URL) && !LowerCaseEqualsASCII(input.scheme(),
> url::kHttpScheme)) {
>     DCHECK_EQ(input.canonicalized_url().scheme(),
> default_match_->destination_url.scheme());
>   }
> i.e., if the user explicitly typed a non-http scheme, then make sure the
> destination_url keeps the same scheme.
> (input.scheme() may be http if the user typed it or if the user didn't type a
> scheme at all.  FixupURL and Parse infer the http scheme on inputs where no
> exist.  So non-http means the user must've explicitly typed a scheme.)
> (It would be harder to simply say "if user explicitly typed a scheme then make
> sure the destination URL scheme matches."  Also, I don't think the http->https
> transition is a problem, which is probably the most likely transition that
comes
> from http.)

I think the only test that would make sense to add is the one you note would be
harder: if the user explicitly typed a scheme, ensure the default match has that
scheme.

I don't know why that would be hard to implement, though.  Don't you just need
to check if AutocompleteInput::parts.scheme.is_nonempty()?  As for defaulting to
https when the user explicitly typed http://... hmm.  I feel like this should
probably be an error; the case where it might not is if the host is HSTS-pinned,
and somehow the omnibox code knows that (it doesn't today).

[Mark P, 2014-07-09 22:31:37]

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/search_provider_unittest.cc (right):

https://codereview.chromium.org/353223002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/search_provider_unittest.cc:2211: true },
On 2014/07/09 21:05:03, Peter Kasting wrote:
> On 2014/07/09 19:54:17, Mark P wrote:
> > Do you want me to remove the tests in this unit test that involve scheme
> > changes?
> 
> Yeah, I think we should.

Done.

> > Also, if you want, I could add something to the autocomplete_result.cc code:
> >       if (AutocompleteMatch::IsSearchType(default_match_->type)) {
> >         DCHECK_NE(metrics::OmniboxInputType::URL, input.type()) <<
debug_info;
> >       } else {
> >         DCHECK_NE(metrics::OmniboxInputType::FORCED_QUERY, input.type())
> >             << debug_info;
> >       }
> > I could add a block within the else clause that checks
> >   if ((input.type() == URL) && !LowerCaseEqualsASCII(input.scheme(),
> > url::kHttpScheme)) {
> >     DCHECK_EQ(input.canonicalized_url().scheme(),
> > default_match_->destination_url.scheme());
> >   }
> > i.e., if the user explicitly typed a non-http scheme, then make sure the
> > destination_url keeps the same scheme.
> > (input.scheme() may be http if the user typed it or if the user didn't type
a
> > scheme at all.  FixupURL and Parse infer the http scheme on inputs where no
> > exist.  So non-http means the user must've explicitly typed a scheme.)
> > (It would be harder to simply say "if user explicitly typed a scheme then
make
> > sure the destination URL scheme matches."  Also, I don't think the
http->https
> > transition is a problem, which is probably the most likely transition that
> comes
> > from http.)
> 
> I think the only test that would make sense to add is the one you note would
be
> harder: if the user explicitly typed a scheme, ensure the default match has
that
> scheme.
> 
> I don't know why that would be hard to implement, though.  Don't you just need
> to check if AutocompleteInput::parts.scheme.is_nonempty()?

Oh, I guess you're right.  I assumed that parts would correspond to
canonicalized_url and have lost memory of what was originally empty or
non-empty.  i.e., I noticed that scheme() in autocomplete_input is set (to
"http") even when not typed explicitly.  I assumed autocomplete_input's parts
would agree.  Apparently it doesn't so I can do what we both want.

Done.

> As for defaulting to
> https when the user explicitly typed http://... hmm.  I feel like this should
> probably be an error; the case where it might not is if the host is
HSTS-pinned,
> and somehow the omnibox code knows that (it doesn't today).

[Peter Kasting, 2014-07-09 22:36:00]

LGTM

https://codereview.chromium.org/353223002/diff/140001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/353223002/diff/140001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_result.cc:272:
DCHECK_EQ(base::UTF16ToUTF8(base::i18n::ToLower(input.scheme())),
This isn't already lowercased for us?

https://codereview.chromium.org/353223002/diff/140001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/353223002/diff/140001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider.cc:12: #include
"base/i18n/case_conversion.h"
Nit: Is this needed?

[Mark P, 2014-07-09 23:12:23]

https://codereview.chromium.org/353223002/diff/140001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_result.cc (right):

https://codereview.chromium.org/353223002/diff/140001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_result.cc:272:
DCHECK_EQ(base::UTF16ToUTF8(base::i18n::ToLower(input.scheme())),
On 2014/07/09 22:36:00, Peter Kasting wrote:
> This isn't already lowercased for us?

Right you are.  Done.

https://codereview.chromium.org/353223002/diff/140001/chrome/browser/autocomp...
File chrome/browser/autocomplete/history_quick_provider.cc (right):

https://codereview.chromium.org/353223002/diff/140001/chrome/browser/autocomp...
chrome/browser/autocomplete/history_quick_provider.cc:12: #include
"base/i18n/case_conversion.h"
On 2014/07/09 22:36:00, Peter Kasting wrote:
> Nit: Is this needed?

Not anymore.  Removed.

[Mark P, 2014-07-09 23:12:30]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-07-09 23:13:53]

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

[commit-bot: I haz the power, 2014-07-10 03:49:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-10 04:56:02]

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

[commit-bot: I haz the power, 2014-07-10 04:56:02]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[Mark P, 2014-07-11 17:38:35]

The failure was real.  The scheme equality was a little too aggressive for
about: schemes.  (about:blank and about: were particular offenders, where the
canonicalized input scheme remains "about" yet the destination scheme is
"chrome".)  I revised the DCHECK.

[Mark P, 2014-07-11 17:38:43]

The CQ bit was checked by mpearson@chromium.org

[commit-bot: I haz the power, 2014-07-11 17:40:04]

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

[commit-bot: I haz the power, 2014-07-11 19:46:39]

Change committed as 282669