Use the new CanonicalizeHostVerbose() function.

- Pull in googleurl r107, which includes the new CanonicalizeHostVerbose() function:
http://code.google.com/p/google-url/source/detail?r=107

- Atomically update Chromium to make use of this new function.  This allows us to extract better information about IP addresses using fewer, and cleaner, calls to googleurl.

- Also, change a call to CanonicalizeIPAddress() to stay compatible with r107.

The upshot of all this is, Chrome will no longer try to connect to IPv4 addresses with overflow "http://192.168.0.257", or hostnames surrounded by square brackets "http://[google.com]"


BUG=none
TEST={unit_tests,googleurl_unittests,net_unittests} pass for me on Linux.


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

[brettw, 2009-05-28 18:27:47]

Your other changes make more sense to me now. This looks like a nice
improvement.

http://codereview.chromium.org/113944/diff/1/3
File net/base/net_util.cc (right):

http://codereview.chromium.org/113944/diff/1/3#newcode791
Line 791: // If no host_info was provided, allocate a temporary one.
Do we worry that much about extra initializations of this struct? Seems like
this could be shorter and more efficient like:
  url_canon::CanonHostInfo temp_info;
  if (!host_info)
    host_info = &temp_info;

[pmarks, 2009-05-28 19:23:09]

http://codereview.chromium.org/113944/diff/1/3
File net/base/net_util.cc (right):

http://codereview.chromium.org/113944/diff/1/3#newcode791
Line 791: // If no host_info was provided, allocate a temporary one.
On 2009/05/28 18:27:47, brettw wrote:
> Do we worry that much about extra initializations of this struct? Seems like
> this could be shorter and more efficient like:
>   url_canon::CanonHostInfo temp_info;
>   if (!host_info)
>     host_info = &temp_info;

Hm, that was needlessly complicated.  Fixed.

[pmarks, 2009-06-22 23:11:28]

This change is ready for review now.

[brettw, 2009-06-22 23:53:57]

http://codereview.chromium.org/113944/diff/5025/4029
File net/base/net_util.cc (right):

http://codereview.chromium.org/113944/diff/5025/4029#newcode835
Line 835: url_canon::CanonHostInfo temp_host_info;
I like to avoid option arguments when possible, especially when it means there
has to be a few lines of code like this. I only see one obvious place that calls
this with NULL, so I'd say make it non-optional and fix that place.

http://codereview.chromium.org/113944/diff/5025/4029#newcode847
Line 847: if (host_info->out_host.is_valid() &&
Note that is_valid includes empty hosts, which constradicts your comment below
about "empty host." If you want to disallow empty ones, use is_nonempty().

[pmarks, 2009-06-23 01:31:35]

http://codereview.chromium.org/113944/diff/5025/4029
File net/base/net_util.cc (right):

http://codereview.chromium.org/113944/diff/5025/4029#newcode835
Line 835: url_canon::CanonHostInfo temp_host_info;
On 2009/06/22 23:53:57, brettw wrote:
> I like to avoid option arguments when possible, especially when it means there
> has to be a few lines of code like this. I only see one obvious place that
calls
> this with NULL, so I'd say make it non-optional and fix that place.

Okay, if you say so.  Fixed cookie_monster.cc.

http://codereview.chromium.org/113944/diff/5025/4029#newcode847
Line 847: if (host_info->out_host.is_valid() &&
On 2009/06/22 23:53:57, brettw wrote:
> Note that is_valid includes empty hosts, which constradicts your comment below
> about "empty host." If you want to disallow empty ones, use is_nonempty().

When url_canon::CanonicalizeHost*() sees an empty host, it converts it into an
invalid component, but returns success.  Thus, is_valid() catches all empty
hosts.  But this test is only really there to avoid hitting DCHECK_EQ(-1, 0)
anyhow.

I suppose I could get rid of is_valid() and DCHECK_EQ, and just assume that
StdStringCanonOutput will always continue to behave the same way, but that seems
less safe.

[brettw, 2009-06-23 18:21:56]

http://codereview.chromium.org/113944/diff/5025/4029
File net/base/net_util.cc (right):

http://codereview.chromium.org/113944/diff/5025/4029#newcode847
Line 847: if (host_info->out_host.is_valid() &&
On 2009/06/23 01:31:35, pmarks wrote:
> On 2009/06/22 23:53:57, brettw wrote:
> > Note that is_valid includes empty hosts, which constradicts your comment
below
> > about "empty host." If you want to disallow empty ones, use is_nonempty().
> 
> When url_canon::CanonicalizeHost*() sees an empty host, it converts it into an
> invalid component, but returns success.  Thus, is_valid() catches all empty
> hosts.

But then wouldn't is_nonempty() also catch them since it would be converted to
invalid, which is also empty?

I see how this works now, but it seems more clear to do the nonempty check here.

[pmarks, 2009-06-23 18:48:52]

On 2009/06/23 18:21:56, brettw wrote:
> 
> But then wouldn't is_nonempty() also catch them since it would be converted to
> invalid, which is also empty?
> 
> I see how this works now, but it seems more clear to do the nonempty check
here.

Ok, sounds good.  Fixed.

(Yesterday, I was looking at GURL instead of Component, so I didn't think
is_nonempty() actually existed)

[brettw, 2009-06-23 20:38:43]

LGTM

[pmarks, 2009-06-23 20:50:38]

On 2009/06/23 20:38:43, brettw wrote:
> LGTM

Can you submit this for me?  I don't have write access to Chromium.

I assume eroman doesn't have much to say, as he didn't comment on the previous
change either.