Allow socket API to send binary data.

- Changed the data structure being passed around in the socket API from a string to an array of longs. This is a stepping stone to get us to ArrayBuffers.

- Changed "message" key to "data" for consistency.

BUG=120567
TEST=no new ones, but existing, as updated, still pass.

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

[miket_OOO, 2012-04-16 20:15:23]

In particular, please scrutinize the scoped_refptr usage. I think it's correct,
but I could use the second set of eyes.

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:174: unsigned char*
data_buffer =
Note reinterpret_cast. I think this is defensible; we're intentionally changing
a signed type to binary octets. Or if there's a Chromium coding convention that
I'm not aware of, perhaps there's a better way.

[miket_OOO, 2012-04-16 20:21:07]

I changed reviewers from Peng to Jeremy because this CL will break IRC!

[jeremya, 2012-04-16 22:28:26]

lgtm, can't wait for ArrayBuffers :)

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

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket.cc:50:
base::Bind(&Socket::OnDataRead, base::Unretained(this), io_buffer));
Is it possible that this read will call the callback after this Socket object
has been destructed (e.g. the page is closed, or the socket is closed, etc.)?

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket.cc:64: return bytes_sent > 0 ?
bytes_sent : result;
(Side note, since you don't change this behaviour in this patch, but: what
happens if we get a number of positive results from Write(), followed by an
error that is not ERR_IO_PENDING? How does the client detect the error?)

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:130: const int buffer_len =
2048;
FWIW, some noodling around with getsockopt(SO_RCVBUF) on my machine tells me
that the kernel keeps a receive buffer of nearly 256kb (4 bytes shy).

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:172: if (size != 0) {
https://memegen.googleplex.com/5604701

[asargent_no_longer_on_chrome, 2012-04-16 22:51:13]

lgtm

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:174: unsigned char*
data_buffer =
On 2012/04/16 20:15:23, miket wrote:
> Note reinterpret_cast. I think this is defensible; we're intentionally
changing
> a signed type to binary octets. Or if there's a Chromium coding convention
that
> I'm not aware of, perhaps there's a better way.

Seems ok, but I think it might be better to use uint8 instead of unsigned char.

(As an aside, you will be frightened to hear that in some places we actually use
std::string to pass around binary data because, hey, here's a convenient wrapper
for a variable number of bytes with a length property).

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:193: } else {
nit: looks like elsewhere in this file you use the no-braces style for
single-line if/else body, so you should remain consistent here

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_api.h (right):

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.h:10: #include "base/values.h"
nit: do you need to include values.h here? I don't see any *Value types being
used except for in the .cc file.

http://codereview.chromium.org/10095020/diff/2003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/socket/api/background.js (right):

http://codereview.chromium.org/10095020/diff/2003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/socket/api/background.js:31: }
nit: add semicolon after } here

http://codereview.chromium.org/10095020/diff/2003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/socket/api/background.js:41: }
same nit here

[miket_OOO, 2012-04-16 23:15:53]

Rebuilding, will submit. Thanks.

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

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket.cc:50:
base::Bind(&Socket::OnDataRead, base::Unretained(this), io_buffer));
On 2012/04/16 22:28:27, jeremya wrote:
> Is it possible that this read will call the callback after this Socket object
> has been destructed (e.g. the page is closed, or the socket is closed, etc.)?

I can't point you to the code that proves it, but sockets are not threadsafe and
aggressively assert to that effect. So if there were an ordering problem, it
would be a bug in the socket code (not extensions::socket but net::socket). I am
not aware of buggy behavior like this.

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket.cc:64: return bytes_sent > 0 ?
bytes_sent : result;
On 2012/04/16 22:28:27, jeremya wrote:
> (Side note, since you don't change this behaviour in this patch, but: what
> happens if we get a number of positive results from Write(), followed by an
> error that is not ERR_IO_PENDING? How does the client detect the error?)

We're OK, but I think the code needs to be refactored. The client learns of the
error by getting a negative number, but the number of bytes successfully written
so far is lost. I should stick to our goal of mirroring the underlying socket
API and get rid of the whole DrainableIOBuffer. Instead we should make a single
Write call and return that result. I'll do that.

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:174: unsigned char*
data_buffer =
On 2012/04/16 22:51:13, Antony Sargent wrote:
> On 2012/04/16 20:15:23, miket wrote:
> > Note reinterpret_cast. I think this is defensible; we're intentionally
> changing
> > a signed type to binary octets. Or if there's a Chromium coding convention
> that
> > I'm not aware of, perhaps there's a better way.
> 
> Seems ok, but I think it might be better to use uint8 instead of unsigned
char.

Done.

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:193: } else {
On 2012/04/16 22:51:13, Antony Sargent wrote:
> nit: looks like elsewhere in this file you use the no-braces style for
> single-line if/else body, so you should remain consistent here

Right! I was blindly listening to the lint checker. (how does one "blindly"
listen?)

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/socket_api.h (right):

http://codereview.chromium.org/10095020/diff/2003/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.h:10: #include "base/values.h"
On 2012/04/16 22:51:13, Antony Sargent wrote:
> nit: do you need to include values.h here? I don't see any *Value types being
> used except for in the .cc file.

Right you are!

http://codereview.chromium.org/10095020/diff/2003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/socket/api/background.js (right):

http://codereview.chromium.org/10095020/diff/2003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/socket/api/background.js:31: }
On 2012/04/16 22:51:13, Antony Sargent wrote:
> nit: add semicolon after } here

That is insanely stupid code. I am not going to dumb down background.js for this
case.

[commit-bot: I haz the power, 2012-04-16 23:56:20]

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

[commit-bot: I haz the power, 2012-04-17 02:07:49]

Change committed as 132512