Implement chrome.socket.bind/listen/accept for TCP server socket.

Implement chrome.socket.listen/accept for TCP server socket.

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

[justinlin, 2012-08-17 00:25:20]

Hi Peng, Mike, Mihai,

This is still very much work in progress, but I wanted to gather some initial
feedback and see whether this interface makes sense. Also, this is one of my
first chrome bugs, so please let me know if there's anything I'm blatantly
forgetting to do. Thanks!

[Peng, 2012-08-18 12:17:12]

Those two API look good to me. But it is not very clear how to use them. Could
please give a simple demo code?

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/socket.idl (right):

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/common/extensio...
chrome/common/extensions/api/socket.idl:145: long backlog);
No callback?

[Peng, 2012-08-18 12:17:14]

Those two API look good to me. But it is not very clear how to use them. Could
please give a simple demo code?

[miket_OOO, 2012-08-31 23:18:01]

Drive-by, as you requested. I know you're still working on this, but I couldn't
help myself and made some nit style comments. Otherwise, looks like you're on
the right track.

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/socket/socket.cc (right):

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/socket/socket.cc:93: void Socket::Accept(const
AcceptCompletionCallback &callback) {
whitespace (& callback)

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/socket/socket_api.cc:228:
EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &socket_id_));
You can do an EXTENSION_FUNCTION_VALIDATE(params_.get()) if that's easier, and
then use fewer custom member variables.

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/socket/socket_api.cc:249: }
vertical whitespace

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/socket/socket_api.cc:276:
result->SetInteger(kSocketIdKey, manager_->Add(client_socket));
You might want to add a note to
http://code.google.com/p/chromium/issues/detail?id=142521 and make sure that
these sockets are added to whatever is-OK list is part of the solution to that
bug. Charles is going to be working on that bug very soon, and he might make
your code mysteriously stop working if you guys don't keep in sync.

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/socket/tcp_socket.cc:185: return false;
An error message would be nice.

https://chromiumcodereview.appspot.com/10827390/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/socket/tcp_socket.cc:207: // Limits to only 1
blocked accept call.
Should this be a TODO to allow multiple? (I'm not clear whether this means that
multiple concurrent connection attempts will fail. Does the queueing up happen
lower, at the socket level? Probably.)

[justinlin, 2012-09-11 04:32:31]

Hi Mike, Peng,

Please take another look. Addressed comments and added tests. Note: this depends
on this change landing for the tests to pass consistently
(http://codereview.chromium.org/10907154/)

Thanks!

http://codereview.chromium.org/10827390/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10827390/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/socket/socket.cc:93: void Socket::Accept(const
AcceptCompletionCallback &callback) {
On 2012/08/31 23:18:01, miket wrote:
> whitespace (& callback)

Done.

http://codereview.chromium.org/10827390/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/socket/socket_api.cc:228:
EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &socket_id_));
On 2012/08/31 23:18:01, miket wrote:
> You can do an EXTENSION_FUNCTION_VALIDATE(params_.get()) if that's easier, and
> then use fewer custom member variables.

Done.

http://codereview.chromium.org/10827390/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/socket/socket_api.cc:249: }
On 2012/08/31 23:18:01, miket wrote:
> vertical whitespace

Done.

http://codereview.chromium.org/10827390/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/socket/socket_api.cc:276:
result->SetInteger(kSocketIdKey, manager_->Add(client_socket));
I'll keep an eye out for that issue. I'm not entirely sure how that will be
implemented, but there shouldn't be issues since this will look like any other
socket.

On 2012/08/31 23:18:01, miket wrote:
> You might want to add a note to
> http://code.google.com/p/chromium/issues/detail?id=142521 and make sure that
> these sockets are added to whatever is-OK list is part of the solution to that
> bug. Charles is going to be working on that bug very soon, and he might make
> your code mysteriously stop working if you guys don't keep in sync.

http://codereview.chromium.org/10827390/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

http://codereview.chromium.org/10827390/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/socket/tcp_socket.cc:185: return false;
On 2012/08/31 23:18:01, miket wrote:
> An error message would be nice.

