Omnibox: Don't Promote Intranet Hostnames Pound Sign Space Whatever ("c# foo")

Omnibox: Don't Promote Intranet Hostnames Pound Sign Space Whatever ("c# foo")

Suppose you have an intranet hostname "c" that you've previously visited.

After this change, the behavior will be as follows:
input  ->  inline suggestion (what happens what you hit return)
c -> navigate to URL
c/ -> navigate to URL
c#foo -> search (this behavior differs from before this change)
c# foo -> search (this behavior differs from before this change)
c#  foo -> search (this behavior differs from before this change)
c# -> search (this behavior differs from before this change)
c#  -> search (this behavior differs from before this change) (notice the extra spaces the URL parser drops them)
c/#foo -> navigate to URL

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

[Mark P, 2013-04-01 23:55:40]

Please take a look.

Also, what do you think we should do for "c#" itself?

--mark

[Peter Kasting, 2013-04-02 21:05:46]

On 2013/04/01 23:55:40, Mark P wrote:
> Please take a look.
> 
> Also, what do you think we should do for "c#" itself?

"c#" should search.

"c#foo" should also search.  See more detailed comment on bug.

[Mark P, 2013-04-02 23:07:50]

On 2013/04/02 21:05:46, Peter Kasting wrote:
> On 2013/04/01 23:55:40, Mark P wrote:
> > Please take a look.
> > 
> > Also, what do you think we should do for "c#" itself?
> 
> "c#" should search.
> 
> "c#foo" should also search.  See more detailed comment on bug.

Did both.

Please take another look.

--mark

[Peter Kasting, 2013-04-03 00:21:29]

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

https://codereview.chromium.org/13190020/diff/4001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:750: false) == 0) &&
Why do we have to check the registry length?  Doesn't UNVISITED_INTRANET already
get us this?

https://codereview.chromium.org/13190020/diff/4001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:752: (url.path().empty() ||
(url.path() == "/")) &&
For the port and path checks, it'd be nice to avoid checking cases which never
happen.  For example, if the user doesn't specify a port, do we get the empty
port, or -1?  Presumably it's always one and not the other, right?

https://codereview.chromium.org/13190020/diff/4001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:756: // If the
what-you-typed result looks like a single word (which
Nit: Put this comment above the conditional instead of inside it.  (We should do
this with the comment block just below, too.)

https://codereview.chromium.org/13190020/diff/4001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:759: // as is and hope the
top result ends up being a
Nit: This sounds a bit sketchy when you say "hope".  Aren't we guaranteed that
for non-URL input types, if we don't promote the exact match, we'll either get
an inline autocompletion or else some type of search ranking above this?  If so
we don't really need to "hope", we can rely on this.

[Mark P, 2013-04-03 16:08:01]

Revised as suggested.  Please take another look.

--mark

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

https://codereview.chromium.org/13190020/diff/4001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:750: false) == 0) &&
On 2013/04/03 00:21:29, Peter Kasting wrote:
> Why do we have to check the registry length?  Doesn't UNVISITED_INTRANET
already
> get us this?

You are correct.  Removed.

https://codereview.chromium.org/13190020/diff/4001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:752: (url.path().empty() ||
(url.path() == "/")) &&
On 2013/04/03 00:21:29, Peter Kasting wrote:
> For the port and path checks, it'd be nice to avoid checking cases which never
> happen.  For example, if the user doesn't specify a port, do we get the empty
> port, or -1?  Presumably it's always one and not the other, right?

Done.

https://codereview.chromium.org/13190020/diff/4001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:756: // If the
what-you-typed result looks like a single word (which
On 2013/04/03 00:21:29, Peter Kasting wrote:
> Nit: Put this comment above the conditional instead of inside it.  (We should
do
> this with the comment block just below, too.)

Done.

https://codereview.chromium.org/13190020/diff/4001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:759: // as is and hope the
top result ends up being a
On 2013/04/03 00:21:29, Peter Kasting wrote:
> Nit: This sounds a bit sketchy when you say "hope".  Aren't we guaranteed that
> for non-URL input types, if we don't promote the exact match, we'll either get
> an inline autocompletion or else some type of search ranking above this?  If
so
> we don't really need to "hope", we can rely on this.

We're guaranteed to be a search query above this (assuming SearchProvider is
enabled).  Rephrased to eliminate the wishy-washy-ness.

[Peter Kasting, 2013-04-03 21:43:49]

LGTM

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

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:749: // result be a search
query from SearchProvider. This test fixes cases
Nit: be -> will be, from -> from the?

This sounds maybe inaccurate though, can't we have an inline autocompletion of a
non-exact match outscore both things?

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:751: // site "c".  We want
the search-what-you-typed to beat the
Nit: to beat -> score to beat?

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:752: // URL-what-you-typed
in this case.  Most of the below test tries to make
Nit: in -> score in?

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:755: // pass the test
because those will be classified as input type URL.  The
Nit: those -> that

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:765: (url.path() == "/") &&
url.query().empty() &&
Nit: Fractionally faster: url.path().length() == 1

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:778: if (type ==
UNVISITED_INTRANET && !matches->empty()) {
Nit: No {}

[Mark P, 2013-04-03 22:58:18]

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

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:749: // result be a search
query from SearchProvider. This test fixes cases
On 2013/04/03 21:43:49, Peter Kasting wrote:
> Nit: be -> will be, from -> from the?

Done.  (x2)

> This sounds maybe inaccurate though, can't we have an inline autocompletion of
a
> non-exact match outscore both things?

Correct.  Rewrote part of the sentence.

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:751: // site "c".  We want
the search-what-you-typed to beat the
On 2013/04/03 21:43:49, Peter Kasting wrote:
> Nit: to beat -> score to beat?

Okay.

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:752: // URL-what-you-typed
in this case.  Most of the below test tries to make
On 2013/04/03 21:43:49, Peter Kasting wrote:
> Nit: in -> score in?

Done.

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:755: // pass the test
because those will be classified as input type URL.  The
On 2013/04/03 21:43:49, Peter Kasting wrote:
> Nit: those -> that

Done.

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:765: (url.path() == "/") &&
url.query().empty() &&
On 2013/04/03 21:43:49, Peter Kasting wrote:
> Nit: Fractionally faster: url.path().length() == 1

A seriously less readable.  It also makes me nervous (for no good reason).  I'd
strongly prefer to leave it as is.

https://codereview.chromium.org/13190020/diff/7001/chrome/browser/autocomplet...
chrome/browser/autocomplete/history_url_provider.cc:778: if (type ==
UNVISITED_INTRANET && !matches->empty()) {
On 2013/04/03 21:43:49, Peter Kasting wrote:
> Nit: No {}

Done.

[commit-bot: I haz the power, 2013-04-05 14:45:40]

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

[commit-bot: I haz the power, 2013-04-05 19:54:13]

Change committed as 192615