Remove timing limitation to set Broadcast, ReceiveBuffer, and SendBuffer options from UDPSocket.

Remove timing limitation to set Broadcast, ReceiveBuffer, and SendBuffer options from UDPSocket.

Currently, we have timing limitation to set these options.
This CL splits Open() from Connect() and Bind(), so that those options can be set anytime after Open().
This is the preparation to remove the same limitation from PPAPI's socket APIs.

BUG=425563, 420697
TEST=Ran trybots.

Committed: https://crrev.com/17fac55b85211ba10c1bb3bf8a46798bcf0eb729
Cr-Commit-Position: refs/heads/master@{#307204}

[hidehiko, 2014-11-13 16:57:14]

Patchset #2 (id:20001) has been deleted

[hidehiko, 2014-11-13 16:57:25]

Patchset #1 (id:1) has been deleted

[hidehiko, 2014-11-13 17:56:18]

hidehiko@chromium.org changed reviewers:
+ bbudge@chromium.org, rvargas@chromium.org

[hidehiko, 2014-11-13 17:56:18]

This CL is extracted from crrev.com/690903002, with reflecting rvargas@'s
comments.

Could you take a look?

Thanks,
- hidehiko

[rvargas (doing something else), 2014-11-14 02:44:43]

https://codereview.chromium.org/721273002/diff/40001/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

https://codereview.chromium.org/721273002/diff/40001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:487: if (is_connected()) {
As I mentioned on the other review, I don't really see the purpose of this
pattern. If we are just forwarding the call to the OS we should just do it here.

(applies everywhere)

[hidehiko, 2014-11-19 06:11:24]

Thank you for review.

https://codereview.chromium.org/721273002/diff/40001/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

https://codereview.chromium.org/721273002/diff/40001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:487: if (is_connected()) {
On 2014/11/14 02:44:43, rvargas wrote:
> As I mentioned on the other review, I don't really see the purpose of this
> pattern. If we are just forwarding the call to the OS we should just do it
here.
> 
> (applies everywhere)

This is to address mmenke@'s comment
https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev...

There are two reasons of this pattern:
1) Now, the OS socket is created in Bind(), so to support setting options from
beginning of OS's bind(), we need to cache it.
2) According to mmenke@'s comment, it is necessary to support
Bind()->Close()->Bind() pattern. For that purpose, we need to remember the
option in the members.

Alternative approach would be:
1) Split CreateSocket() from Bind(), so that the API can have users call option
setting methods between CreateSocket() and Bind(). (This is what TCPSocket does,
IIUC).
2) Disallow rebinding. (Though, this still needs to remember the values before
CreateSocket()).

WDYT?

[rvargas (doing something else), 2014-11-19 22:18:56]

https://codereview.chromium.org/721273002/diff/40001/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

https://codereview.chromium.org/721273002/diff/40001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:487: if (is_connected()) {
On 2014/11/19 06:11:24, hidehiko wrote:
> On 2014/11/14 02:44:43, rvargas wrote:
> > As I mentioned on the other review, I don't really see the purpose of this
> > pattern. If we are just forwarding the call to the OS we should just do it
> here.
> > 
> > (applies everywhere)
> 
> This is to address mmenke@'s comment
>
https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev...
> 
> There are two reasons of this pattern:
> 1) Now, the OS socket is created in Bind(), so to support setting options from
> beginning of OS's bind(), we need to cache it.
> 2) According to mmenke@'s comment, it is necessary to support
> Bind()->Close()->Bind() pattern. For that purpose, we need to remember the
> option in the members.
> 
> Alternative approach would be:
> 1) Split CreateSocket() from Bind(), so that the API can have users call
option
> setting methods between CreateSocket() and Bind(). (This is what TCPSocket
does,
> IIUC).
> 2) Disallow rebinding. (Though, this still needs to remember the values before
> CreateSocket()).
> 
> WDYT?

Both? I Agree with Matt in that rebinding is a weird pattern. I don't think we
should actively support that. I hope no one relies on this behavior today. 

It also makes sense to add an Open method to bring this class closer to
TCPSocket.

[hidehiko, 2014-12-01 11:08:19]

Patchset #3 (id:80001) has been deleted

[hidehiko, 2014-12-01 15:57:03]

Patchset #3 (id:100001) has been deleted

[hidehiko, 2014-12-01 16:06:25]

Hi,

Thank you for the discussion last week.
As we agreed on it, I updated this CL.
Could you take another look?