Done.

http://codereview.chromium.org/10827390/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/socket/tcp_socket.cc:207: // Limits to only 1
blocked accept call.
Yea, the number of connections held in the queue is determined by the backlog
argument to the Listen function. I think client-side code would get messy if you
allow multiple and js is single threaded anyway, so I'm not sure there's any
point to allow this.

On 2012/08/31 23:18:01, miket wrote:
> Should this be a TODO to allow multiple? (I'm not clear whether this means
that
> multiple concurrent connection attempts will fail. Does the queueing up happen
> lower, at the socket level? Probably.)

http://codereview.chromium.org/10827390/diff/1/chrome/common/extensions/api/s...
File chrome/common/extensions/api/socket.idl (right):

http://codereview.chromium.org/10827390/diff/1/chrome/common/extensions/api/s...
chrome/common/extensions/api/socket.idl:145: long backlog);
On 2012/08/18 12:17:12, Peng wrote:
> No callback?

Done.

[Peng, 2012-09-11 14:01:18]

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

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:150:
manager_->Remove(socket_id_);
Could we check if the listen port is release after destroy.

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

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.h:166:
DECLARE_EXTENSION_FUNCTION_NAME("socket.listen");
maybe experimental first?

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:73:
DCHECK(server_socket_.get() == NULL);
DCHECK(!server_socket_.get())

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:91: if (bind_address_.get()
!= NULL) {
if(bind_address_.get())

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:92: if
(socket_->Bind(*bind_address_) != 0) {
Is there an ERR code for 0? like net::OK

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:114: if (bind_address_.get()
!= NULL) {
ditto

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:190: int
TCPSocket::Listen(int backlog, std::string& error_msg) {
I remember our coding style recommend to use  pointer instead of ref for out
argument.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:195: DCHECK(socket_.get() ==
NULL);
ditto

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:198: if (bind_address_.get()
== NULL) {
ditto

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:202: if (server_socket_.get()
== NULL) {
ditto

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:207: if (result != 0) {
ditto

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:214: if (!is_server_socket_
|| server_socket_.get() == NULL) {
ditto

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:247: if (socket_.get() !=
NULL) {
ditto

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:249: } else if
(server_socket_.get() != NULL) {
ditto

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/tcp_socket.h (right):

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.h:81: bool is_server_socket_;
Could we use one variable? Like int socket_mode_, it could be unknown, client
and server. I think it could make code simpler.

[justinlin, 2012-09-12 07:29:42]

Thanks for the review, PTAL.

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

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:150:
manager_->Remove(socket_id_);
I think the only way is to bind() again, but there's no easy way to do this
here.

On 2012/09/11 14:01:18, Peng wrote:
> Could we check if the listen port is release after destroy.

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

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.h:166:
DECLARE_EXTENSION_FUNCTION_NAME("socket.listen");
On 2012/09/11 14:01:18, Peng wrote:
> maybe experimental first?

Sure, but it doesn't seem to work when I just add "socket.experimental.listen"
here. The browser tests fail saying "listen" is undefined. Do you know how to
fix this?

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:73:
DCHECK(server_socket_.get() == NULL);
On 2012/09/11 14:01:18, Peng wrote:
> DCHECK(!server_socket_.get())

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:91: if (bind_address_.get()
!= NULL) {
On 2012/09/11 14:01:18, Peng wrote:
> if(bind_address_.get())

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:92: if
(socket_->Bind(*bind_address_) != 0) {
On 2012/09/11 14:01:18, Peng wrote:
> Is there an ERR code for 0? like net::OK

The function explicitly just returns 0, but net::OK would work too.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:114: if (bind_address_.get()
!= NULL) {
On 2012/09/11 14:01:18, Peng wrote:
> ditto

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:190: int
TCPSocket::Listen(int backlog, std::string& error_msg) {
On 2012/09/11 14:01:18, Peng wrote:
> I remember our coding style recommend to use  pointer instead of ref for out
> argument.
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:195: DCHECK(socket_.get() ==
NULL);
On 2012/09/11 14:01:18, Peng wrote:
> ditto

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:198: if (bind_address_.get()
== NULL) {
On 2012/09/11 14:01:18, Peng wrote:
> ditto

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:202: if (server_socket_.get()
== NULL) {
On 2012/09/11 14:01:18, Peng wrote:
> ditto

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:207: if (result != 0) {
On 2012/09/11 14:01:18, Peng wrote:
> ditto

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:214: if (!is_server_socket_
|| server_socket_.get() == NULL) {
On 2012/09/11 14:01:18, Peng wrote:
> ditto

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:247: if (socket_.get() !=
NULL) {
On 2012/09/11 14:01:18, Peng wrote:
> ditto

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.cc:249: } else if
(server_socket_.get() != NULL) {
On 2012/09/11 14:01:18, Peng wrote:
> ditto

Done.

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
File chrome/browser/extensions/api/socket/tcp_socket.h (right):

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/tcp_socket.h:81: bool is_server_socket_;
On 2012/09/11 14:01:18, Peng wrote:
> Could we use one variable? Like int socket_mode_, it could be unknown, client
> and server. I think it could make code simpler.

Done.

[Peng, 2012-09-12 19:49:49]

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

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:150:
manager_->Remove(socket_id_);
On 2012/09/12 07:29:42, justinlin wrote:
> I think the only way is to bind() again, but there's no easy way to do this
> here.
> 
> On 2012/09/11 14:01:18, Peng wrote:
> > Could we check if the listen port is release after destroy.
> 

That's fine. I think we could file an issue to remind us. So we could do it
later.

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

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.h:166:
DECLARE_EXTENSION_FUNCTION_NAME("socket.listen");
On 2012/09/12 07:29:42, justinlin wrote:
> On 2012/09/11 14:01:18, Peng wrote:
> > maybe experimental first?
> 
> Sure, but it doesn't seem to work when I just add "socket.experimental.listen"
> here. The browser tests fail saying "listen" is undefined. Do you know how to
> fix this?

I think you need create another idl file which just includes those new added
API. Like experimental_media_galleries.idl and media_galleries.idl in
https://cs.corp.google.com/#chrome/src/chrome/common/extensions/api/&sq=packa...

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:292: Socket* socket =
manager_->Get(extension_id(), params_->socket_id);
GetSocket()?

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:315: Socket* socket =
manager_->Get(extension_id(), params_->socket_id);
ditto

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:76:
DCHECK(server_socket_.get() == NULL);
Done?

[justinlin, 2012-09-13 08:21:12]

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

http://codereview.chromium.org/10827390/diff/9002/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.h:166:
DECLARE_EXTENSION_FUNCTION_NAME("socket.listen");
On 2012/09/12 19:49:49, Peng wrote:
> On 2012/09/12 07:29:42, justinlin wrote:
> > On 2012/09/11 14:01:18, Peng wrote:
> > > maybe experimental first?
> > 
> > Sure, but it doesn't seem to work when I just add
"socket.experimental.listen"
> > here. The browser tests fail saying "listen" is undefined. Do you know how
to
> > fix this?
> 
> I think you need create another idl file which just includes those new added
> API. Like experimental_media_galleries.idl and media_galleries.idl in
>
https://cs.corp.google.com/#chrome/src/chrome/common/extensions/api/&sq=packa...

Done. Thanks!

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:292: Socket* socket =
manager_->Get(extension_id(), params_->socket_id);
On 2012/09/12 19:49:49, Peng wrote:
> GetSocket()?

Done.

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:315: Socket* socket =
manager_->Get(extension_id(), params_->socket_id);
On 2012/09/12 19:49:49, Peng wrote:
> ditto

Done.

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

http://codereview.chromium.org/10827390/diff/12002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:76:
DCHECK(server_socket_.get() == NULL);
On 2012/09/12 19:49:49, Peng wrote:
> Done?

Done. Clicked this first one prematurely.

[Peng, 2012-09-13 16:51:04]

almost look good to me. except one problem. I did not find you check permission
in listen() function.

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

http://codereview.chromium.org/10827390/diff/6014/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:266: if
(!GetExtension()->CheckAPIPermissionWithParam(APIPermission::kSocket,
I think you need check permission in Listen() function too.

[justinlin, 2012-09-14 05:26:52]

PTAL, Thanks!

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

http://codereview.chromium.org/10827390/diff/6014/chrome/browser/extensions/a...
chrome/browser/extensions/api/socket/socket_api.cc:266: if
(!GetExtension()->CheckAPIPermissionWithParam(APIPermission::kSocket,
On 2012/09/13 16:51:04, Peng wrote:
> I think you need check permission in Listen() function too.

Done.

[Peng, 2012-09-14 16:27:16]

http://codereview.chromium.org/10827390/diff/16013/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/16013/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:295: if (socket &&
socket->GetBindAddress(&bindAddress)) {
The permissions for bind and listen are udp-bind:ip:port and tcp-listen:ip:port.
But your implementation need call both bind(ip, port) and listen() for TCP
server socket. I think it may confuse users.  So I think we need change
interface or permissions.

[justinlin, 2012-09-17 05:17:32]

On 2012/09/14 16:27:16, Peng wrote:
>
http://codereview.chromium.org/10827390/diff/16013/chrome/browser/extensions/...
> File chrome/browser/extensions/api/socket/socket_api.cc (right):
> 
>
http://codereview.chromium.org/10827390/diff/16013/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/socket_api.cc:295: if (socket &&
> socket->GetBindAddress(&bindAddress)) {
> The permissions for bind and listen are udp-bind:ip:port and
tcp-listen:ip:port.
> But your implementation need call both bind(ip, port) and listen() for TCP
> server socket. I think it may confuse users.  So I think we need change
> interface or permissions.

Went ahead and made this change you suggested. I think it makes sense. PTAL

[Peng, 2012-09-17 15:02:48]

lgtm with a suggestion.

http://codereview.chromium.org/10827390/diff/10012/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/10012/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:272: }
As you implemented bind() for TCP socket. TCPSocket::Bind() will not return
ERROR anymore. And user may use bind() and listen() together on TCP socket. It
may cause some confusion. I suggest - if it is TCP socket, return ERROR and with
console message like 'Does not support bind() on tcp socket. For TCP server
please use listen(ip, port)' in here for now, and find a better solution later.

[justinlin, 2012-09-20 23:11:22]

Sorry for the delay. Thanks, done. Yea, bind() was for the TCP client socket at
that point, but it probably makes sense to just remove it altogether to avoid
confusion.

Mike, would it be possible to get an owners review?

http://codereview.chromium.org/10827390/diff/10012/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/10012/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:272: }
On 2012/09/17 15:02:48, Peng wrote:
> As you implemented bind() for TCP socket. TCPSocket::Bind() will not return
> ERROR anymore. And user may use bind() and listen() together on TCP socket. It
> may cause some confusion. I suggest - if it is TCP socket, return ERROR and
with
> console message like 'Does not support bind() on tcp socket. For TCP server
> please use listen(ip, port)' in here for now, and find a better solution
later.

Done.

[miket_OOO, 2012-09-24 18:14:20]

Getting close. Please see the email referenced below & make changes.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:91: std::string* error_msg) {
Can you put NOTREACHED in here? I think we want to die loudly if this happens.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:96:
callback.Run(net::ERR_NOT_IMPLEMENTED, NULL);
Same.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:198: *error_msg =
kSocketListenError;
No braces.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:216: &TCPSocket::OnAccept,
base::Unretained(this)));
This indentation is nonstandard.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:219: return;
Are the returns necessary?

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:253: if (socket_mode_ !=
CLIENT)
If this can actually happen (i.e., an application programming error can cause
it), then please set the error message.

http://codereview.chromium.org/10827390/diff/22001/chrome/common/extensions/a...
File chrome/common/extensions/api/api.gyp (right):

http://codereview.chromium.org/10827390/diff/22001/chrome/common/extensions/a...
chrome/common/extensions/api/api.gyp:54: 'experimental_socket.idl',
Remove (see other comments).

http://codereview.chromium.org/10827390/diff/22001/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_socket.idl (right):

http://codereview.chromium.org/10827390/diff/22001/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_socket.idl:5: namespace
experimental.socket {
Please take a look at "[extensions-team] Mixing stable and experimental
extensions APIs." The way we'd like to go about this is to put the new API in
with the regular non-experimental socket, but check in code that the
experimental permission has been declared before allowing access to it. This is
a bit more manual work, and there is talk about making it more data-driven.

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

http://codereview.chromium.org/10827390/diff/22001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/socket/api/background.js:239: if (protocol
== 'tcp_server')
It'd make more sense to change "protocol" to something like "test_type" or
"test_name," now that it's no longer switching on protocol.

[justinlin, 2012-09-26 08:59:59]

Address all the comments except for 1 (see comment). Please take another look,
thanks!

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:91: std::string* error_msg) {
On 2012/09/24 18:14:21, miket wrote:
> Can you put NOTREACHED in here? I think we want to die loudly if this happens.

Done. This will happen if you try to call listen/accept on a UDP socket though.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:96:
callback.Run(net::ERR_NOT_IMPLEMENTED, NULL);
On 2012/09/24 18:14:21, miket wrote:
> Same.

Done.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:198: *error_msg =
kSocketListenError;
On 2012/09/24 18:14:21, miket wrote:
> No braces.

Done.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:216: &TCPSocket::OnAccept,
base::Unretained(this)));
On 2012/09/24 18:14:21, miket wrote:
> This indentation is nonstandard.

Done.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:219: return;
On 2012/09/24 18:14:21, miket wrote:
> Are the returns necessary?

Done.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:253: if (socket_mode_ !=
CLIENT)
On 2012/09/24 18:14:21, miket wrote:
> If this can actually happen (i.e., an application programming error can cause
> it), then please set the error message.

This is actually tricky. I think to do this, we would need to know at the
socket_api.cc level whether this is a Client or Server socket, and I had
prefered to keep this information local to tcp_socket. I already documented in
the IDL file that read,write and connect will always fail after calling listen()
on a socket. Do you think the error message is necessary?

http://codereview.chromium.org/10827390/diff/22001/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_socket.idl (right):

http://codereview.chromium.org/10827390/diff/22001/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_socket.idl:5: namespace
experimental.socket {
On 2012/09/24 18:14:21, miket wrote:
> Please take a look at "[extensions-team] Mixing stable and experimental
> extensions APIs." The way we'd like to go about this is to put the new API in
> with the regular non-experimental socket, but check in code that the
> experimental permission has been declared before allowing access to it. This
is
> a bit more manual work, and there is talk about making it more data-driven.

Done.

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

http://codereview.chromium.org/10827390/diff/22001/chrome/test/data/extension...
chrome/test/data/extensions/api_test/socket/api/background.js:239: if (protocol
== 'tcp_server')
On 2012/09/24 18:14:21, miket wrote:
> It'd make more sense to change "protocol" to something like "test_type" or
> "test_name," now that it's no longer switching on protocol.

Done.

[miket_OOO, 2012-09-26 17:17:55]

We're almost there, thanks. I'd feel more comfortable if I saw a test showing we
fail to accept/listen without the experimental permission. It should be a simple
matter of making a second copy of a test manifest without the permission, then
writing one browser test that loads that manifest but otherwise runs exactly the
same code.

The reason I think this is worth doing is that the
experimental-part-of-non-experimental API aspect requires us to write custom
permission code (as you've done), so we can't really say we have test coverage
of that code already, as we do for the standard permission code.

Thanks. After this we should be able to get it landed.

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:91: std::string* error_msg) {
On 2012/09/26 08:59:59, justinlin wrote:
> On 2012/09/24 18:14:21, miket wrote:
> > Can you put NOTREACHED in here? I think we want to die loudly if this
happens.
> 
> Done. This will happen if you try to call listen/accept on a UDP socket
though.

Oh, OK, then my request was in error. NOTREACHED is like a runtime assertion --
if it gets here, our code is bad. If a user can legitimately (if erroneously)
get here, then it should return an error rather than killing the process.

I think, though, that ERR_NOT_IMPLEMENTED might not be exactly the right code to
return. It's more like an invalid state or an invalid request, rather than
something we haven't gotten around to implementing yet. (If you think
NOT_IMPLEMENTED represents the right sentiment, then it's OK to leave as-is.)

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:93: return
net::ERR_NOT_IMPLEMENTED;
See comments in earlier patch. I retract my request for NOTREACHED, but would
like to make sure you mean NOT_IMPLEMENTED rather than something like "invalid
request."

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:98:
callback.Run(net::ERR_NOT_IMPLEMENTED, NULL);
Same as above.

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:304:
SetResult(Value::CreateIntegerValue(result));
Excellent. Could this happen in Prepare()? Not a big deal, but it saves the
trouble of an IPC, and we generally try to perform as much validation as
possible in Prepare.

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:345: error_ =
kExperimentalPermissionError;
Same.

[justinlin, 2012-09-26 20:53:11]

Done, added the test. Also found a bug with accept() in the process. PTAL,
thanks!

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10827390/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:91: std::string* error_msg) {
On 2012/09/26 17:17:55, miket wrote:
> On 2012/09/26 08:59:59, justinlin wrote:
> > On 2012/09/24 18:14:21, miket wrote:
> > > Can you put NOTREACHED in here? I think we want to die loudly if this
> happens.
> > 
> > Done. This will happen if you try to call listen/accept on a UDP socket
> though.
> 
> Oh, OK, then my request was in error. NOTREACHED is like a runtime assertion
--
> if it gets here, our code is bad. If a user can legitimately (if erroneously)
> get here, then it should return an error rather than killing the process.
> 
> I think, though, that ERR_NOT_IMPLEMENTED might not be exactly the right code
to
> return. It's more like an invalid state or an invalid request, rather than
> something we haven't gotten around to implementing yet. (If you think
> NOT_IMPLEMENTED represents the right sentiment, then it's OK to leave as-is.)

Done. Yea, I agree NOT_IMPLEMENTED might not be the right error here. I changed
it to net::FAILED to just match what we do for TCP bind (which is not supported
either).

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:93: return
net::ERR_NOT_IMPLEMENTED;
On 2012/09/26 17:17:55, miket wrote:
> See comments in earlier patch. I retract my request for NOTREACHED, but would
> like to make sure you mean NOT_IMPLEMENTED rather than something like "invalid
> request."

Done.

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:98:
callback.Run(net::ERR_NOT_IMPLEMENTED, NULL);
On 2012/09/26 17:17:55, miket wrote:
> Same as above.

Done.

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:304:
SetResult(Value::CreateIntegerValue(result));
On 2012/09/26 17:17:55, miket wrote:
> Excellent. Could this happen in Prepare()? Not a big deal, but it saves the
> trouble of an IPC, and we generally try to perform as much validation as
> possible in Prepare.

Done.

http://codereview.chromium.org/10827390/diff/32001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:345: error_ =
kExperimentalPermissionError;
On 2012/09/26 17:17:55, miket wrote:
> Same.

Done.

[miket_OOO, 2012-09-26 21:02:44]

LGTM, thanks. See a few comments below.

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:92: return net::ERR_FAILED;
Good stuff, thanks. This looks much better.

Next time you're back in here, you could probably set error_msg (sorry & never
mind if you already told me why this was hard).

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:338: error_ =
kExperimentalPermissionError;
Nice.

http://codereview.chromium.org/10827390/diff/34002/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/socket/experimental/background.js
(right):

http://codereview.chromium.org/10827390/diff/34002/chrome/test/data/extension...
chrome/test/data/extensions/api_test/socket/experimental/background.js:8: var
onAccept = function(acceptInfo) {
Perfect. For the sanity of future engineers in this file, how about a comment
right here explaining that this should be undefined because the manifest is
failing to declare experimental? It's obvious to you and me, but I'll surely
have forgotten in two weeks.

[Mihai Parparita -not on Chrome, 2012-09-26 21:50:33]

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:41: "Caller does not have
permission for experimental api";
Nit: Capitalize API.

Also, for both this and kPermissionError, "Caller" is overly technical and isn't
used in other error messages. Can it say "App does not have permission"?

http://codereview.chromium.org/10827390/diff/34002/chrome/common/extensions/a...
File chrome/common/extensions/api/socket.idl (right):

http://codereview.chromium.org/10827390/diff/34002/chrome/common/extensions/a...
chrome/common/extensions/api/socket.idl:200: // This method applies to TCP
sockets only.
The documentation strings for these two methods should mention the experimental
permission requirement.

[justinlin, 2012-09-26 23:03:57]

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket.cc (right):

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket.cc:92: return net::ERR_FAILED;
On 2012/09/26 21:02:44, miket wrote:
> Good stuff, thanks. This looks much better.
> 
> Next time you're back in here, you could probably set error_msg (sorry & never
> mind if you already told me why this was hard).

Done. Added the error message. I think write() was the one that was tricky to
add an error message to.

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

http://codereview.chromium.org/10827390/diff/34002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/socket_api.cc:41: "Caller does not have
permission for experimental api";
On 2012/09/26 21:50:33, Mihai Parparita wrote:
> Nit: Capitalize API.
> 
> Also, for both this and kPermissionError, "Caller" is overly technical and
isn't
> used in other error messages. Can it say "App does not have permission"?

Done.

http://codereview.chromium.org/10827390/diff/34002/chrome/common/extensions/a...
File chrome/common/extensions/api/socket.idl (right):

http://codereview.chromium.org/10827390/diff/34002/chrome/common/extensions/a...
chrome/common/extensions/api/socket.idl:200: // This method applies to TCP
sockets only.
On 2012/09/26 21:50:33, Mihai Parparita wrote:
> The documentation strings for these two methods should mention the
experimental
> permission requirement.

Done.

http://codereview.chromium.org/10827390/diff/34002/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/socket/experimental/background.js
(right):

http://codereview.chromium.org/10827390/diff/34002/chrome/test/data/extension...
chrome/test/data/extensions/api_test/socket/experimental/background.js:8: var
onAccept = function(acceptInfo) {
On 2012/09/26 21:02:44, miket wrote:
> Perfect. For the sanity of future engineers in this file, how about a comment
> right here explaining that this should be undefined because the manifest is
> failing to declare experimental? It's obvious to you and me, but I'll surely
> have forgotten in two weeks.

Done. :)

[commit-bot: I haz the power, 2012-09-28 00:16:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/10827390/40002

[commit-bot: I haz the power, 2012-09-28 01:34:13]

Change committed as 159172

[Mihai Parparita -not on Chrome, 2012-09-28 14:41:03]

Once this change makes it to a canary build, feel free to send out an email
to chrome-apps-announce@ (internal) and
chromium-apps@chromium.org(external) encouraging people to try it out.

Also, building a sample app that uses it (for
http://github.com/GoogleChrome/chrome-app-samples) would be great too.

Mihai

On Thu, Sep 27, 2012 at 6:34 PM, <commit-bot@chromium.org> wrote:

> Change committed as 159172
>
>
https://chromiumcodereview.**appspot.com/10827390/<https://chromiumcodereview...
>

[justinlin, 2012-10-01 21:47:16]

On 2012/09/28 14:41:03, Mihai Parparita wrote:
> Once this change makes it to a canary build, feel free to send out an email
> to chrome-apps-announce@ (internal) and
> chromium-apps@chromium.org(external) encouraging people to try it out.
> 
> Also, building a sample app that uses it (for
> http://github.com/GoogleChrome/chrome-app-samples) would be great too.
> 
> Mihai
> 
> On Thu, Sep 27, 2012 at 6:34 PM, <mailto:commit-bot@chromium.org> wrote:
> 
> > Change committed as 159172
> >
> >
>
https://chromiumcodereview.**appspot.com/10827390/%3Chttps://chromiumcoderevi...>
> >

Will do, I'll try to cleanup the small app I wrote for testing and submit it to
github.