Add bind(), recvFrom(), sendTo() for UDP socket.

Add bind(), recvFrom(), sendTo() for UDP socket.


BUG=None
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133935

[miket_OOO, 2012-04-20 17:14:29]

Continuing email discussion: if TCP and UDP create/connect semantics are so
different, would it make more sense to split them? Instead of
socket.create(type), have socket.createTCP(a, b, c...) and socket.createUDP(d,
e, f...).

Then we aren't in an artificial situation where we're forcing ourselves to lie
(either socket.connect does nothing for UDP, or socket.create does nothing for
TCP).

[Peng, 2012-04-23 21:02:12]

Hi Mike, Could you please review this CL.

Hi Agl, Could you please review code in net/ folder?

Thanks.

[agl, 2012-04-23 21:07:08]

net/ LGTM with suggested change.

https://chromiumcodereview.appspot.com/10134008/diff/4016/net/udp/udp_socket_...
File net/udp/udp_socket_win.h (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/net/udp/udp_socket_...
net/udp/udp_socket_win.h:28: class NET_EXPORT UDPSocketWin : public
base::NonThreadSafe {
See NON_EXPORTED_BASE macro for this.

[miket_OOO, 2012-04-23 22:23:40]

Generally: we need the tests re-renabled, or at least an explanation why it's
impossible to test this code.

Generally: please file a tracking bug for BUG=.

http://codereview.chromium.org/10134008/diff/4016/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10134008/diff/4016/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket.cc:41: int port = 0;
I checked and TCP/UDP port 0 is indeed reserved and not to be used. So it's a
valid initialization value.

http://codereview.chromium.org/10134008/diff/4016/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket.cc:84: &addr_len)) {
indent

http://codereview.chromium.org/10134008/diff/4016/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket.cc:90: *port = 0;
Should you return false in this case? If not, can we switch return type to void?

http://codereview.chromium.org/10134008/diff/4016/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_apitest.cc (right):

http://codereview.chromium.org/10134008/diff/4016/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_apitest.cc:37: #if 0
Can we do this before submitting? How do we know this works?

http://codereview.chromium.org/10134008/diff/4016/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right):

http://codereview.chromium.org/10134008/diff/4016/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/udp_socket_unittest.cc:23: #if 0
Same here. Why not now?

[Peng, 2012-04-24 15:07:14]

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.cc:41: int port = 0;
On 2012/04/23 22:23:40, miket wrote:
> I checked and TCP/UDP port 0 is indeed reserved and not to be used. So it's a
> valid initialization value.
http://msdn.microsoft.com/en-us/library/windows/desktop/ms737550(v=vs.85).aspx

For TCP/IP, if the port is specified as zero, the service provider assigns a
unique port to the application from the dynamic client port range. 

I also tried it on Linux. It works as expected. But I can not find document
about it.

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.cc:84: &addr_len)) {
On 2012/04/23 22:23:40, miket wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket.cc:90: *port = 0;
On 2012/04/23 22:23:40, miket wrote:
> Should you return false in this case? If not, can we switch return type to
void?

Done.

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket_apitest.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket_apitest.cc:37: #if 0
On 2012/04/23 22:23:40, miket wrote:
> Can we do this before submitting? How do we know this works?

The UDP part of apitest has to be re-implemented. I am not sure if I can finish
them in time. I will try.

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/udp_socket_unittest.cc:23: #if 0
On 2012/04/23 22:23:40, miket wrote:
> Same here. Why not now?

It is impossible to reuse those UDP tests. Because methods of net::UDPSocket are
not virtual methods. We can not test those functions by creating a
MockUDPSocket. Do you have good ideas?

https://chromiumcodereview.appspot.com/10134008/diff/4016/net/udp/udp_socket_...
File net/udp/udp_socket_win.h (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/net/udp/udp_socket_...
net/udp/udp_socket_win.h:28: class NET_EXPORT UDPSocketWin : public
base::NonThreadSafe {
On 2012/04/23 21:07:08, agl wrote:
> See NON_EXPORTED_BASE macro for this.

Done.

[miket_OOO, 2012-04-24 18:13:56]

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket_apitest.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket_apitest.cc:37: #if 0
> The UDP part of apitest has to be re-implemented. I am not sure if I can
finish
> them in time. I will try.

Thanks. There should be at least minimal end-to-end testing to be sure the API
works (and keeps working).

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/udp_socket_unittest.cc:23: #if 0
On 2012/04/24 15:07:14, Peng wrote:
> On 2012/04/23 22:23:40, miket wrote:
> > Same here. Why not now?
> 
> It is impossible to reuse those UDP tests. Because methods of net::UDPSocket
are
> not virtual methods. We can not test those functions by creating a
> MockUDPSocket. Do you have good ideas?
> 

Can you make them virtual?

[Peng, 2012-04-24 20:31:38]

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket_apitest.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket_apitest.cc:37: #if 0
On 2012/04/24 18:13:56, miket wrote:
> > The UDP part of apitest has to be re-implemented. I am not sure if I can
> finish
> > them in time. I will try.
> 
> Thanks. There should be at least minimal end-to-end testing to be sure the API
> works (and keeps working).

Done.

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/udp_socket_unittest.cc:23: #if 0
On 2012/04/24 18:13:56, miket wrote:
> On 2012/04/24 15:07:14, Peng wrote:
> > On 2012/04/23 22:23:40, miket wrote:
> > > Same here. Why not now?
> > 
> > It is impossible to reuse those UDP tests. Because methods of net::UDPSocket
> are
> > not virtual methods. We can not test those functions by creating a
> > MockUDPSocket. Do you have good ideas?
> > 
> 
> Can you make them virtual?

Yes. But it seems a little expensive to make them virtual only because of
testing. And I removed the Socket::socket() virtual function and implementations
of Socket::Read() and Socket::Write() in Socket class. So maybe the tests are
not very necessary anymore. What do you think?

[miket_OOO, 2012-04-24 20:49:05]

LGTM but first please clean up lots of style nits noted below.

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/4016/chrome/browser/exte...
chrome/browser/extensions/api/socket/udp_socket_unittest.cc:23: #if 0
If you think so, then delete them rather than #if 0'ing them out.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/api_function.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/api_function.cc:52: }
This seems to be a leftover from debugging.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/api_resource_event_notifier.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/api_resource_event_notifier.cc:52: LOG(ERROR) <<
"APIResourceEventNotifier::OnConnectComplete()" << result_code;
Leftover?

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/tcp_socket.cc:79: (net::IPEndPoint
*)NULL));
Please do this cast in C++ rather than C style.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/tcp_socket.cc:90: net::IPEndPoint *address)
{
* placement

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/socket/udp_socket.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket.cc:66: int result =
socket_->Bind(ip_end_point);
No need for temporary variable.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket.cc:80: (net::IPEndPoint
*)NULL));
cast

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket.cc:91: net::IPEndPoint *address)
{
* placement

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/socket/udp_socket.h (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket.h:35: int bytes) OVERRIDE;
indent

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket_unittest.cc:23: #if 0
Just delete. Use source control history if we want to recover.

https://chromiumcodereview.appspot.com/10134008/diff/30001/net/udp/udp_socket...
File net/udp/udp_socket_libevent.h (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/net/udp/udp_socket...
net/udp/udp_socket_libevent.h:24: class NET_EXPORT UDPSocketLibevent : public
base::NonThreadSafe {
Please check and make sure this is necessary.

[Peng, 2012-04-24 21:13:46]

Update the CL. Sorry for those style problems.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/api_function.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/api_function.cc:52: }
On 2012/04/24 20:49:06, miket wrote:
> This seems to be a leftover from debugging.

Done.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/api_resource_event_notifier.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/api_resource_event_notifier.cc:52: LOG(ERROR) <<
"APIResourceEventNotifier::OnConnectComplete()" << result_code;
On 2012/04/24 20:49:06, miket wrote:
> Leftover?

Done.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/tcp_socket.cc:79: (net::IPEndPoint
*)NULL));
On 2012/04/24 20:49:06, miket wrote:
> Please do this cast in C++ rather than C style.