Thanks,
- hidehiko

https://codereview.chromium.org/721273002/diff/40001/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

https://codereview.chromium.org/721273002/diff/40001/net/udp/udp_socket_win.c...
net/udp/udp_socket_win.cc:487: if (is_connected()) {
On 2014/11/19 22:18:56, rvargas wrote:
> On 2014/11/19 06:11:24, hidehiko wrote:
> > On 2014/11/14 02:44:43, rvargas wrote:
> > > As I mentioned on the other review, I don't really see the purpose of this
> > > pattern. If we are just forwarding the call to the OS we should just do it
> > here.
> > > 
> > > (applies everywhere)
> > 
> > This is to address mmenke@'s comment
> >
>
https://codereview.chromium.org/690903002/diff/40001/net/udp/udp_socket_libev...
> > 
> > There are two reasons of this pattern:
> > 1) Now, the OS socket is created in Bind(), so to support setting options
from
> > beginning of OS's bind(), we need to cache it.
> > 2) According to mmenke@'s comment, it is necessary to support
> > Bind()->Close()->Bind() pattern. For that purpose, we need to remember the
> > option in the members.
> > 
> > Alternative approach would be:
> > 1) Split CreateSocket() from Bind(), so that the API can have users call
> option
> > setting methods between CreateSocket() and Bind(). (This is what TCPSocket
> does,
> > IIUC).
> > 2) Disallow rebinding. (Though, this still needs to remember the values
before
> > CreateSocket()).
> > 
> > WDYT?
> 
> Both? I Agree with Matt in that rebinding is a weird pattern. I don't think we
> should actively support that. I hope no one relies on this behavior today. 
> 
> It also makes sense to add an Open method to bring this class closer to
> TCPSocket.

Ok, Done both.
Note: along the change, now UDPServerSocket has a bit more code rather than just
a proxy to UDPSocket. The semantics of the class should not change by this CL.
It is something similar to TCPServerSocket does, so I assume it's ok.
Also, Set{Receive,Send}BufferSize are reverted. Because Open() is split from
Connect() and Bind(), client code can set them before Connect() or Bind() with
the current implementation.

