Fixes bug 12305 -- 1.66:1 should be UNKNOWN, not URL.

Fixes bug 12305 -- 1.66:1 should be UNKNOWN, not URL.

Rearranges the autocomplete heuristics to reject malformed IP addresses before accepting them based on having a port or a username or a password.

BUG=12305
TEST=AutocompleteTest.InputType

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54977

[isherman, 2010-07-30 01:17:38]

This is my first patch, so apologies in advance if I did something trivially
foolish.

[Peter Kasting, 2010-07-30 01:37:41]

http://codereview.chromium.org/2868085/diff/1/2
File chrome/browser/autocomplete/autocomplete.cc (left):

http://codereview.chromium.org/2868085/diff/1/2#oldcode244
chrome/browser/autocomplete/autocomplete.cc:244: // Presence of a username could
either indicate a URL or an email address
I don't think we should move the username/password checks below the IP check. 
"user:pass@1.2" seems more likely a URL than a query to me.

http://codereview.chromium.org/2868085/diff/1/2
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/2868085/diff/1/2#newcode235
chrome/browser/autocomplete/autocomplete.cc:235: // If the port number is just
garbage after a colon, this is a query.
Nit: How about:
// A port number is a good indicator that this is a URL.  However, it might also
be a query like "1.66:1" that looks kind of like an IP address and port number. 
So here we only check for "port numbers" that are illegal and thus mean this
can't be navigated to (e.g. "1.2.3.4:garbage"), and we save handling legal port
numbers until after the "IP address" determination below.

http://codereview.chromium.org/2868085/diff/1/2#newcode238
chrome/browser/autocomplete/autocomplete.cc:238: if (!(StringToInt(WideToUTF16(
Nit: Distribute the ! through for readability rather than just negating the
entire expression.

http://codereview.chromium.org/2868085/diff/1/2#newcode240
chrome/browser/autocomplete/autocomplete.cc:240: (port >= 0) && (port <=
65535))) {
Nit: no {}

http://codereview.chromium.org/2868085/diff/1/2#newcode260
chrome/browser/autocomplete/autocomplete.cc:260: // Presence of a port means
this is likely a URL -- note that we check above that
Nit: 80 columns.  How about:
// Now that we've ruled out invalid ports and queries that look like they have a
port, the presence of a port means this is likely a URL.

http://codereview.chromium.org/2868085/diff/1/3
File chrome/browser/autocomplete/autocomplete_unittest.cc (right):

http://codereview.chromium.org/2868085/diff/1/3#newcode238
chrome/browser/autocomplete/autocomplete_unittest.cc:238: { L"user@1.2:45",
AutocompleteInput::UNKNOWN },
As noted in the .cc file, I think the second and third cases here should be
URLs, not UNKNOWN.

We should also test "1.2.3.4:garbage".

[isherman, 2010-07-31 04:58:23]

Thanks for the suggestions :)  About to upload an updated patch addressing most
of them, but think it's worth giving a bit more thought to the "user:pass@host"
case -- see my comments below.

http://codereview.chromium.org/2868085/diff/1/2
File chrome/browser/autocomplete/autocomplete.cc (left):

http://codereview.chromium.org/2868085/diff/1/2#oldcode244
chrome/browser/autocomplete/autocomplete.cc:244: // Presence of a username could
either indicate a URL or an email address
On 2010/07/30 01:37:41, Peter Kasting wrote:
> I don't think we should move the username/password checks below the IP check. 
> "user:pass@1.2" seems more likely a URL than a query to me.

I think there are two problems with treating "user:pass@1.2" as a URL:

First of all, we don't even treat "user:pass@foo.com" as a URL, presumably
because we can't distinguish it from a query like "author:user@foo.com".  It
seems like the right thing to do is to treat this as UNKNOWN and search by
default, but pop up a "Did you mean to go to http://user:pass@foo.com ?" dialog,
just as we do in other cases.  For some reason we don't seem to pop up that
dialog -- I can try to dig into why that's not happening.

Secondly, we don't seem to do a great job of handling partial IP addresses like
"http://1.2" anyway -- we just assume the user meant "http://1.0.0.2".  I might
be completely off base, but it seems like treating this as a query (again, with
a "Did you mean..." dialog if it somehow resolves) is more likely to be useful.

[Peter Kasting, 2010-07-31 17:48:10]

http://codereview.chromium.org/2868085/diff/1/2
File chrome/browser/autocomplete/autocomplete.cc (left):

http://codereview.chromium.org/2868085/diff/1/2#oldcode244
chrome/browser/autocomplete/autocomplete.cc:244: // Presence of a username could
either indicate a URL or an email address
On 2010/07/31 04:58:23, isherman wrote:
> First of all, we don't even treat "user:pass@foo.com" as a URL, presumably
> because we can't distinguish it from a query like "author:user@foo.com".

The real problem in this case is that the username is being detected as an
unknown scheme and never reaches this point.  In the unknown scheme handling
code above, we should probably try a test-fixup of "http://" + the input string
and see if there's a valid username, password, and hostname, and if so continue
on through this function using that fixed-up string to make decisions about.

[isherman, 2010-08-02 10:01:46]

http://codereview.chromium.org/2868085/diff/1/2
File chrome/browser/autocomplete/autocomplete.cc (left):

http://codereview.chromium.org/2868085/diff/1/2#oldcode244
chrome/browser/autocomplete/autocomplete.cc:244: // Presence of a username could
either indicate a URL or an email address
On 2010/07/31 17:48:10, Peter Kasting wrote:
> On 2010/07/31 04:58:23, isherman wrote:
> In the unknown scheme handling
> code above, we should probably try a test-fixup of "http://" + the input
string
> and see if there's a valid username, password, and hostname, and if so
continue
> on through this function using that fixed-up string to make decisions about.

Maybe I'm missing something obvious, but how can we determine whether there's a
valid username, password, and hostname without thinking that our fixed-up
"http://author:user@foo.com" is valid?  More generally, all three of the
following patterns seem like they should look equally valid to us:
"user:pass@foo.com", "unknown_scheme:user@foo.com" and
"search_operator:user@foo.com".  Do we have a handy-dandy way to distinguish
these?

http://codereview.chromium.org/2868085/diff/5001/6001
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/2868085/diff/5001/6001#newcode127
chrome/browser/autocomplete/autocomplete.cc:127: // correspond to?  Both?
The TODO is obviously not meant to be part of the final patch, but I think it
would be nice to make the comment a bit clearer -- unless it's already clear to
everyone else and I'm just being ignorant :)

[Peter Kasting, 2010-08-04 17:54:13]

LGTM

http://codereview.chromium.org/2868085/diff/9001/10001
File chrome/browser/autocomplete/autocomplete.cc (right):

http://codereview.chromium.org/2868085/diff/9001/10001#newcode126
chrome/browser/autocomplete/autocomplete.cc:126: // A user might or might not
type a scheme when entering a file URL.  In
Nit: Thanks for the comment update, it's a lot clearer.

This might be even clearer:

  // Check if this is a file: URL.  We need to check |parsed_scheme| instead of
  // |parts->scheme| because the latter will be empty if the user didn't
  // explicitly type "file://" and instead just typed e.g. "C:\foo".
  if (parsed_scheme == L"file")
    return URL;