Done.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/tcp_socket.cc:90: net::IPEndPoint *address)
{
On 2012/04/24 20:49:06, miket wrote:
> * placement

Done.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/socket/udp_socket.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket.cc:66: int result =
socket_->Bind(ip_end_point);
On 2012/04/24 20:49:06, miket wrote:
> No need for temporary variable.

Done.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket.cc:80: (net::IPEndPoint
*)NULL));
On 2012/04/24 20:49:06, miket wrote:
> cast

Done.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket.cc:91: net::IPEndPoint *address)
{
On 2012/04/24 20:49:06, miket wrote:
> * placement

Done.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/socket/udp_socket.h (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket.h:35: int bytes) OVERRIDE;
On 2012/04/24 20:49:06, miket wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
File chrome/browser/extensions/api/socket/udp_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/chrome/browser/ext...
chrome/browser/extensions/api/socket/udp_socket_unittest.cc:23: #if 0
On 2012/04/24 20:49:06, miket wrote:
> Just delete. Use source control history if we want to recover.

Done.

https://chromiumcodereview.appspot.com/10134008/diff/30001/net/udp/udp_socket...
File net/udp/udp_socket_libevent.h (right):

https://chromiumcodereview.appspot.com/10134008/diff/30001/net/udp/udp_socket...
net/udp/udp_socket_libevent.h:24: class NET_EXPORT UDPSocketLibevent : public
base::NonThreadSafe {
On 2012/04/24 20:49:06, miket wrote:
> Please check and make sure this is necessary.

It is necessary for building with GYP_DEFINES="component=shared_library".

[commit-bot: I haz the power, 2012-04-25 15:53:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10134008/47001

[commit-bot: I haz the power, 2012-04-25 16:31:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10134008/46039

[commit-bot: I haz the power, 2012-04-25 17:54:56]

Change committed as 133935