Wire 'blink::Platform::isHostnameReservedIPAddress()' to 'net::IsReservedIPAddress()'.

Wire 'blink::Platform::isHostnameReservedIPAddress()' to 'net::IsReservedIPAddress()'.

This is a fairly straightforward patch which wires up the Blink platform
API over to the network stack.

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

[Mike West, 2014-08-02 15:22:15]

And this is the Chromium side. WDYT?

-mike

[Ryan Sleevi, 2014-08-05 00:01:38]

Mostly concerned with performance here

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
content/child/blink_platform_impl.cc:450: std::string canonical_name =
net::CanonicalizeHost(host, &host_info);
Is there no way to pre-canonicalize? This seems a relatively expensive thing to
be calling regularly, especially if Blink has an already-canonicalized form.

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
content/child/blink_platform_impl.cc:457: host =
host.substr(host_info.out_host.begin, host_info.out_host.len);
BUG: host_info.out_host is "location of the host within the canoncalized output"
-
https://code.google.com/p/chromium/codesearch#chromium/src/url/url_canon.h&rc...
- but you're using the non-canonical input.

Use canonical_name.substr()

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
content/child/blink_platform_impl.cc:460: // so we strip them here. Hooray for
consistency.
URLs vs not-URLs ;)

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
content/child/blink_platform_impl.cc:465: if (!net::ParseIPLiteralToNumber(host,
&host_addr))
If you do find it unavoidable to call this every time, we should update this api
to base::StringPiece() instead of requiring multiple copies. Then, your
.substr() calls (line 461, 457) are not so expensive, even if you can
(generally) rely on the small string optimization.

[Mike West, 2014-08-05 07:20:29]

Since we know that we're calling this method with input from WebSecurityOrigin
and WebURL, we can drop some of the expensive steps.

If that assumption concerns you, I suppose we could add two variants of the
method, one which accepts a WebSecurityOrigin and the other which accepts a
WebURL, rather than just the one which accepts a WebString.

WDYT?

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
content/child/blink_platform_impl.cc:450: std::string canonical_name =
net::CanonicalizeHost(host, &host_info);
On 2014/08/05 00:01:38, Ryan Sleevi wrote:
> Is there no way to pre-canonicalize? This seems a relatively expensive thing
to
> be calling regularly, especially if Blink has an already-canonicalized form.

That's fair. We're getting the hostname from either a SecurityOrigin or a URL,
so I think we can safely assume that the hostname is already canonical.

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
content/child/blink_platform_impl.cc:457: host =
host.substr(host_info.out_host.begin, host_info.out_host.len);
On 2014/08/05 00:01:38, Ryan Sleevi wrote:
> BUG: host_info.out_host is "location of the host within the canoncalized
output"
> -
>
https://code.google.com/p/chromium/codesearch#chromium/src/url/url_canon.h&rc...
> - but you're using the non-canonical input.
> 
> Use canonical_name.substr()

I can drop this entirely.

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
content/child/blink_platform_impl.cc:460: // so we strip them here. Hooray for
consistency.
On 2014/08/05 00:01:37, Ryan Sleevi wrote:
> URLs vs not-URLs ;)

Acknowledged.

https://codereview.chromium.org/430193002/diff/1/content/child/blink_platform...
content/child/blink_platform_impl.cc:465: if (!net::ParseIPLiteralToNumber(host,
&host_addr))
On 2014/08/05 00:01:37, Ryan Sleevi wrote:
> If you do find it unavoidable to call this every time, we should update this
api
> to base::StringPiece() instead of requiring multiple copies. Then, your
> .substr() calls (line 461, 457) are not so expensive, even if you can
> (generally) rely on the small string optimization.

What do you think of the current patchset, which drops the substr() calls
entirely, given what we know about the source?

[Ryan Sleevi, 2014-08-05 08:51:17]

