Real (but naive) UDP socket sending.

Changed some API names. Wrapped existing UDPClientSocket. Added one unit test for controller. The socket API itself isn't tested for two reasons: (1) this is an interim checkin, where Part 2 will add UDP receiving, and that will likely involve some API changes, so I didn't bother constructing a whole extension built on a temporary API; and (2) the wrapper itself is so thin around the existing UDPClientSocket API that a unit test of the wrapper wouldn't accomplish much.

BUG=none
TEST=see above.


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

[Mihai Parparita -not on Chrome, 2011-12-01 23:39:32]

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api.cc (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api.cc:9: #include
"chrome/browser/extensions/socket_api_constants.h"
Per Aaron separate constants.h/.cc files are no longer necessary.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api.cc:29: if (socket_type !=
constants::kUdpSocketType) {
Per Aaron's feedback in http://codereview.chromium.org/8682013/, he prefers
acronyms to be capitalized in names (this is also consistent with existing code
like UDPClientSocket).

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api.cc:40: void
SocketCreateFunction::WorkOnIOThread() {
Still too early to refactor this to avoid some of the repetition caused by
ping-ponging between threads?

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api.h (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api.h:23: SocketCreateFunction() {}
The style guide strongly discourages inline constructors and destructors:
http://www.chromium.org/developers/coding-style#TOC-Inline-functions

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api_controller.cc (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.cc:68: // We pass all errors,
including ERROR_IO_PENDING, back to the caller.
Based on my reading of
http://www.google.com/codesearch#OAMlx_jo-ck/src/net/socket/socket.h&l=35, if
ERROR_IO_PENDING returned, then the callback will be invoked when all bytes are
eventually written. It seems like we would want to give clients a way of getting
called for that too (or is this what you're planning on using events for?)

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.cc:103:
socket_map_[next_socket_id_] = socket;
Should you also be setting the socket's src_id?

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.cc:118: // leave experimental
without DNS resolution.
It's customary to add a "// static" line in the comments for static class
functions:
http://www.google.com/codesearch#search&q=//.static+file:extensions&exact_pac...

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api_controller.h (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.h:33: class Socket {
It seems like this class could live entirely inside of the .cc file (including
its declaration), since SocketController's public interface only exposes socket
IDs.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.h:38: void
set_src_extension_id(const std::string& src_extension_id) {
Having these properties as constructor parameters seems cleaner (presumably
they'll never change once the socket is created).

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.h:60: net::UDPClientSocket*
udp_client_socket_;
Will TCP socket functionality go into the same class? (I'm asking because I was
wondering if this class should be called UDPSocket and/or whether it should
serve as a base class for a UDPSocket class).

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.h:86: // "go out of scope" (in
quotes because they never do).
We could perhaps expose sockets as v8 objects, in which case it is possible to
get a callback when they're GC-ed. Maybe grab Aaron and I for some
brainstorming?

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_apitest.cc (left):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_apitest.cc:62:
CommandLine::ForCurrentProcess()->AppendSwitch(
This test doesn't inherit from SocketApiTest, so presumably you still need to
add the command line flags.

[Aaron Boodman, 2011-12-02 06:06:12]

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api.h (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_SOCKET_API_H_
I have been trying to figure out what to do with APIs like this one that are
owned by the extension team, and/or aren't tightly related to some other module
in Chrome.

Here's what I think:

chrome/browser/extensions/api/<foo>/*

If this sgty (and Mihai), please move this there and make yourself an OWNER.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api_controller.h (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.h:86: // "go out of scope" (in
quotes because they never do).
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> We could perhaps expose sockets as v8 objects, in which case it is possible to
> get a callback when they're GC-ed. Maybe grab Aaron and I for some
> brainstorming?

GC is non-deterministic so it's not usually a good choice for resources that
should be freed in a timely fashion.

The more JavaScript-y approach for this kind of thing is to abuse closures:

socket.create(..., function(...) {
  socket.write(..., function(...) {
    socket.read(...)
    socket.read(...)
    socket.write(...)
  });
});

The socket is kept alive as long as there is at least one callback pending on
it. When the callback count goes to zero, it is immediately destroyed.

Also, unrelated API nit: Is it possible to combine create and connect?

[miket_OOO, 2011-12-02 21:06:36]

Changes made, comments submitted, tests running. Please re-review.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api.cc (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api.cc:29: if (socket_type !=
constants::kUdpSocketType) {
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> Per Aaron's feedback in http://codereview.chromium.org/8682013/, he prefers
> acronyms to be capitalized in names (this is also consistent with existing
code
> like UDPClientSocket).

OK. I must have picked the wrong examples in the source code. :) Is "Id"
(kSocketId) an exception? It's not an acronym, and I can't recall seeing it
anywhere as "ID."

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api.cc:40: void
SocketCreateFunction::WorkOnIOThread() {
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> Still too early to refactor this to avoid some of the repetition caused by
> ping-ponging between threads?

Yes. But don't worry, it'll be done.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api.h (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_SOCKET_API_H_
> If this sgty (and Mihai), please move this there and make yourself an OWNER.

Done!

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api.h:23: SocketCreateFunction() {}
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> The style guide strongly discourages inline constructors and destructors:
> http://www.chromium.org/developers/coding-style#TOC-Inline-functions

Fixed.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api_controller.cc (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.cc:68: // We pass all errors,
including ERROR_IO_PENDING, back to the caller.
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> Based on my reading of
> http://www.google.com/codesearch#OAMlx_jo-ck/src/net/socket/socket.h&l=35, if
> ERROR_IO_PENDING returned, then the callback will be invoked when all bytes
are
> eventually written. It seems like we would want to give clients a way of
getting
> called for that too (or is this what you're planning on using events for?)

Ugh, yeah, you're right. I thought we were supposed to call Write() again. Yes,
we're going to have to use events. So this CL is going to be basically broken at
this point, but still I think it's worth continuing this particular review as a
checkpoint, because I didn't want to bite off events in this one.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.cc:103:
socket_map_[next_socket_id_] = socket;
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> Should you also be setting the socket's src_id?

No. I've removed src_id from this CL. I took it from TTS's Utterance class, and
I'm not convinced it's needed at this point. It has to do with events. I'm
leaving it out until I thoroughly understand events in a future CL.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.cc:118: // leave experimental
without DNS resolution.
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> It's customary to add a "// static" line in the comments for static class
> functions:
>
http://www.google.com/codesearch#search&q=//.static+file:extensions&exact_pac...

Done.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
File chrome/browser/extensions/socket_api_controller.h (right):

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.h:33: class Socket {
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> It seems like this class could live entirely inside of the .cc file (including
> its declaration), since SocketController's public interface only exposes
socket
> IDs.

Sounds great! Done.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.h:38: void
set_src_extension_id(const std::string& src_extension_id) {
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> Having these properties as constructor parameters seems cleaner (presumably
> they'll never change once the socket is created).

Done.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.h:60: net::UDPClientSocket*
udp_client_socket_;
On 2011/12/01 23:39:32, Mihai Parparita wrote:
> Will TCP socket functionality go into the same class? (I'm asking because I
was
> wondering if this class should be called UDPSocket and/or whether it should
> serve as a base class for a UDPSocket class). 

I've thought about the same thing. I'd like to do a bit of design work on the
TCP API before making a decision.

http://codereview.chromium.org/8743017/diff/5001/chrome/browser/extensions/so...
chrome/browser/extensions/socket_api_controller.h:86: // "go out of scope" (in
quotes because they never do).
> socket.create(..., function(...) {
>   socket.write(..., function(...) {
>     socket.read(...)
>     socket.read(...)
>     socket.write(...)
>   });
> });
> 
> The socket is kept alive as long as there is at least one callback pending on
> it. When the callback count goes to zero, it is immediately destroyed.

OK. I'll do that in a separate CL.

> Also, unrelated API nit: Is it possible to combine create and connect?

It's possible, yes, but let me compare the TCP API first to see whether we can
leverage any similarity between the two. It's attractive to have Create/Destroy
be symmetric in the sense that they're associated with memory allocation, but on
the other hand I could see the argument that the Connect arguments are like
constructor arguments that every socket's going to need anyway.

[Aaron Boodman, 2011-12-03 00:07:23]

LGTM

On 2011/12/02 21:06:36, miket wrote:
> It's attractive to have Create/Destroy
> be symmetric in the sense that they're associated with memory allocation

But if there's no explicit Destroy...

[miket_OOO, 2011-12-03 00:17:20]

> But if there's no explicit Destroy...

Ah, you meant ALL your comments to be taken TOGETHER as if they existed in the
SAME universe. Yeah, gotcha, when the closure refcounting stuff is in, then I'll
collapse all the setup into Create. Thanks!

[Aaron Boodman, 2011-12-03 00:17:37]

http://codereview.chromium.org/8743017/diff/8019/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/OWNERS (right):

http://codereview.chromium.org/8743017/diff/8019/chrome/browser/extensions/ap...
chrome/browser/extensions/api/OWNERS:2: aa@chromium.org
Now that I see this, I guess it is silly since it's all the same people from
chrome/browser/extensions.

Nevermind, you can remove this.

[miket_OOO, 2011-12-03 00:42:49]

> Nevermind, you can remove this.

Teams will probably dive deep on specific APIs, but then we can have
api/<foo>-specific owners.

[commit-bot: I haz the power, 2011-12-05 22:12:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miket@chromium.org/8743017/20003

[Mihai Parparita -not on Chrome, 2011-12-05 22:43:33]

(Belated) LGTM, feel free to fix the nit in a follow-up CL.

http://codereview.chromium.org/8743017/diff/20003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_api_controller.cc (right):

http://codereview.chromium.org/8743017/diff/20003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api_controller.cc:25: explicit
Socket(const Profile* profile, const std::string& src_extension_id,
Nit: explicit should no longer be needed.

[miket_OOO, 2011-12-05 22:47:11]

> Nit: explicit should no longer be needed.

Good catch, thanks. Trybots are green so I'll follow up later.

[commit-bot: I haz the power, 2011-12-06 00:15:41]

Change committed as 113069

[miket_OOO, 2011-12-06 22:03:53]

> (Belated) LGTM, feel free to fix the nit in a follow-up CL.
>
> chrome/browser/extensions/api/socket/socket_api_controller.cc:25: explicit
> Socket(const Profile* profile, const std::string& src_extension_id,
> Nit: explicit should no longer be needed.

This fix will land in http://codereview.chromium.org/8819029.