Add DatagramClientSocket::BindToDefaultNetwork(),GetBoundNetwork()

Add DatagramClientSocket::BindToDefaultNetwork(),GetBoundNetwork()

Committed: https://crrev.com/03c445802b1a6bfd80e43d4d085c51636fe643cf
Cr-Commit-Position: refs/heads/master@{#364050}

[pauljensen, 2015-12-04 19:20:58]

pauljensen@chromium.org changed reviewers:
+ jri@chromium.org

[pauljensen, 2015-12-04 19:20:59]

Jana, want to review this? if you don't, we can ask Helen or reviewed the
original NCN NetworkHandle CL.

[Jana, 2015-12-04 20:30:16]

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
net/udp/udp_client_socket.cc:42: for (int attempt = 0; attempt < 3; attempt++) {
attempt < 2 should be enough for this, right? is it possible for this to go to
the attempt = 2?

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
File net/udp/udp_socket_posix.cc (right):

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:339: return ERR_NOT_IMPLEMENTED;
Not for this CL, but this seems like another platform support check that
probably belongs in NCN.

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:363: return ERR_NETWORK_CHANGED;
This is probably fine, but shouldn't this be ERR_INTERNET_DISCONNECTED? (You
could loop a few times in the BindToDefaultNetwork() and not return immediately
on ERR_INTERNET_DISCONNECTED as well.)

[Jana, 2015-12-04 20:30:43]

On 2015/12/04 19:20:59, pauljensen wrote:
> Jana, want to review this? if you don't, we can ask Helen or reviewed the
> original NCN NetworkHandle CL.

Happy to do it!

[pauljensen, 2015-12-07 13:05:55]

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
net/udp/udp_client_socket.cc:42: for (int attempt = 0; attempt < 3; attempt++) {
On 2015/12/04 20:30:16, Jana wrote:
> attempt < 2 should be enough for this, right? is it possible for this to go to
> the attempt = 2?

I think 2 might be enough but as I said in the comment "Loop up to three times
for good measure."  I see little cost in this as all calls here should be very
fast.

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
File net/udp/udp_socket_posix.cc (right):

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:339: return ERR_NOT_IMPLEMENTED;
On 2015/12/04 20:30:16, Jana wrote:
> Not for this CL, but this seems like another platform support check that
> probably belongs in NCN.

Why?  This support check is directly related to the setNetworkForSocket() call. 
NCN should not be thought of as a general-purpose system interface.  Of course
this is all somewhat silly because there is no way to get a NetworkHandle to
pass to this call without being on at least Lollipop.

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:363: return ERR_NETWORK_CHANGED;
On 2015/12/04 20:30:16, Jana wrote:
> This is probably fine, but shouldn't this be ERR_INTERNET_DISCONNECTED? (You
> could loop a few times in the BindToDefaultNetwork() and not return
immediately
> on ERR_INTERNET_DISCONNECTED as well.)

ERR_INTERNET_DISCONNECTED means there are no networks connected at all (i.e.
completely disconnected).  Failing here with ENONET just means |network| has
gone away but does not imply anything about other networks.  Frequently the race
here will be encountered when we change from one default network to another, so
we may not be completely disconnected.

[Jana, 2015-12-07 17:30:17]

On 2015/12/07 13:05:55, pauljensen wrote:
>
https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
> File net/udp/udp_client_socket.cc (right):
> 
>
https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
> net/udp/udp_client_socket.cc:42: for (int attempt = 0; attempt < 3; attempt++)
{
> On 2015/12/04 20:30:16, Jana wrote:
> > attempt < 2 should be enough for this, right? is it possible for this to go
to
> > the attempt = 2?
> 
> I think 2 might be enough but as I said in the comment "Loop up to three times
> for good measure."  I see little cost in this as all calls here should be very
> fast.
> 
>
https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
> File net/udp/udp_socket_posix.cc (right):
> 
>
https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
> net/udp/udp_socket_posix.cc:339: return ERR_NOT_IMPLEMENTED;
> On 2015/12/04 20:30:16, Jana wrote:
> > Not for this CL, but this seems like another platform support check that
> > probably belongs in NCN.
> 
> Why?  This support check is directly related to the setNetworkForSocket()
call. 
> NCN should not be thought of as a general-purpose system interface.  Of course
> this is all somewhat silly because there is no way to get a NetworkHandle to
> pass to this call without being on at least Lollipop.
> 
>
https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
> net/udp/udp_socket_posix.cc:363: return ERR_NETWORK_CHANGED;
> On 2015/12/04 20:30:16, Jana wrote:
> > This is probably fine, but shouldn't this be ERR_INTERNET_DISCONNECTED? (You
> > could loop a few times in the BindToDefaultNetwork() and not return
> immediately
> > on ERR_INTERNET_DISCONNECTED as well.)
> 
> ERR_INTERNET_DISCONNECTED means there are no networks connected at all (i.e.
> completely disconnected).  Failing here with ENONET just means |network| has
> gone away but does not imply anything about other networks.  Frequently the
race
> here will be encountered when we change from one default network to another,
so
> we may not be completely disconnected.

SGTM.

[Jana, 2015-12-07 17:30:52]

lgtm

LGTM, but nit below on num iterations. I'll leave it up to you :-)

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
net/udp/udp_client_socket.cc:42: for (int attempt = 0; attempt < 3; attempt++) {
On 2015/12/07 13:05:55, pauljensen wrote:
> On 2015/12/04 20:30:16, Jana wrote:
> > attempt < 2 should be enough for this, right? is it possible for this to go
to
> > the attempt = 2?
> 
> I think 2 might be enough but as I said in the comment "Loop up to three times
> for good measure."  I see little cost in this as all calls here should be very
> fast.

I don't think it's a matter of cost; it's a matter of code readability. Given
that the loop is to protect against the specific race that should _never_ take
more than 2 iterations, why do more than 2 iterations?

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
File net/udp/udp_socket_posix.cc (right):

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:339: return ERR_NOT_IMPLEMENTED;
On 2015/12/07 13:05:55, pauljensen wrote:
> On 2015/12/04 20:30:16, Jana wrote:
> > Not for this CL, but this seems like another platform support check that
> > probably belongs in NCN.
> 
> Why?  This support check is directly related to the setNetworkForSocket()
call. 
> NCN should not be thought of as a general-purpose system interface.  Of course
> this is all somewhat silly because there is no way to get a NetworkHandle to
> pass to this call without being on at least Lollipop.

Oh yes, apologies -- this doesn't belong in NCN. What I was after was that
having a different global way to check platform that's better than #defines
would be useful.

[pauljensen, 2015-12-08 14:30:48]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2015-12-08 14:30:48]

The patchset sent to the CQ was uploaded after l-g-t-m from jri@chromium.org
Link to the patchset: https://codereview.chromium.org/1416213003/#ps40001
(title: "loop only twice")

[pauljensen, 2015-12-08 14:31:19]

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
File net/udp/udp_client_socket.cc (right):

https://codereview.chromium.org/1416213003/diff/20001/net/udp/udp_client_sock...
net/udp/udp_client_socket.cc:42: for (int attempt = 0; attempt < 3; attempt++) {
On 2015/12/07 17:30:52, Jana wrote:
> On 2015/12/07 13:05:55, pauljensen wrote:
> > On 2015/12/04 20:30:16, Jana wrote:
> > > attempt < 2 should be enough for this, right? is it possible for this to
go
> to
> > > the attempt = 2?
> > 
> > I think 2 might be enough but as I said in the comment "Loop up to three
times
> > for good measure."  I see little cost in this as all calls here should be
very
> > fast.
> 
> I don't think it's a matter of cost; it's a matter of code readability. Given
> that the loop is to protect against the specific race that should _never_ take
> more than 2 iterations, why do more than 2 iterations?

I changed it to 2.

[commit-bot: I haz the power, 2015-12-08 14:33:04]

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

[commit-bot: I haz the power, 2015-12-08 15:04:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 15:04:05]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jana, 2015-12-08 18:35:38]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2015-12-08 18:36:31]

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

[commit-bot: I haz the power, 2015-12-08 19:12:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-08 19:12:43]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pauljensen, 2015-12-09 11:49:06]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2015-12-09 11:49:48]

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

[commit-bot: I haz the power, 2015-12-09 12:52:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-09 12:53:14]

Description was changed from

==========
Add DatagramClientSocket::BindToDefaultNetwork(),GetBoundNetwork()
==========

to

==========
Add DatagramClientSocket::BindToDefaultNetwork(),GetBoundNetwork()

Committed: https://crrev.com/03c445802b1a6bfd80e43d4d085c51636fe643cf
Cr-Commit-Position: refs/heads/master@{#364050}
==========

[commit-bot: I haz the power, 2015-12-09 12:53:14]

Patchset 3 (id:??) landed as
https://crrev.com/03c445802b1a6bfd80e43d4d085c51636fe643cf
Cr-Commit-Position: refs/heads/master@{#364050}