[rvargas (doing something else), 2014-12-02 23:28:10]

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:344:
scoped_ptr<net::UDPServerSocket> receive_socket(
The class used here should come from the actual dependencies of this module. Are
you saying that the test was using the wrong layer before?

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/dial_service.cc (right):

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/dial_service.cc:171: socket_.reset(new
UDPSocket(net::DatagramSocket::RANDOM_BIND,
this is the same code (extensions/api) so the dependencies should be the same.

https://codereview.chromium.org/721273002/diff/120001/cloud_print/gcp20/proto...
File cloud_print/gcp20/prototype/dns_sd_server.h (right):

https://codereview.chromium.org/721273002/diff/120001/cloud_print/gcp20/proto...
cloud_print/gcp20/prototype/dns_sd_server.h:16: #include
"net/udp/udp_server_socket.h"
same question (etc)

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:350: DCHECK(!rv) << "Could not set socket
broadcast option: " << last_error;
Not a good idea to DCHECK the return of an operation performed by software
outside of our control.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:36: // Should be called after Open().
The comments should be the same for both implementations. (and elsewhere)

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:227: // Returns a network error code.
nit: network -> net

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:282: DCHECK(socket_ == INVALID_SOCKET);
DCHECK_EQ

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:418: net_log_.BeginEvent(NetLog::TYPE_UDP_CONNECT,
DCHECK_NE(socket_, invalid)

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:452: // Close() may change the last error. Map it
beforehand.
The comment is stale

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:460: int UDPSocketWin::Bind(const IPEndPoint& address)
{
DCHECK_NE(socket_, invalid)

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:476: int UDPSocketWin::CreateSocket(int addr_family) {
we should move this implementation to Open()

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:485: int UDPSocketWin::SetReceiveBufferSize(int32
size) {
DCHECK_NE(socket_, invalid) etc

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:722: if (socket_options_ &
SOCKET_OPTION_REUSE_ADDRESS) {
For consistency it looks like we should move this to AllowAddressReuse and maybe
rename this method to SetMulticastOptions or something like that.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.h:35: int Open(AddressFamily address_family);
#include "net/base/address_family.h"

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.h:46: // Close the socket.
I'd like to document that this object cannot be Open()'d  again. We may not add
code to DCHECK for that, but it should at least be documented.

[hidehiko, 2014-12-03 12:22:47]

Patchset #4 (id:140001) has been deleted

[hidehiko, 2014-12-03 17:33:26]

hidehiko@chromium.org changed reviewers:
+ hubbe@chromium.org, rockot@chromium.org, scottbyer@chromium.org

[hidehiko, 2014-12-03 17:33:28]

Thank you for review. PTAL.
I think we agreed on the generic implementation direction, so please let me add
more OWNERS.

hubbe@, could you review 	chrome/browser/extensions/api/cast_streaming/... and
media/cast/... ?
scottbyer@, could you review cloud_print/...?
rockot@, could you review extensions/... ? Also, if you have any comment about
https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....,
please let us know.

Also, bbudge@, this CL does not change the pepper code, but as we chatted, I'll
make some more change in pepper following this CL, so could you double-check
about the direction?

Thank you for your help in advance!
- hidehiko

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:344:
scoped_ptr<net::UDPServerSocket> receive_socket(
On 2014/12/02 23:28:09, rvargas wrote:
> The class used here should come from the actual dependencies of this module.
Are
> you saying that the test was using the wrong layer before?

Without this CL, UDPServerSocket is just a subset of UDPSocket.
In this CL, I replace the UDPSocket with UDPServerSocket if possible,
because the UDPServerSocket keeps the current behavior, also, "possible" here
means that the socket is used as a server socket, so that should be fine.

Though I'd defer the decision to OWNERS of each directory.

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/dial_service.cc (right):

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/dial_service.cc:171: socket_.reset(new
UDPSocket(net::DatagramSocket::RANDOM_BIND,
On 2014/12/02 23:28:09, rvargas wrote:
> this is the same code (extensions/api) so the dependencies should be the same.

Here, UDPSocket is kept to be used, because UDPServerSocket does not accept
RANDOM_BIND.

https://codereview.chromium.org/721273002/diff/120001/cloud_print/gcp20/proto...
File cloud_print/gcp20/prototype/dns_sd_server.h (right):

https://codereview.chromium.org/721273002/diff/120001/cloud_print/gcp20/proto...
cloud_print/gcp20/prototype/dns_sd_server.h:16: #include
"net/udp/udp_server_socket.h"
On 2014/12/02 23:28:09, rvargas wrote:
> same question (etc)

Acknowledged.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.cc (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.cc:350: DCHECK(!rv) << "Could not set socket
broadcast option: " << last_error;
On 2014/12/02 23:28:09, rvargas wrote:
> Not a good idea to DCHECK the return of an operation performed by software
> outside of our control.

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:36: // Should be called after Open().
On 2014/12/02 23:28:09, rvargas wrote:
> The comments should be the same for both implementations. (and elsewhere)

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:227: // Returns a network error code.
On 2014/12/02 23:28:09, rvargas wrote:
> nit: network -> net

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win.cc
File net/udp/udp_socket_win.cc (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:282: DCHECK(socket_ == INVALID_SOCKET);
On 2014/12/02 23:28:10, rvargas wrote:
> DCHECK_EQ

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:418: net_log_.BeginEvent(NetLog::TYPE_UDP_CONNECT,
On 2014/12/02 23:28:09, rvargas wrote:
> DCHECK_NE(socket_, invalid)

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:452: // Close() may change the last error. Map it
beforehand.
On 2014/12/02 23:28:09, rvargas wrote:
> The comment is stale

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:460: int UDPSocketWin::Bind(const IPEndPoint& address)
{
On 2014/12/02 23:28:09, rvargas wrote:
> DCHECK_NE(socket_, invalid)

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:476: int UDPSocketWin::CreateSocket(int addr_family) {
On 2014/12/02 23:28:10, rvargas wrote:
> we should move this implementation to Open()

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:485: int UDPSocketWin::SetReceiveBufferSize(int32
size) {
On 2014/12/02 23:28:09, rvargas wrote:
> DCHECK_NE(socket_, invalid) etc

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.cc:722: if (socket_options_ &
SOCKET_OPTION_REUSE_ADDRESS) {
On 2014/12/02 23:28:09, rvargas wrote:
> For consistency it looks like we should move this to AllowAddressReuse and
maybe
> rename this method to SetMulticastOptions or something like that.

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.h:35: int Open(AddressFamily address_family);
On 2014/12/02 23:28:10, rvargas wrote:
> #include "net/base/address_family.h"

Done.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.h:46: // Close the socket.
On 2014/12/02 23:28:10, rvargas wrote:
> I'd like to document that this object cannot be Open()'d  again. We may not
add
> code to DCHECK for that, but it should at least be documented.

+1, but, unfortunately, I found that extensions/browser/api/socket/udp_socket.cc
relies on that behavior.
We probably have two options;
1) Move back to allow Bind()->Close()->Bind() patterns.
2) Edit extensions/.../udp_socket.cc to recreate socket.
Which do you prefer?
If latter case, I think I can make a change for it, but it needs to touch the
code a bit more, so I'd like to separate it from this CL, and working on it in a
following CL.

WDYT?

[hubbe, 2014-12-03 18:22:52]

LGTM for media/cast/*

Separating UDP sockets into server and client sockets never made any sense to
me, but I don't object to the changes.

[Scott Byer, 2014-12-03 19:13:11]

LGTM for cloud_print

[rvargas (doing something else), 2014-12-03 21:00:16]

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:344:
scoped_ptr<net::UDPServerSocket> receive_socket(
On 2014/12/03 17:33:27, hidehiko wrote:
> On 2014/12/02 23:28:09, rvargas wrote:
> > The class used here should come from the actual dependencies of this module.
> Are
> > you saying that the test was using the wrong layer before?
> 
> Without this CL, UDPServerSocket is just a subset of UDPSocket.
> In this CL, I replace the UDPSocket with UDPServerSocket if possible,
> because the UDPServerSocket keeps the current behavior, also, "possible" here
> means that the socket is used as a server socket, so that should be fine.
> 
> Though I'd defer the decision to OWNERS of each directory.

To be honest, I completely forgot about UDPServerSocket/DatagramServerSocket
before seeing it here. Part of the question is why do we have two interfaces
that are so close yet slightly different. That doesn't have to be addressed by
this CL, but I don't think we (net) want to have to support both things :(. I
think we should remove one of them fairly soon (unless there are strong reasons
that I'm missing)

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.h:46: // Close the socket.
On 2014/12/03 17:33:28, hidehiko wrote:
> On 2014/12/02 23:28:10, rvargas wrote:
> > I'd like to document that this object cannot be Open()'d  again. We may not
> add
> > code to DCHECK for that, but it should at least be documented.
> 
> +1, but, unfortunately, I found that
extensions/browser/api/socket/udp_socket.cc
> relies on that behavior.
> We probably have two options;
> 1) Move back to allow Bind()->Close()->Bind() patterns.
> 2) Edit extensions/.../udp_socket.cc to recreate socket.
> Which do you prefer?
> If latter case, I think I can make a change for it, but it needs to touch the
> code a bit more, so I'd like to separate it from this CL, and working on it in
a
> following CL.
> 
> WDYT?
>

The current version is still broken for that use case (and it's broken on ToT).
I think it is fine to fix it in a dedicated CL, but I still don't think that is
a use case we should support (it would be Open - Bind - Close - Open - Bind,
that doesn't really make sense to me).

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:117: // other processes. Should be called before
Bind().
nit: returns a ...

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:128: // Return a net error code.
nit: return -> returns

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:137: // Return a net error code.
etc

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:179: SOCKET_OPTION_REUSE_ADDRESS  = 1 << 0,
should not need this

[Ken Rockot(use gerrit already), 2014-12-03 21:07:49]

extensions lgtm

[hidehiko, 2014-12-04 04:03:36]

Thank you for review. PTAL.

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc
(right):

https://codereview.chromium.org/721273002/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/cast_streaming/cast_streaming_apitest.cc:344:
scoped_ptr<net::UDPServerSocket> receive_socket(
On 2014/12/03 21:00:16, rvargas wrote:
> On 2014/12/03 17:33:27, hidehiko wrote:
> > On 2014/12/02 23:28:09, rvargas wrote:
> > > The class used here should come from the actual dependencies of this
module.
> > Are
> > > you saying that the test was using the wrong layer before?
> > 
> > Without this CL, UDPServerSocket is just a subset of UDPSocket.
> > In this CL, I replace the UDPSocket with UDPServerSocket if possible,
> > because the UDPServerSocket keeps the current behavior, also, "possible"
here
> > means that the socket is used as a server socket, so that should be fine.
> > 
> > Though I'd defer the decision to OWNERS of each directory.
> 
> To be honest, I completely forgot about UDPServerSocket/DatagramServerSocket
> before seeing it here. Part of the question is why do we have two interfaces
> that are so close yet slightly different. That doesn't have to be addressed by
> this CL, but I don't think we (net) want to have to support both things :(. I
> think we should remove one of them fairly soon (unless there are strong
reasons
> that I'm missing)

Ok, then let's address it separately. Maybe we want to keep UDPSocket, and get
rid of UDPServerSocket, IMHO.
As for PPAPI, which is my current focus, I talked about it with Bill, and I'm
planning to do it in a following CL.

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win.h
File net/udp/udp_socket_win.h (right):

https://codereview.chromium.org/721273002/diff/120001/net/udp/udp_socket_win....
net/udp/udp_socket_win.h:46: // Close the socket.
On 2014/12/03 21:00:16, rvargas wrote:
> On 2014/12/03 17:33:28, hidehiko wrote:
> > On 2014/12/02 23:28:10, rvargas wrote:
> > > I'd like to document that this object cannot be Open()'d  again. We may
not
> > add
> > > code to DCHECK for that, but it should at least be documented.
> > 
> > +1, but, unfortunately, I found that
> extensions/browser/api/socket/udp_socket.cc
> > relies on that behavior.
> > We probably have two options;
> > 1) Move back to allow Bind()->Close()->Bind() patterns.
> > 2) Edit extensions/.../udp_socket.cc to recreate socket.
> > Which do you prefer?
> > If latter case, I think I can make a change for it, but it needs to touch
the
> > code a bit more, so I'd like to separate it from this CL, and working on it
in
> a
> > following CL.
> > 
> > WDYT?
> >
> 
> The current version is still broken for that use case (and it's broken on
ToT).
> I think it is fine to fix it in a dedicated CL, but I still don't think that
is
> a use case we should support (it would be Open - Bind - Close - Open - Bind,
> that doesn't really make sense to me).

OK, let's discuss it separately, considering it is not a regression of this CL.
I'm fine to drop it as long as extensions/ team, who is the only user of it for
now in Chrome, agrees.
Added TODO.

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:117: // other processes. Should be called before
Bind().
On 2014/12/03 21:00:16, rvargas wrote:
> nit: returns a ...

Done. Walked though the udp_socket_libevent.h and upd_socket_win.h, and
standardized to third-person-singular-person form for all function comments.

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:128: // Return a net error code.
On 2014/12/03 21:00:16, rvargas wrote:
> nit: return -> returns

Done.

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:137: // Return a net error code.
On 2014/12/03 21:00:16, rvargas wrote:
> etc

Done.

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:179: SOCKET_OPTION_REUSE_ADDRESS  = 1 << 0,
On 2014/12/03 21:00:16, rvargas wrote:
> should not need this

Oops, overlooked. Removed.

[rvargas (doing something else), 2014-12-04 19:51:37]

LGTM after last nit.

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:117: // other processes. Should be called before
Bind().
On 2014/12/04 04:03:36, hidehiko wrote:
> On 2014/12/03 21:00:16, rvargas wrote:
> > nit: returns a ...
> 
> Done. Walked though the udp_socket_libevent.h and upd_socket_win.h, and
> standardized to third-person-singular-person form for all function comments.

I'm sorry I was not clear with this one. Now there is a return value so we have
to document what it is.

[bbudge, 2014-12-06 01:25:54]

LGTM
(though I'm not at all familiar with this code.)

[hidehiko, 2014-12-08 05:07:41]

Thank you for review, all.
Submitting.

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
File net/udp/udp_socket_libevent.h (right):

https://codereview.chromium.org/721273002/diff/160001/net/udp/udp_socket_libe...
net/udp/udp_socket_libevent.h:117: // other processes. Should be called before
Bind().
On 2014/12/04 19:51:37, rvargas wrote:
> On 2014/12/04 04:03:36, hidehiko wrote:
> > On 2014/12/03 21:00:16, rvargas wrote:
> > > nit: returns a ...
> > 
> > Done. Walked though the udp_socket_libevent.h and upd_socket_win.h, and
> > standardized to third-person-singular-person form for all function comments.
> 
> I'm sorry I was not clear with this one. Now there is a return value so we
have
> to document what it is.

Oops, I misunderstood your comments. Fixed the comments.

[hidehiko, 2014-12-08 05:07:47]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-12-08 05:07:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/721273002/200001

[commit-bot: I haz the power, 2014-12-08 06:02:25]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2014-12-08 06:03:06]

Patchset 6 (id:??) landed as
https://crrev.com/17fac55b85211ba10c1bb3bf8a46798bcf0eb729
Cr-Commit-Position: refs/heads/master@{#307204}