Migrate net/proxy/* to net::IPAddress.

Migrate net/proxy/* to net::IPAddress.

This CL also moves the ParseCIDRBlock() implementation to ip_address.h.

BUG=496258
Committed: https://crrev.com/579658b7b5e5ef315ad4513574c669fc5363ea9d
Cr-Commit-Position: refs/heads/master@{#382225}

[martijnc, 2016-03-17 21:27:19]

Description was changed from

==========
Migrate net/base/* to net::IPAddress.

BUG=496258
==========

to

==========
Migrate net/proxy/* to net::IPAddress.

BUG=496258
==========

[martijnc, 2016-03-17 21:32:27]

Description was changed from

==========
Migrate net/proxy/* to net::IPAddress.

BUG=496258
==========

to

==========
Migrate net/proxy/* to net::IPAddress.

This CL also moves the ParseCIDRLiteral() implementation to ip_address.h.

BUG=496258
==========

[martijnc, 2016-03-17 21:32:27]

martijn@martijnc.be changed reviewers:
+ eroman@chromium.org

[martijnc, 2016-03-17 21:39:26]

Patchset #1 (id:1) has been deleted

[martijnc, 2016-03-17 21:46:52]

Patchset #1 (id:20001) has been deleted

[martijnc, 2016-03-17 21:48:42]

Patchset #1 (id:40001) has been deleted

[martijnc, 2016-03-17 21:53:40]

Description was changed from

==========
Migrate net/proxy/* to net::IPAddress.

This CL also moves the ParseCIDRLiteral() implementation to ip_address.h.

BUG=496258
==========

to

==========
Migrate net/proxy/* to net::IPAddress.

This CL also moves the ParseCIDRBlock() implementation to ip_address.h.

BUG=496258
==========

[martijnc, 2016-03-18 18:58:30]

Hi, can you review this patch? Thanks!

https://codereview.chromium.org/1810183002/diff/60001/net/base/ip_address_num...
File net/base/ip_address_number_unittest.cc (right):

https://codereview.chromium.org/1810183002/diff/60001/net/base/ip_address_num...
net/base/ip_address_number_unittest.cc:157: {"10.10.1.32", 27, "10.10.1.44",
true},
git cl format reformatted this.

I just separated the CIDR block so that the ParseCIDRBlock() call below can be
removed. The ParseCIDRBlock() function no longer accepts IPAddressNumber (and it
isn't the code being tested here).

https://codereview.chromium.org/1810183002/diff/60001/net/base/ip_address_uni...
File net/base/ip_address_unittest.cc (right):

https://codereview.chromium.org/1810183002/diff/60001/net/base/ip_address_uni...
net/base/ip_address_unittest.cc:242: TEST(IPAddressTest, ParseCIDRBlock_Invalid)
{
These tests are copies from the ones that were in ip_address_number_unittest.cc.

https://codereview.chromium.org/1810183002/diff/60001/net/base/ip_address_uni...
net/base/ip_address_unittest.cc:277:
EXPECT_TRUE(ParseCIDRBlock("::ffff:192.168.0.1/112", ip_address,
Except for this one.

https://codereview.chromium.org/1810183002/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/1810183002/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:248: struct IPAddressInfo {
Renamed this to avoid conflicts. Suggestions for a better name are welcome :).

[eroman, 2016-03-18 19:59:45]

https://codereview.chromium.org/1810183002/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/1810183002/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:248: struct IPAddressInfo {
On 2016/03/18 18:58:30, martijnc wrote:
> Renamed this to avoid conflicts. Suggestions for a better name are welcome :).

This is used exclusively for sorting; my original comment and struct name were
not very good :)

How about this to improve it (change both comment and name):


// Wrapper around an IP address that stores the original string as well as a
// corresponding parsed IPAddress.

// This struct is used as a helper for sorting IP address strings - the IP
// literal is parsed just once and used as the sorting key, while also
// preserving the original IP literal string.
struct IPAddressSortingEntry {

https://codereview.chromium.org/1810183002/diff/80001/net/base/ip_address.h
File net/base/ip_address.h (right):

https://codereview.chromium.org/1810183002/diff/80001/net/base/ip_address.h#n...
net/base/ip_address.h:163: IPAddress& ip_address,
Why was the signature changed?

Google C++ style requires that out parameters be pointers rather than
references.

https://codereview.chromium.org/1810183002/diff/80001/net/base/ip_address_num...
File net/base/ip_address_number_unittest.cc (right):

https://codereview.chromium.org/1810183002/diff/80001/net/base/ip_address_num...
net/base/ip_address_number_unittest.cc:152: size_t prefix_length_in_bits;
Good idea! (Better that this doesn't depend on ParseCIDRBlock(), more explicit
what the inputs are.)

https://codereview.chromium.org/1810183002/diff/80001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/1810183002/diff/80001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:260: return memcmp(ip1.bytes().data(),
ip2.bytes().data(), ip1.size()) <
Since IPAddress already has an operator<() we can fallback tot that here:

return ip1 < ip2;

[martijnc, 2016-03-18 21:41:11]

https://codereview.chromium.org/1810183002/diff/60001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/1810183002/diff/60001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:248: struct IPAddressInfo {
On 2016/03/18 at 19:59:45, eroman wrote:
> On 2016/03/18 18:58:30, martijnc wrote:
> > Renamed this to avoid conflicts. Suggestions for a better name are welcome
:).
> 
> This is used exclusively for sorting; my original comment and struct name were
not very good :)
> 
> How about this to improve it (change both comment and name):
> 
> 
> // Wrapper around an IP address that stores the original string as well as a
> // corresponding parsed IPAddress.
> 
> // This struct is used as a helper for sorting IP address strings - the IP
> // literal is parsed just once and used as the sorting key, while also
> // preserving the original IP literal string.
> struct IPAddressSortingEntry {

Done, thanks.

https://codereview.chromium.org/1810183002/diff/80001/net/base/ip_address.h
File net/base/ip_address.h (right):

https://codereview.chromium.org/1810183002/diff/80001/net/base/ip_address.h#n...
net/base/ip_address.h:163: IPAddress& ip_address,
On 2016/03/18 at 19:59:45, eroman wrote:
> Why was the signature changed?
> 
> Google C++ style requires that out parameters be pointers rather than
references.

Changed.

Wanted to avoid passing a pointer and wasn't aware of the styleguide rule.

https://codereview.chromium.org/1810183002/diff/80001/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/1810183002/diff/80001/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:260: return memcmp(ip1.bytes().data(),
ip2.bytes().data(), ip1.size()) <
On 2016/03/18 at 19:59:45, eroman wrote:
> Since IPAddress already has an operator<() we can fallback tot that here:
> 
> return ip1 < ip2;

Done.

[eroman, 2016-03-18 23:52:17]

lgtm

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address.h
File net/base/ip_address.h (right):

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address.h#n...
net/base/ip_address.h:154: // |ip_address| with the numeric value of the IP
address and sets
The previous documentation preceded these with an asterisk (*) to show
de-reference.

You may as well keep the original documentation since the signature is
unchanged.

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address_uni...
File net/base/ip_address_unittest.cc (right):

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address_uni...
net/base/ip_address_unittest.cc:250: "192.168.1.1/-1",
I wonder if we currently allow :

192.168.1.1/+1

I suspect StringToInt() allows a leading plus sign, in which case we are more
permissive than we should be!

If it does fail (as it should) then would be a good idea to add that to the list
too.

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address_uni...
net/base/ip_address_unittest.cc:257: for (size_t i = 0; i <
arraysize(bad_literals); ++i) {
If you like can change this to:

for (const auto& bad_literal : bad_literals) {
   ...
}

(I believe that works)

https://codereview.chromium.org/1810183002/diff/10009/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/1810183002/diff/10009/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:259: // IP6 addresses are placed ahead of IPv4
addresses.
Can you fix IP6 --> IPv6 while you are here? Thanks!

https://codereview.chromium.org/1810183002/diff/10009/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:265: DCHECK(ip1.size() == ip2.size());
Can remove this DCHECK as it doesn't serve a purpose anymore

(Before it was asserting that the bounds on memcpy were legitimate)

[martijnc, 2016-03-19 12:06:07]

Thank you!

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address.h
File net/base/ip_address.h (right):

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address.h#n...
net/base/ip_address.h:154: // |ip_address| with the numeric value of the IP
address and sets
On 2016/03/18 at 23:52:16, eroman wrote:
> The previous documentation preceded these with an asterisk (*) to show
de-reference.
> 
> You may as well keep the original documentation since the signature is
unchanged.

Done.

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address_uni...
File net/base/ip_address_unittest.cc (right):

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address_uni...
net/base/ip_address_unittest.cc:250: "192.168.1.1/-1",
On 2016/03/18 at 23:52:16, eroman wrote:
> I wonder if we currently allow :
> 
> 192.168.1.1/+1
> 
> I suspect StringToInt() allows a leading plus sign, in which case we are more
permissive than we should be!
> 
> If it does fail (as it should) then would be a good idea to add that to the
list too.

ParseCIDRBlock (incorrectly) parses this as valid. I'll leave this as a refactor
but filled [1] to fix this.

[1] https://crbug.com/596258

https://codereview.chromium.org/1810183002/diff/10009/net/base/ip_address_uni...
net/base/ip_address_unittest.cc:257: for (size_t i = 0; i <
arraysize(bad_literals); ++i) {
On 2016/03/18 at 23:52:17, eroman wrote:
> If you like can change this to:
> 
> for (const auto& bad_literal : bad_literals) {
>    ...
> }
> 
> (I believe that works)

Done.

https://codereview.chromium.org/1810183002/diff/10009/net/proxy/proxy_resolve...
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/1810183002/diff/10009/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:259: // IP6 addresses are placed ahead of IPv4
addresses.
On 2016/03/18 at 23:52:17, eroman wrote:
> Can you fix IP6 --> IPv6 while you are here? Thanks!

Done.

https://codereview.chromium.org/1810183002/diff/10009/net/proxy/proxy_resolve...
net/proxy/proxy_resolver_v8.cc:265: DCHECK(ip1.size() == ip2.size());
On 2016/03/18 at 23:52:17, eroman wrote:
> Can remove this DCHECK as it doesn't serve a purpose anymore
> 
> (Before it was asserting that the bounds on memcpy were legitimate)

Done.

[martijnc, 2016-03-20 12:14:25]

The CQ bit was checked by martijn@martijnc.be

[martijnc, 2016-03-20 12:14:26]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1810183002/#ps110001
(title: "comments eroman")

[commit-bot: I haz the power, 2016-03-20 12:14:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1810183002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1810183002/110001

[commit-bot: I haz the power, 2016-03-20 20:22:23]

Committed patchset #4 (id:110001)

[commit-bot: I haz the power, 2016-03-20 20:24:43]

Description was changed from

==========
Migrate net/proxy/* to net::IPAddress.

This CL also moves the ParseCIDRBlock() implementation to ip_address.h.

BUG=496258
==========

to

==========
Migrate net/proxy/* to net::IPAddress.

This CL also moves the ParseCIDRBlock() implementation to ip_address.h.

BUG=496258

Committed: https://crrev.com/579658b7b5e5ef315ad4513574c669fc5363ea9d
Cr-Commit-Position: refs/heads/master@{#382225}
==========

[commit-bot: I haz the power, 2016-03-20 20:24:44]

Patchset 4 (id:??) landed as
https://crrev.com/579658b7b5e5ef315ad4513574c669fc5363ea9d
Cr-Commit-Position: refs/heads/master@{#382225}