Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
590263002:
Mojo UDP socket API definition. (Closed)
|
Created:
6 years, 3 months ago by yzshen1 Modified:
6 years, 2 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionMojo UDP socket API definition.
BUG=402671
TEST=None
Committed: https://crrev.com/6e21296a8e024684f3b15ce8e562ad0104d48de3
Cr-Commit-Position: refs/heads/master@{#297286}
Patch Set 1 #
Total comments: 15
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 1
Messages
Total messages: 18 (4 generated)
yzshen@chromium.org changed reviewers: + darin@chromium.org
Hi, Darin. Would you please take a look? Thanks! https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/net_address.mojom (right): https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/net_address.mojom:27: NetAddressIPv4? ipv4; This approach has the disadvantage that whenever we add a new address type, we will have another pointer and waste 8 more bytes. (Do we want union support?) Another approach is to have a uint8 array here. And then the user can use our binding helpers to Deserialize() it into NetAddressIPv4 or NetAddressIPv6, depending on the address type. Which do you prefer? Thanks!
https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:53: SendTo(NetAddress addr, uint8[] data) => (NetworkError result); NetworkError here seems to be consistent with other network APIs. However, it is probably a waste: I don't think users need the description string in most cases. I prefer to just return an error code. If we need to map error code to a string, we could consider a utility function/class in a client library, or a method on the NetworkService interface. What do you think?
Friendly ping, Darin. :)
yzshen@chromium.org changed reviewers: + brettw@chromium.org
+Brett as reviewer Thanks Brett!
https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/net_address.mojom (right): https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/net_address.mojom:27: NetAddressIPv4? ipv4; I like the way you did it better, but it seems like there should be some comments here explaining that only one is valid depending on the "family" https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:12: // Sets the send buffer size (in bytes) for the socket. Does this need to be called before bind or can it be any time? Should be documented Same for the other ones. https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:25: // other processes. Should be called before Bind(). I don't really understand what this means or what the implications are for using this. https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:30: Bind(NetAddress addr) => (NetworkError result, NetAddress? bound_addr); It's not quite clear to me how I use this class. Is it possible to write a class-level comment and have it appear in the generated files? I'm looking for an overall flow, like "create, set buffer sizes, bind, send/receive". https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:53: SendTo(NetAddress addr, uint8[] data) => (NetworkError result); I like the network error.
https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/net_address.mojom (right): https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/net_address.mojom:13: // All members are expressed in network byte order. Can you expand on this or give an example. Some users won't know what this means.
Thanks Brett. Please take another look! https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/net_address.mojom (right): https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/net_address.mojom:13: // All members are expressed in network byte order. On 2014/09/29 17:49:16, brettw wrote: > Can you expand on this or give an example. Some users won't know what this > means. Done. https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/net_address.mojom:27: NetAddressIPv4? ipv4; On 2014/09/29 17:41:22, brettw wrote: > I like the way you did it better, but it seems like there should be some > comments here explaining that only one is valid depending on the "family" Done. https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:12: // Sets the send buffer size (in bytes) for the socket. On 2014/09/29 17:41:22, brettw wrote: > Does this need to be called before bind or can it be any time? Should be > documented Same for the other ones. Right. Thanks! https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:25: // other processes. Should be called before Bind(). Pepper UDP socket users don't seem to have problem with the same comment. It is basically SO_REUSEADDR. I decided to not comment more than the /net UDP socket API documentation, otherwise I may need to go into the details like http://stackoverflow.com/questions/14388706/socket-options-so-reuseaddr-and-s... Or do you think it is okay to refer to SO_REUSEADDR directly? On 2014/09/29 17:41:22, brettw wrote: > I don't really understand what this means or what the implications are for using > this. https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:30: Bind(NetAddress addr) => (NetworkError result, NetAddress? bound_addr); On 2014/09/29 17:41:22, brettw wrote: > It's not quite clear to me how I use this class. Is it possible to write a > class-level comment and have it appear in the generated files? I'm looking for > an overall flow, like "create, set buffer sizes, bind, send/receive". Done. Please let me know whether it looks okay. Thanks!
lgtm https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:25: // other processes. Should be called before Bind(). I think mentioning that this is equivalent to SO_REUSEADDR is good, since then it's something I can look up on Google and get all the background I need.
Thanks, Brett! https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/590263002/diff/1/mojo/services/public/interfa... mojo/services/public/interfaces/network/udp_socket.mojom:25: // other processes. Should be called before Bind(). On 2014/09/29 19:55:33, brettw wrote: > I think mentioning that this is equivalent to SO_REUSEADDR is good, since then > it's something I can look up on Google and get all the background I need. Done.
The CQ bit was checked by yzshen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/590263002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 4e9a792dd382696386eca72dca302200a349576f
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6e21296a8e024684f3b15ce8e562ad0104d48de3 Cr-Commit-Position: refs/heads/master@{#297286}
Message was sent while issue was closed.
willchan@chromium.org changed reviewers: + willchan@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/590263002/diff/60001/mojo/services/public/int... File mojo/services/public/interfaces/network/udp_socket.mojom (right): https://codereview.chromium.org/590263002/diff/60001/mojo/services/public/int... mojo/services/public/interfaces/network/udp_socket.mojom:35: // doesn't guarantee it will conform to the size. What does this note mean? It's not obvious if the send buffer here refers to the OS kernel send buffer or the mojo service send buffer (if one exists, does it?). How are you handling backpressure (kernel buffers filling up, and notifying when they have space again)?
Message was sent while issue was closed.
Oops, I sent this to the wrong CL, sorry. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
