Mojo UDP API implementation.

Mojo UDP API implementation.

BUG=402671
TEST=udp_socket_unittest.cc (this CL hasn't run that on the bots)

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

[yzshen1, 2014-09-25 18:22:39]

yzshen@chromium.org changed reviewers:
+ alokp@chromium.org, mikhal@chromium.org

[yzshen1, 2014-09-25 18:25:11]

yzshen@chromium.org changed reviewers:
+ darin@chromium.org, mpcomplete@google.com

[yzshen1, 2014-09-25 18:25:12]

Hi,

Would you please take a look? Thanks!
This CL depends on https://codereview.chromium.org/590263002/

Darin: general design
Matt: everything
Alok and Mikhal: FYI, but suggestions are welcomed!

[yzshen1, 2014-09-26 22:00:21]

yzshen@chromium.org changed reviewers:
+ brettw@chromium.org

[yzshen1, 2014-09-26 22:00:21]

+Brett as reviewer

Thanks Brett!

[brettw, 2014-09-29 18:24:26]

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ne...
File mojo/services/network/net_address_type_converters.cc (right):

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ne...
mojo/services/network/net_address_type_converters.cc:21: 
I think it would be easier to follow if you moved this blank line to after the
return. Same for the other case.

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
File mojo/services/network/udp_socket_impl.cc (right):

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.cc:58: if (size >
static_cast<uint32_t>(std::numeric_limits<int32>::max()))
I think we're recommending int32_t for new code. Same below in multiple cases.

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.cc:148: if (pending_send_requests_.size()
>= kMaxPendingSendRequests) {
It's not clear to me from the .mojom that there would be a maximum number of
outgoing requests, It makes sense, but it's not obvious and this is a
significant downside to using the no-callback version. Can we specifically
mention this error in the SendTo? All other errors are either "you used this
wrong" or "the network silently dropped your packet" so I feel like this one is
more special in this regard since we're explicitly telling you we dropped your
packet.

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
File mojo/services/network/udp_socket_impl.h (right):

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.h:77: scoped_refptr<net::IOBufferWithSize>
sendto_buffer_;
This seems to be used in a nontrivial way and has some interesting invariants.
These should be documented here.

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.h:81: size_t remaining_recv_slots_;
This one isn't self-explanatory to me.

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.h:83: std::queue<PendingSendRequest*>
pending_send_requests_;
Can you clarify these are owning pointers?

[yzshen1, 2014-09-29 21:51:12]

Thanks Brett!

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ne...
File mojo/services/network/net_address_type_converters.cc (right):

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ne...
mojo/services/network/net_address_type_converters.cc:21: 
On 2014/09/29 18:24:25, brettw wrote:
> I think it would be easier to follow if you moved this blank line to after the
> return. Same for the other case.

Done.

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
File mojo/services/network/udp_socket_impl.cc (right):

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.cc:58: if (size >
static_cast<uint32_t>(std::numeric_limits<int32>::max()))
On 2014/09/29 18:24:25, brettw wrote:
> I think we're recommending int32_t for new code. Same below in multiple cases.

I think you mean "changing int32 to int32_t" instead of "changing uint32_t to
int32_t", right?

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.cc:148: if (pending_send_requests_.size()
>= kMaxPendingSendRequests) {
On 2014/09/29 18:24:25, brettw wrote:
> It's not clear to me from the .mojom that there would be a maximum number of
> outgoing requests, It makes sense, but it's not obvious and this is a
> significant downside to using the no-callback version. Can we specifically
> mention this error in the SendTo? All other errors are either "you used this
> wrong" or "the network silently dropped your packet" so I feel like this one
is
> more special in this regard since we're explicitly telling you we dropped your
> packet.

Done. Comments added.

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
File mojo/services/network/udp_socket_impl.h (right):

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.h:77: scoped_refptr<net::IOBufferWithSize>
sendto_buffer_;
On 2014/09/29 18:24:26, brettw wrote:
> This seems to be used in a nontrivial way and has some interesting invariants.
> These should be documented here.

Done.

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.h:81: size_t remaining_recv_slots_;
On 2014/09/29 18:24:26, brettw wrote:
> This one isn't self-explanatory to me.

Done.

https://codereview.chromium.org/596383002/diff/20001/mojo/services/network/ud...
mojo/services/network/udp_socket_impl.h:83: std::queue<PendingSendRequest*>
pending_send_requests_;
On 2014/09/29 18:24:26, brettw wrote:
> Can you clarify these are owning pointers?

Done.

[Matt Perry, 2014-09-29 23:39:07]

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

[Matt Perry, 2014-09-29 23:39:08]

https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
File mojo/services/public/interfaces/network/udp_socket.mojom (right):

https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
mojo/services/public/interfaces/network/udp_socket.mojom:55: SendTo(NetAddress
addr, uint8[] data) => (NetworkError result);
Have you considered using a DataPipe for the data transfer? If so, I'm curious
what made you decide to send the data over the message pipe. I have a similar
interface in the WebSocket API, and used DataPipes. The DataPipe way does end up
being more complicated, unfortunately.

[yzshen1, 2014-09-29 23:54:58]

On 2014/09/29 23:39:08, Matt Perry wrote:
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> File mojo/services/public/interfaces/network/udp_socket.mojom (right):
> 
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> mojo/services/public/interfaces/network/udp_socket.mojom:55: SendTo(NetAddress
> addr, uint8[] data) => (NetworkError result);
> Have you considered using a DataPipe for the data transfer? If so, I'm curious
> what made you decide to send the data over the message pipe. I have a similar
> interface in the WebSocket API, and used DataPipes. The DataPipe way does end
up
> being more complicated, unfortunately.

One issue making the data pipe approach more complicated is that the data is
structural.
I understand that some people in our team has already been working on sending
structural data over DataPipe.
But without performance testing, I am not sure whether that is more efficient.
I decided to write out this simple version first and if it proves to be slow, I
can switch later.

[Matt Perry, 2014-09-30 00:09:57]

On 2014/09/29 23:54:58, yzshen1 wrote:
> On 2014/09/29 23:39:08, Matt Perry wrote:
> >
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> > File mojo/services/public/interfaces/network/udp_socket.mojom (right):
> > 
> >
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> > mojo/services/public/interfaces/network/udp_socket.mojom:55:
SendTo(NetAddress
> > addr, uint8[] data) => (NetworkError result);
> > Have you considered using a DataPipe for the data transfer? If so, I'm
curious
> > what made you decide to send the data over the message pipe. I have a
similar
> > interface in the WebSocket API, and used DataPipes. The DataPipe way does
end
> up
> > being more complicated, unfortunately.
> 
> One issue making the data pipe approach more complicated is that the data is
> structural.

What do you mean by this? I'm just referring to the uint8 data array. That's not
structural, is it?

> I understand that some people in our team has already been working on sending
> structural data over DataPipe.
> But without performance testing, I am not sure whether that is more efficient.
> I decided to write out this simple version first and if it proves to be slow,
I
> can switch later.

Sure, sounds good.

LGTM

[yzshen1, 2014-09-30 00:25:20]

On 2014/09/30 00:09:57, Matt Perry wrote:
> On 2014/09/29 23:54:58, yzshen1 wrote:
> > On 2014/09/29 23:39:08, Matt Perry wrote:
> > >
> >
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> > > File mojo/services/public/interfaces/network/udp_socket.mojom (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> > > mojo/services/public/interfaces/network/udp_socket.mojom:55:
> SendTo(NetAddress
> > > addr, uint8[] data) => (NetworkError result);
> > > Have you considered using a DataPipe for the data transfer? If so, I'm
> curious
> > > what made you decide to send the data over the message pipe. I have a
> similar
> > > interface in the WebSocket API, and used DataPipes. The DataPipe way does
> end
> > up
> > > being more complicated, unfortunately.
> > 
> > One issue making the data pipe approach more complicated is that the data is
> > structural.
> 
> What do you mean by this? I'm just referring to the uint8 data array. That's
not
> structural, is it?

Right. But along with each packet we need to also send an address (dest addr for
send; src addr for receive).
Therefore if we want to send {addr, data} on the data pipe, it is not a
non-structrual byte stream.


> 
> > I understand that some people in our team has already been working on
sending
> > structural data over DataPipe.
> > But without performance testing, I am not sure whether that is more
efficient.
> > I decided to write out this simple version first and if it proves to be
slow,
> I
> > can switch later.
> 
> Sure, sounds good.
> 
> LGTM

[Matt Perry, 2014-09-30 00:30:45]

On 2014/09/30 00:25:20, yzshen1 wrote:
> On 2014/09/30 00:09:57, Matt Perry wrote:
> > On 2014/09/29 23:54:58, yzshen1 wrote:
> > > On 2014/09/29 23:39:08, Matt Perry wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> > > > File mojo/services/public/interfaces/network/udp_socket.mojom (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> > > > mojo/services/public/interfaces/network/udp_socket.mojom:55:
> > SendTo(NetAddress
> > > > addr, uint8[] data) => (NetworkError result);
> > > > Have you considered using a DataPipe for the data transfer? If so, I'm
> > curious
> > > > what made you decide to send the data over the message pipe. I have a
> > similar
> > > > interface in the WebSocket API, and used DataPipes. The DataPipe way
does
> > end
> > > up
> > > > being more complicated, unfortunately.
> > > 
> > > One issue making the data pipe approach more complicated is that the data
is
> > > structural.
> > 
> > What do you mean by this? I'm just referring to the uint8 data array. That's
> not
> > structural, is it?
> 
> Right. But along with each packet we need to also send an address (dest addr
for
> send; src addr for receive).
> Therefore if we want to send {addr, data} on the data pipe, it is not a
> non-structrual byte stream.

Right. The way I handled that was to send the structured data in the message
pipe, and the unstructured in the data pipe. The receiving end coordinates the
message pipe read with the data pipe read (they are both FIFO so the 2 messages
will arrive in the same sequence). If you're curious, here's the CL
https://codereview.chromium.org/550003005/

But your way is definitely simpler, so fine unless we get some performance
numbers that say otherwise.

[yzshen1, 2014-09-30 16:29:04]

On 2014/09/30 00:30:45, Matt Perry wrote:
> On 2014/09/30 00:25:20, yzshen1 wrote:
> > On 2014/09/30 00:09:57, Matt Perry wrote:
> > > On 2014/09/29 23:54:58, yzshen1 wrote:
> > > > On 2014/09/29 23:39:08, Matt Perry wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> > > > > File mojo/services/public/interfaces/network/udp_socket.mojom (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/596383002/diff/20001/mojo/services/public/int...
> > > > > mojo/services/public/interfaces/network/udp_socket.mojom:55:
> > > SendTo(NetAddress
> > > > > addr, uint8[] data) => (NetworkError result);
> > > > > Have you considered using a DataPipe for the data transfer? If so, I'm
> > > curious
> > > > > what made you decide to send the data over the message pipe. I have a
> > > similar
> > > > > interface in the WebSocket API, and used DataPipes. The DataPipe way
> does
> > > end
> > > > up
> > > > > being more complicated, unfortunately.
> > > > 
> > > > One issue making the data pipe approach more complicated is that the
data
> is
> > > > structural.
> > > 
> > > What do you mean by this? I'm just referring to the uint8 data array.
That's
> > not
> > > structural, is it?
> > 
> > Right. But along with each packet we need to also send an address (dest addr
> for
> > send; src addr for receive).
> > Therefore if we want to send {addr, data} on the data pipe, it is not a
> > non-structrual byte stream.
> 
> Right. The way I handled that was to send the structured data in the message
> pipe, and the unstructured in the data pipe. The receiving end coordinates the
> message pipe read with the data pipe read (they are both FIFO so the 2
messages
> will arrive in the same sequence). If you're curious, here's the CL
> https://codereview.chromium.org/550003005/
> 
> But your way is definitely simpler, so fine unless we get some performance
> numbers that say otherwise.

Thanks for the pointer! I will read the code.

[yzshen1, 2014-10-07 23:44:36]

Hi, Brett.

I have updated the code according to the API change. Please take a look. Thanks!

[brettw, 2014-10-08 00:37:09]

lgtm

[yzshen1, 2014-10-09 21:50:08]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-10-09 21:51:51]

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

[commit-bot: I haz the power, 2014-10-09 22:07:17]

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

[commit-bot: I haz the power, 2014-10-09 22:07:19]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[yzshen1, 2014-10-09 22:13:38]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-10-09 22:15:36]

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

[commit-bot: I haz the power, 2014-10-09 22:29:46]

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

[commit-bot: I haz the power, 2014-10-09 22:29:48]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[yzshen1, 2014-10-09 23:46:47]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-10-09 23:48:48]

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

[commit-bot: I haz the power, 2014-10-10 01:09:52]

Committed patchset #8 (id:580001)

[commit-bot: I haz the power, 2014-10-10 02:04:48]

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