Code-wise, I think the latest patch set has more of a smell, because it bleeds
out more of the parsing logic, especially somewhere we (//net) will look. I
definitely would want to keep V4-vs-V6 handling constrained to //net,
//googleurl, and if necessary (though I hope not), KURL. It feels more error
prone as written.

I will have to look closer at the Blink side to.understand the
kurl-or-securityorigin, as well as look at the paths into here. I think that
if/when/as some of these checks move browser side, it might be easier.

[Mike West, 2014-08-05 09:06:38]

On 2014/08/05 08:51:17, Ryan Sleevi wrote:
> Code-wise, I think the latest patch set has more of a smell, because it bleeds
> out more of the parsing logic, especially somewhere we (//net) will look. I
> definitely would want to keep V4-vs-V6 handling constrained to //net,
> //googleurl, and if necessary (though I hope not), KURL. It feels more error
> prone as written.

Yup, I agree.

Should I try to grab some time on your calendar to chat about this, or do you
have a specific suggestion about where what should go?

[Ryan Sleevi, 2014-08-05 09:10:50]

On 2014/08/05 09:06:38, Mike West wrote:
> On 2014/08/05 08:51:17, Ryan Sleevi wrote:
> > Code-wise, I think the latest patch set has more of a smell, because it
bleeds
> > out more of the parsing logic, especially somewhere we (//net) will look. I
> > definitely would want to keep V4-vs-V6 handling constrained to //net,
> > //googleurl, and if necessary (though I hope not), KURL. It feels more error
> > prone as written.
> 
> Yup, I agree.
> 
> Should I try to grab some time on your calendar to chat about this, or do you
> have a specific suggestion about where what should go?

I thought your original approach was (likely) reasonable, and was looking to see
if we can reduce the copying further.

I think in the fast path, most of it should be a no-op, mod one additional
string allocation (for the canonical version), down from 3-4 (your original
patchset). We could elimate that copy if we were guaranteed
pre-canonicalization.

[Mike West, 2014-08-06 08:14:02]

Now with more layering! WDYT?

[Ryan Sleevi, 2014-08-06 19:42:12]

Comment updates, but morally/spiritually/conceptually/technically LGTM

https://codereview.chromium.org/430193002/diff/40001/content/child/blink_plat...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/430193002/diff/40001/content/child/blink_plat...
content/child/blink_platform_impl.cc:53: #include "url/url_canon_ip.h"
Not needed?

https://codereview.chromium.org/430193002/diff/40001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/430193002/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:804: // |hostname| must be a canonicalized hostname with
brackets and everything.
"with brackets and everything" is a bit too colloquial.

// |hostname| is an already canoncalized hostname,
// conforming to RFC 3986.
// For an IP address, this is defined in Section 3.2.2 of RFC 3986,
// with the canonical form for IPv6 addresses defined in Section 4
// of RFC 5952.

https://codereview.chromium.org/430193002/diff/40001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/430193002/diff/40001/net/base/net_util.h#newc...
net/base/net_util.h:375: // Parses a URL-safe IP literal to its numeric value.
Returns true on success,
// Parses a URL-safe IP literal (see RFC 3986, Sec 3.2.2) to its numeric value.
// Returns true on success, and fills |ip_number| with the numeric value.

[Mike West, 2014-08-07 17:58:49]

Blink-side platform stubs landed in https://codereview.chromium.org/446363002/.
Hopefully they'll roll into Chromium overnight and I can update and land this
tomorrow.

[Mike West, 2014-08-07 18:05:40]

jam@: Would you mind taking a look at the content/child change? 

Thanks!

-mike

[jam, 2014-08-07 18:26:23]

lgtm

[Mike West, 2014-08-08 08:15:04]

Thanks to you both. If Blink ever rolls, I'll land this today. :)

https://codereview.chromium.org/430193002/diff/40001/content/child/blink_plat...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/430193002/diff/40001/content/child/blink_plat...
content/child/blink_platform_impl.cc:53: #include "url/url_canon_ip.h"
On 2014/08/06 19:42:12, Ryan Sleevi wrote:
> Not needed?

Done.

https://codereview.chromium.org/430193002/diff/40001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/430193002/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:804: // |hostname| must be a canonicalized hostname with
brackets and everything.
On 2014/08/06 19:42:12, Ryan Sleevi wrote:
> "with brackets and everything" is a bit too colloquial.
> 
> // |hostname| is an already canoncalized hostname,
> // conforming to RFC 3986.
> // For an IP address, this is defined in Section 3.2.2 of RFC 3986,
> // with the canonical form for IPv6 addresses defined in Section 4
> // of RFC 5952.

Done.

https://codereview.chromium.org/430193002/diff/40001/net/base/net_util.h
File net/base/net_util.h (right):

https://codereview.chromium.org/430193002/diff/40001/net/base/net_util.h#newc...
net/base/net_util.h:375: // Parses a URL-safe IP literal to its numeric value.
Returns true on success,
On 2014/08/06 19:42:12, Ryan Sleevi wrote:
> // Parses a URL-safe IP literal (see RFC 3986, Sec 3.2.2) to its numeric
value.
> // Returns true on success, and fills |ip_number| with the numeric value.

Done.

[Mike West, 2014-08-08 08:31:39]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2014-08-08 08:36:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mkwst@chromium.org/430193002/60001

[commit-bot: I haz the power, 2014-08-08 11:04:49]

Change committed as 288327

[Mike West, 2014-08-12 04:36:51]

A revert of this CL has been created in
https://codereview.chromium.org/459363002/ by mkwst@chromium.org.

The reason for reverting is: Suspected of breaking Chrome builds:
https://crbug.com/402037.