Migrate Local Discovery from net::IPAddressNumber to net::IPAddress.

This CL migrates Local Discovery from net::IPAddressNumber to net::IPAddress.

This CL is part of the net::IPAddressNumber migration[1].

BUG=496258
[1] https://code.google.com/p/chromium/issues/detail?id=496258#c10

Committed: https://crrev.com/aed54d71aad92c60a684f21e30e90bd2f4d9cc7f
Cr-Commit-Position: refs/heads/master@{#372550}

Committed: https://crrev.com/b90328470b5d381d539e012f65fca9eb464ba658
Cr-Commit-Position: refs/heads/master@{#372559}

[martijnc, 2015-12-20 22:00:03]

Patchset #1 (id:1) has been deleted

[martijnc, 2015-12-20 22:00:13]

Patchset #1 (id:20001) has been deleted

[martijnc, 2015-12-20 22:00:18]

Patchset #1 (id:40001) has been deleted

[martijnc, 2015-12-20 22:00:23]

Patchset #1 (id:60001) has been deleted

[martijnc, 2015-12-20 22:00:29]

Patchset #2 (id:100001) has been deleted

[martijnc, 2015-12-20 22:40:53]

Description was changed from

==========
Migrate Local Discovery from net::IPAddressNumber to net::IPAddress.

This patch:
  * Makes net::IPAddress serializable for IPC.
  * Replaces net::IPAddressNumber usage with net::IPAddress.

Context: https://codereview.chromium.org/1408803010

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

to

==========
Migrate Local Discovery from net::IPAddressNumber to net::IPAddress.

This patch:
  * Makes net::IPAddress serializable for IPC.
  * Replaces net::IPAddressNumber usage with net::IPAddress in Local Discovery
    (and related) code.

Context: https://codereview.chromium.org/1408803010

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

[martijnc, 2015-12-20 22:40:53]

martijn@martijnc.be changed reviewers:
+ dgozman@chromium.org, eroman@chromium.org, palmer@chromium.org,
vitalybuka@chromium.org

[martijnc, 2015-12-21 14:22:30]

Patchset #1 (id:80001) has been deleted

[martijnc, 2015-12-21 14:22:41]

Patchset #3 (id:160001) has been deleted

[martijnc, 2015-12-21 15:23:59]

Hi, can you review this patch? Thanks!

+vitalybuk:
dns_sd_device_lister.cc
chrome/browser/local_discovery/*
chrome/common/local_discovery/*
service_discovery_message_handler.(h/cc)

+eroman for net/base/ip_endpoint.(h/cc)
+palmer for content/public/common/common_param_traits.(h/cc)
+dgozman for cast_device_provider.cc & cast_device_provider_unittest.cc

[dgozman, 2015-12-21 17:19:50]

chrome/browser/devtools lgtm

[eroman, 2015-12-21 20:47:33]

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/devtool...
File chrome/browser/devtools/device/cast_device_provider_unittest.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/devtool...
chrome/browser/devtools/device/cast_device_provider_unittest.cc:54:
cast_service.ip_address = net::IPAddress(address1, sizeof(address1));
Or alternately:

ASSERT_TRUE(net::IPAddress::FromIPLiteral("192.168.1.101",
&cast_service.ip_address));

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/mdns/dns_sd_device_lister.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/mdns/dns_sd_device_lister.cc:20: if
(service_description.ip_address.IsIPv4() ||
perhaps we should add a IPAddress::IsValid() method, which returns true if it is
IPv4 or IPv6

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
File chrome/browser/local_discovery/endpoint_resolver.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
chrome/browser/local_discovery/endpoint_resolver.cc:78: if (!address.IsIPv4())
This does not quite have the same meaning as before.

Without knowing more about this code, please match the same semantics as before.
Options for accomplishing that are:

  * Add an empty() or is_null() method, which returns true if the address is
empty.
  * Add an IsValid() method as described earlier, and use that.

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
chrome/browser/local_discovery/endpoint_resolver.cc:81: DCHECK(address.IsIPv4()
|| address.IsIPv6());
address.IsValid()  or !address.is_null()

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
File chrome/browser/local_discovery/service_discovery_client_mac.mm (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
chrome/browser/local_discovery/service_discovery_client_mac.mm:355:
service_description_.ip_address = net::IPAddress(
net::IPAddress::address() needs to (eventually) return an IPAddress rather than
an IPAddressNumber.

I expect this is going to come up during several of the conversion.

Rather then adding lines such as this one, I recommend:

  (1) Rename net::IPEndPoint::address() to address_number(), and indicate it is
deprecated
  (2) Introduce IPEndPoint::address() as returning a const IPAddress&   (then
this line no longer needs to be changed)
  (3) address_number() can be implemented as {return address().byes();}

These changes would be done as separate CLs (not this one);

If you don't want to block this change on the above, then instead you can
introduce a constructor:

   IPAddress(const IPAddressNumber&) 

and then use that here. I think such a constructor is generally useful anyway.

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
File chrome/common/local_discovery/local_domain_resolver_unittest.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
chrome/common/local_discovery/local_domain_resolver_unittest.cc:71: if
(!address.IsIPv4() && !address.IsIPv6())
!IsValid()

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
File chrome/common/local_discovery/service_discovery_client_impl.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
chrome/common/local_discovery/service_discovery_client_impl.cc:501: return
net::IPAddress(&a_rdata->address().front(), a_rdata->address().size());
Use a net::IPAddress(const IPAddressNumber&) constructor instead.

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
File chrome/common/local_discovery/service_discovery_client_unittest.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
chrome/common/local_discovery/service_discovery_client_unittest.cc:414:
ip_address_expected_ = net::IPAddress(address, sizeof(address));
This pattern is common enough, that you might want o create a constructor that
takes an array (and infers the length using template)

https://codereview.chromium.org/1534583002/diff/180001/chrome/utility/local_d...
File chrome/utility/local_discovery/service_discovery_message_handler.cc
(right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/utility/local_d...
chrome/utility/local_discovery/service_discovery_message_handler.cc:468: << ",
IPv4=" << (address_ipv4.IsIPv4() ? address_ipv4.ToString() : "")
This is not a direct translation. Prefer to use an .empty() or is_null() method
so this remains strictly a refactor.

https://codereview.chromium.org/1534583002/diff/180001/content/public/common/...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/content/public/common/...
content/public/common/common_param_traits.cc:153: net::IPAddress
address(&bytes.front(), bytes.size());
This is not correct. bytes might be empty, in which case &bytes.front() is
invalid.

This is solved by using the proposed IPAddress(const IPAddressNumber&)
constructor.

Also, it is better to do the checks below first. (That way avoids copying the
data if it is invalid).

[tfarina, 2015-12-21 23:13:36]

Sorry for bumping here in the middle of this review.

https://codereview.chromium.org/1534583002/diff/180001/net/base/ip_endpoint.h
File net/base/ip_endpoint.h (right):

https://codereview.chromium.org/1534583002/diff/180001/net/base/ip_endpoint.h...
net/base/ip_endpoint.h:32: IPEndPoint(const IPAddress& address, uint16_t port);
Eric, is it a good idea to make this change separately? I think it is so
self-contained that it diserves its own separate CL.

[eroman, 2015-12-21 23:25:05]

https://codereview.chromium.org/1534583002/diff/180001/net/base/ip_endpoint.h
File net/base/ip_endpoint.h (right):

https://codereview.chromium.org/1534583002/diff/180001/net/base/ip_endpoint.h...
net/base/ip_endpoint.h:32: IPEndPoint(const IPAddress& address, uint16_t port);
On 2015/12/21 23:13:36, tfarina wrote:
> Eric, is it a good idea to make this change separately? I think it is so
> self-contained that it diserves its own separate CL.

Yes.
In fact a number of my recommendations make sense to do separately.

(For instance changing address() to return an IPNumber should definitely be its
own CL)

[tfarina, 2015-12-22 00:20:34]

https://codereview.chromium.org/1534583002/diff/180001/net/base/ip_endpoint.h
File net/base/ip_endpoint.h (right):

https://codereview.chromium.org/1534583002/diff/180001/net/base/ip_endpoint.h...
net/base/ip_endpoint.h:32: IPEndPoint(const IPAddress& address, uint16_t port);
On 2015/12/21 23:25:04, eroman wrote:
> On 2015/12/21 23:13:36, tfarina wrote:
> > Eric, is it a good idea to make this change separately? I think it is so
> > self-contained that it diserves its own separate CL.
> 
> Yes.
> In fact a number of my recommendations make sense to do separately.
> 
> (For instance changing address() to return an IPNumber should definitely be
its
> own CL)

I have made it separately.  https://codereview.chromium.org/1540413002/.
Martijn, if want, you can take it from me.

[palmer, 2015-12-22 01:09:10]

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/devtool...
File chrome/browser/devtools/device/cast_device_provider.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/devtool...
chrome/browser/devtools/device/cast_device_provider.cc:175: if
(!ip_address.IsIPv4() && !ip_address.IsIPv6()) {
An IsValid (or is_valid) member function might make sense. (Up to you.)

[martijnc, 2015-12-23 20:10:39]

Patchset #4 (id:200001) has been deleted

[martijnc, 2015-12-23 20:15:27]

Patchset #4 (id:220001) has been deleted

[martijnc, 2016-01-12 18:46:00]

Patchset #4 (id:240001) has been deleted

[martijnc, 2016-01-12 18:48:04]

Patchset #1 (id:120001) has been deleted

[martijnc, 2016-01-12 21:03:27]

Patchset #3 (id:260001) has been deleted

[eroman, 2016-01-26 23:06:32]

What is the status of this CL?

[martijnc, 2016-01-26 23:28:34]

On 2016/01/26 at 23:06:32, eroman wrote:
> What is the status of this CL?

I am(/was) waiting for https://codereview.chromium.org/1565303002/ to land. When
it does I can rebase this CL. The changes here should then be limited to
updating local discovery.

[eroman, 2016-01-26 23:59:28]

Thanks! I didn't realize the dependent change hadn't landed yet :)

[martijnc, 2016-01-27 20:17:56]

Patchset #3 (id:280001) has been deleted

[martijnc, 2016-01-27 20:32:11]

Patchset #3 (id:300001) has been deleted

[martijnc, 2016-01-27 20:36:37]

Patchset #3 (id:320001) has been deleted

[martijnc, 2016-01-27 20:45:09]

Description was changed from

==========
Migrate Local Discovery from net::IPAddressNumber to net::IPAddress.

This patch:
  * Makes net::IPAddress serializable for IPC.
  * Replaces net::IPAddressNumber usage with net::IPAddress in Local Discovery
    (and related) code.

Context: https://codereview.chromium.org/1408803010

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

to

==========
This CL migrates Local Discovery from net::IPAddressNumber to net::IPAddress.

This CL is part of the net::IPAddressNumber migration[1].

BUG=496258

[1] https://code.google.com/p/chromium/issues/detail?id=496258#c10
==========

[martijnc, 2016-01-27 20:45:09]

martijn@martijnc.be changed reviewers:
- palmer@chromium.org

[martijnc, 2016-01-27 21:52:03]

Patchset #3 (id:340001) has been deleted

[martijnc, 2016-01-27 22:50:52]

Sorry about the delay, landing the other CLs took a bit longer than I expected.

This is now ready for review. Please have another look. Thanks!

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/devtool...
File chrome/browser/devtools/device/cast_device_provider.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/devtool...
chrome/browser/devtools/device/cast_device_provider.cc:175: if
(!ip_address.IsIPv4() && !ip_address.IsIPv6()) {
On 2015/12/22 at 01:09:10, palmer wrote:
> An IsValid (or is_valid) member function might make sense. (Up to you.)

Done.

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/devtool...
File chrome/browser/devtools/device/cast_device_provider_unittest.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/devtool...
chrome/browser/devtools/device/cast_device_provider_unittest.cc:54:
cast_service.ip_address = net::IPAddress(address1, sizeof(address1));
On 2015/12/21 at 20:47:33, eroman wrote:
> Or alternately:
> 
> ASSERT_TRUE(net::IPAddress::FromIPLiteral("192.168.1.101",
&cast_service.ip_address));

Done.

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/mdns/dns_sd_device_lister.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/mdns/dns_sd_device_lister.cc:20: if
(service_description.ip_address.IsIPv4() ||
On 2015/12/21 at 20:47:33, eroman wrote:
> perhaps we should add a IPAddress::IsValid() method, which returns true if it
is IPv4 or IPv6

Done.

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
File chrome/browser/local_discovery/endpoint_resolver.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
chrome/browser/local_discovery/endpoint_resolver.cc:78: if (!address.IsIPv4())
On 2015/12/21 at 20:47:33, eroman wrote:
> This does not quite have the same meaning as before.
> 
> Without knowing more about this code, please match the same semantics as
before.
> Options for accomplishing that are:
> 
>   * Add an empty() or is_null() method, which returns true if the address is
empty.
>   * Add an IsValid() method as described earlier, and use that.

Done.

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
chrome/browser/local_discovery/endpoint_resolver.cc:81: DCHECK(address.IsIPv4()
|| address.IsIPv6());
On 2015/12/21 at 20:47:33, eroman wrote:
> address.IsValid()  or !address.is_null()

Done.

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
File chrome/browser/local_discovery/service_discovery_client_mac.mm (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/browser/local_d...
chrome/browser/local_discovery/service_discovery_client_mac.mm:355:
service_description_.ip_address = net::IPAddress(
On 2015/12/21 at 20:47:33, eroman wrote:
> net::IPAddress::address() needs to (eventually) return an IPAddress rather
than an IPAddressNumber.
> 
> I expect this is going to come up during several of the conversion.
> 
> Rather then adding lines such as this one, I recommend:
> 
>   (1) Rename net::IPEndPoint::address() to address_number(), and indicate it
is deprecated
>   (2) Introduce IPEndPoint::address() as returning a const IPAddress&   (then
this line no longer needs to be changed)
>   (3) address_number() can be implemented as {return address().byes();}
> 
> These changes would be done as separate CLs (not this one);
> 
> If you don't want to block this change on the above, then instead you can
introduce a constructor:
> 
>    IPAddress(const IPAddressNumber&) 
> 
> and then use that here. I think such a constructor is generally useful anyway.

Done.

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
File chrome/common/local_discovery/local_domain_resolver_unittest.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
chrome/common/local_discovery/local_domain_resolver_unittest.cc:71: if
(!address.IsIPv4() && !address.IsIPv6())
On 2015/12/21 at 20:47:33, eroman wrote:
> !IsValid()

Done.

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
File chrome/common/local_discovery/service_discovery_client_impl.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
chrome/common/local_discovery/service_discovery_client_impl.cc:501: return
net::IPAddress(&a_rdata->address().front(), a_rdata->address().size());
On 2015/12/21 at 20:47:33, eroman wrote:
> Use a net::IPAddress(const IPAddressNumber&) constructor instead.

Done.

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
File chrome/common/local_discovery/service_discovery_client_unittest.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/common/local_di...
chrome/common/local_discovery/service_discovery_client_unittest.cc:414:
ip_address_expected_ = net::IPAddress(address, sizeof(address));
On 2015/12/21 at 20:47:33, eroman wrote:
> This pattern is common enough, that you might want o create a constructor that
takes an array (and infers the length using template)

Replaced with IPAddress::FromIPLiteral().

https://codereview.chromium.org/1534583002/diff/180001/chrome/utility/local_d...
File chrome/utility/local_discovery/service_discovery_message_handler.cc
(right):

https://codereview.chromium.org/1534583002/diff/180001/chrome/utility/local_d...
chrome/utility/local_discovery/service_discovery_message_handler.cc:468: << ",
IPv4=" << (address_ipv4.IsIPv4() ? address_ipv4.ToString() : "")
On 2015/12/21 at 20:47:33, eroman wrote:
> This is not a direct translation. Prefer to use an .empty() or is_null()
method so this remains strictly a refactor.

(This code has been removed.)

https://codereview.chromium.org/1534583002/diff/180001/content/public/common/...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/1534583002/diff/180001/content/public/common/...
content/public/common/common_param_traits.cc:153: net::IPAddress
address(&bytes.front(), bytes.size());
On 2015/12/21 at 20:47:33, eroman wrote:
> This is not correct. bytes might be empty, in which case &bytes.front() is
invalid.
> 
> This is solved by using the proposed IPAddress(const IPAddressNumber&)
constructor.
> 
> Also, it is better to do the checks below first. (That way avoids copying the
data if it is invalid).

Handled in a separate CL.

[eroman, 2016-01-28 20:08:44]

lgtm

https://codereview.chromium.org/1534583002/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/mdns/dns_sd_device_lister.cc (right):

https://codereview.chromium.org/1534583002/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/mdns/dns_sd_device_lister.cc:19: if
(service_description.ip_address.IsValid()) {
For the record, the conversions from empty() --> IsValid() are not exactly the
same.

I have reviewed the callers though and I believe the more restrictive IsValid()
is appropriate here and in the other locations. (ToString for instance will
crash if passed a non IPv4 or IPv6, so IsValid() is a better condition).

https://codereview.chromium.org/1534583002/diff/360001/chrome/browser/local_d...
File chrome/browser/local_discovery/service_discovery_client_impl.h (right):

https://codereview.chromium.org/1534583002/diff/360001/chrome/browser/local_d...
chrome/browser/local_discovery/service_discovery_client_impl.h:200: const
net::IPAddress RecordToIPAddress(const net::RecordParsed* record) const;
Instead of "const net::IPAddress" write simply "net::IPAddress".

Also once everything else has been converted, it might be possible to switch
this back to returning a reference.

[martijnc, 2016-01-28 21:55:39]

Patchset #4 (id:380001) has been deleted

[martijnc, 2016-01-29 18:33:45]

Thanks you for the review.

vitalybuk: Can you take a look at the following files? Thanks!
dns_sd_device_lister.cc
privet_device_lister_unittest.cc
chrome/browser/local_discovery/*

https://codereview.chromium.org/1534583002/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/api/mdns/dns_sd_device_lister.cc (right):

https://codereview.chromium.org/1534583002/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/api/mdns/dns_sd_device_lister.cc:19: if
(service_description.ip_address.IsValid()) {
On 2016/01/28 at 20:08:44, eroman wrote:
> For the record, the conversions from empty() --> IsValid() are not exactly the
same.
> 
> I have reviewed the callers though and I believe the more restrictive
IsValid() is appropriate here and in the other locations. (ToString for instance
will crash if passed a non IPv4 or IPv6, so IsValid() is a better condition).

Yes, had the same thought. Replacing with IsValid() to protect against
ToString() crashes seemed like a good idea. Will keep this in mind for the rest
of the migration.

https://codereview.chromium.org/1534583002/diff/360001/chrome/browser/local_d...
File chrome/browser/local_discovery/service_discovery_client_impl.h (right):

https://codereview.chromium.org/1534583002/diff/360001/chrome/browser/local_d...
chrome/browser/local_discovery/service_discovery_client_impl.h:200: const
net::IPAddress RecordToIPAddress(const net::RecordParsed* record) const;
On 2016/01/28 at 20:08:44, eroman wrote:
> Instead of "const net::IPAddress" write simply "net::IPAddress".
> 
> Also once everything else has been converted, it might be possible to switch
this back to returning a reference.

Done.

I'll revisit this when net::ARecordRdata is migrated.

[Vitaly Buka (NO REVIEWS), 2016-01-29 18:48:31]

https://code.google.com/p/chromium/codesearch#chromium/src/net/base/ip_addres...
Maybe also following for consistency?
operator>
operator>=
operator<=

[Vitaly Buka (NO REVIEWS), 2016-01-29 18:54:02]

lgtm

[martijnc, 2016-01-30 13:41:32]

On 2016/01/29 at 18:48:31, vitalybuka wrote:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/ip_addres...
> Maybe also following for consistency?
> operator>
> operator>=
> operator<=

Hmm, I added them initially but removed them during the review after a comment.

[martijnc, 2016-01-30 13:41:36]

The CQ bit was checked by martijn@martijnc.be

[martijnc, 2016-01-30 13:41:36]

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

[commit-bot: I haz the power, 2016-01-30 13:41:40]

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

[commit-bot: I haz the power, 2016-01-30 14:24:29]

Committed patchset #4 (id:400001)

[commit-bot: I haz the power, 2016-01-30 14:25:31]

Description was changed from

==========
This CL migrates Local Discovery from net::IPAddressNumber to net::IPAddress.

This CL is part of the net::IPAddressNumber migration[1].

BUG=496258

[1] https://code.google.com/p/chromium/issues/detail?id=496258#c10
==========

to

==========
This CL migrates Local Discovery from net::IPAddressNumber to net::IPAddress.

This CL is part of the net::IPAddressNumber migration[1].

BUG=496258

[1] https://code.google.com/p/chromium/issues/detail?id=496258#c10

Committed: https://crrev.com/aed54d71aad92c60a684f21e30e90bd2f4d9cc7f
Cr-Commit-Position: refs/heads/master@{#372550}
==========

[commit-bot: I haz the power, 2016-01-30 14:25:32]

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

[Nico, 2016-01-30 14:57:10]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-01-30 14:57:11]

FAILED: ninja -t msvc -e environment.x86 --
"..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m32 /nologo
/showIncludes /FC
@obj\chrome\tools\service_discovery_sniffer\service_discovery_sniffer.service_discovery_sniffer.obj.rsp
/c ..\..\chrome\tools\service_discovery_sniffer\service_discovery_sniffer.cc
/Foobj\chrome\tools\service_discovery_sniffer\service_discovery_sniffer.service_discovery_sniffer.obj
/Fdobj\chrome\service_discovery_sniffer.cc.pdb 
..\..\chrome\tools\service_discovery_sniffer\service_discovery_sniffer.cc(60,26)
:  error: invalid operands to binary expression ('const net::IPAddress' and
'net::IPAddressNumber' (aka 'vector<unsigned char>'))
  if (service.ip_address != net::IPAddressNumber()) {
      ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~
..\..\chrome\tools\service_discovery_sniffer\service_discovery_sniffer.cc(61,34)
:  error: no matching function for call to 'IPAddressToString'
    printf("\tIP Address: %s\n", net::IPAddressToString(
                                 ^~~~~~~~~~~~~~~~~~~~~~
..\..\net/base/ip_address_number.h(51,24) :  note: candidate function not
viable: no known conversion from 'const net::IPAddress' to 'const
IPAddressNumber' (aka 'const vector<unsigned char>') for 1st argument
NET_EXPORT std::string IPAddressToString(const IPAddressNumber& addr);
                       ^
..\..\net/base/ip_address_number.h(41,24) :  note: candidate function not
viable: requires 2 arguments, but 1 was provided
NET_EXPORT std::string IPAddressToString(const uint8_t* address,
                       ^
2 errors generated.

https://build.chromium.org/p/chromium.fyi/builders/CrWinClang%28dbg%29/builds...

(most bots probably don't build that target)

[Nico, 2016-01-30 14:57:59]

On the main waterfall too:
https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/14840/steps...

[Nico, 2016-01-30 14:58:39]

A revert of this CL (patchset #4 id:400001) has been created in
https://codereview.chromium.org/1653573003/ by thakis@chromium.org.

The reason for reverting is: Doesn't build on the 'all' builders:

..\..\chrome\tools\service_discovery_sniffer\service_discovery_sniffer.cc(60,26)
:  error: invalid operands to binary expression ('const net::IPAddress' and
'net::IPAddressNumber' (aka 'vector<unsigned char>'))
  if (service.ip_address != net::IPAddressNumber()) {
      ~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~
..\..\chrome\tools\service_discovery_sniffer\service_discovery_sniffer.cc(61,34)
:  error: no matching function for call to 'IPAddressToString'
    printf("\tIP Address: %s\n", net::IPAddressToString(
                                 ^~~~~~~~~~~~~~~~~~~~~~
..\..\net/base/ip_address_number.h(51,24) :  note: candidate function not
viable: no known conversion from 'const net::IPAddress' to 'const
IPAddressNumber' (aka 'const vector<unsigned char>') for 1st argument
NET_EXPORT std::string IPAddressToString(const IPAddressNumber& addr);
                       ^
..\..\net/base/ip_address_number.h(41,24) :  note: candidate function not
viable: requires 2 arguments, but 1 was provided
NET_EXPORT std::string IPAddressToString(const uint8_t* address,

https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/14840/steps....

[martijnc, 2016-01-30 18:49:32]

Patchset #5 (id:420001) has been deleted

[martijnc, 2016-01-30 18:54:37]

vitalybuka: I missed one file; patchset #5 also updates
chrome/tools/service_discovery_sniffer/service_discovery_sniffer.cc.

'service_discovery_sniffer' compiles locally now, is there a trybot I can run to
test this?

[Nico, 2016-01-30 19:02:06]

No. If all targets now build fine locally, just reland and hope for the
best :-)
On Jan 30, 2016 1:54 PM, "martijn@martijnc.be via codereview.chromium.org" <
reply@chromiumcodereview-hr.appspotmail.com> wrote:

>
> vitalybuka: I missed one file; patchset #5 also updates
> chrome/tools/service_discovery_sniffer/service_discovery_sniffer.cc.
>
> 'service_discovery_sniffer' compiles locally now, is there a trybot I can run
to
> test this?
> https://codereview.chromium.org/1534583002/
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[martijnc, 2016-01-30 19:09:48]

The CQ bit was checked by martijn@martijnc.be

[martijnc, 2016-01-30 19:09:49]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
dgozman@chromium.org, vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/1534583002/#ps440001
(title: "fix builders")

[commit-bot: I haz the power, 2016-01-30 19:10:00]

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

[commit-bot: I haz the power, 2016-01-30 19:50:37]

Committed patchset #5 (id:440001)

[commit-bot: I haz the power, 2016-01-30 19:51:49]

Description was changed from

==========
This CL migrates Local Discovery from net::IPAddressNumber to net::IPAddress.

This CL is part of the net::IPAddressNumber migration[1].

BUG=496258

[1] https://code.google.com/p/chromium/issues/detail?id=496258#c10

Committed: https://crrev.com/aed54d71aad92c60a684f21e30e90bd2f4d9cc7f
Cr-Commit-Position: refs/heads/master@{#372550}
==========

to

==========
This CL migrates Local Discovery from net::IPAddressNumber to net::IPAddress.

This CL is part of the net::IPAddressNumber migration[1].

BUG=496258

[1] https://code.google.com/p/chromium/issues/detail?id=496258#c10

Committed: https://crrev.com/aed54d71aad92c60a684f21e30e90bd2f4d9cc7f
Cr-Commit-Position: refs/heads/master@{#372550}

Committed: https://crrev.com/b90328470b5d381d539e012f65fca9eb464ba658
Cr-Commit-Position: refs/heads/master@{#372559}
==========

[commit-bot: I haz the power, 2016-01-30 19:51:50]

Patchset 5 (id:??) landed as
https://crrev.com/b90328470b5d381d539e012f65fca9eb464ba658
Cr-Commit-Position: refs/heads/master@{#372559}