url_parse: Segment partially-typed IPv6 literals....

url_parse: Segment partially-typed IPv6 literals.

In Chromium, when a user is typing an IPv6 literal into the address bar, the
autocomplete system goes nuts, trying to grab a port from the end of the
address before it's complete.

Fix this by parsing unterminated IPv6 literals as if there were no port number.
These partial addresses will not survive canonicalization, but they keep the
autocompleter running smoothly as the address is typed.

Also, fix some minor lint warnings elsewhere in url_parse.cc

[eroman, 2009-05-26 18:31:15]

looks good to me.

i suggest also adding a unit-test for this closer to the point of failure (since
the parse output for invalid urls is kinda iffy).

I am expecting that the autocomplete system is getting its bad data from
URLFixerUpper::SegmentURL (which calls ParseStandardURL), in which case I
recommend adding a unit-test for partial IPv6 URLs at the SegmentURL layer
(and/or something at the autocomplete layer).

Note that if you do this change to chromium, you will need to prefix the test
names with DISABLED_ (since you can't commit google-url and chromium changes
atomically).

http://codereview.chromium.org/115748/diff/1004/7
File src/url_parse.cc (right):

http://codereview.chromium.org/115748/diff/1004/7#newcode152
Line 152: // won't survive canonicalization, but autocomplete needs to see them.
It is not appropriate to reference "autocomplete" here (which is a chromium
concept).

[pmarks, 2009-05-26 20:22:11]

On 2009/05/26 18:31:15, eroman wrote:
> looks good to me.
> 
> i suggest also adding a unit-test for this closer to the point of failure
(since
> the parse output for invalid urls is kinda iffy).
> 
> I am expecting that the autocomplete system is getting its bad data from
> URLFixerUpper::SegmentURL (which calls ParseStandardURL), in which case I
> recommend adding a unit-test for partial IPv6 URLs at the SegmentURL layer
> (and/or something at the autocomplete layer).

I haven't actually looked at the autocomplete code at all yet. I was thinking
about improving this parsing for other reasons, and the autocomplete behavior I
observed gave me an excuse to submit it.  But, I'll add those tests and get back
to you.

> Note that if you do this change to chromium, you will need to prefix the test
> names with DISABLED_ (since you can't commit google-url and chromium changes
> atomically).

Based on this cl I reviewed:

http://codereview.chromium.org/114032

it looks like we should be able to change Chromium atomically as the new
googleurl is pulled in.  Am I reading that correctly?

> http://codereview.chromium.org/115748/diff/1004/7
> File src/url_parse.cc (right):
> 
> http://codereview.chromium.org/115748/diff/1004/7#newcode152
> Line 152: // won't survive canonicalization, but autocomplete needs to see
them.
> It is not appropriate to reference "autocomplete" here (which is a chromium
> concept).

Dropped reference to autocomplete.

[eroman, 2009-05-26 20:49:12]

> googleurl is pulled in.  Am I reading that correctly?

Yes, that is correct.

[pmarks, 2009-06-02 23:05:57]

My CanonicalizeHostVerbose change is now blocked for a while, so I'm coming back
to this one.

I want to submit this change now, and then continue on with
http://codereview.chromium.org/112088 on the Chromium side.

Does that sound good?

[brettw, 2009-06-03 20:08:43]

LGTM with some minor clarifications.

http://codereview.chromium.org/115748/diff/3001/2003
File src/url_parse.cc (right):

http://codereview.chromium.org/115748/diff/3001/2003#newcode153
Line 153: int bracket = spec[serverinfo.begin] == '[' ? serverinfo.end() : -1;
I'm having a hard time defining this initialization. How about a comment about
we want to default to where the closing bracket "would be" if it was present,
even if it's not?

It seems like this should also be renamed now that the definition is a little
more complex (and maybe it was poorly-named at the beginning). Like maybe
"ipv6_terminator" or something?

http://codereview.chromium.org/115748/diff/3001/2003#newcode173
Line 173: *port_num = MakeRange(colon + 1, serverinfo.end());
Thanks for improving this. I think this code is from before I added end().

[pmarks, 2009-06-03 21:50:26]

I've updated the comments/names in Patch Set 4.

[brettw, 2009-06-03 21:55:12]

LGTM

[pmarks, 2009-06-03 22:06:03]

Submitted:
http://code.google.com/p/google-url/source/detail?r=106