Adds socket.getInfo to the socket API

Adds socket.getInfo to the socket API

Allows clients to retrieve the state of a specified socket.

BUG=135464
TEST=


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

[thorogood, 2012-07-24 07:58:53]

Ben has volunteered to take a quick look.

[benwells, 2012-07-24 08:05:33]

I've only had a very quick look...

https://chromiumcodereview.appspot.com/10790137/diff/2001/chrome/common/exten...
File chrome/common/extensions/api/experimental_socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/2001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:64: // in the form
<code>ip:port</code>, e.g., <code>10.0.0.1:1234</code>.
Does it make more sense to have the port available as a long here to avoid
callers having to parse the string?

https://chromiumcodereview.appspot.com/10790137/diff/2001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:190: GetInfoCallback
callback);
nit: indentation

[thorogood, 2012-07-26 01:49:59]

PTAL

https://chromiumcodereview.appspot.com/10790137/diff/2001/chrome/common/exten...
File chrome/common/extensions/api/experimental_socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/2001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:64: // in the form
<code>ip:port</code>, e.g., <code>10.0.0.1:1234</code>.
On 2012/07/24 08:05:33, benwells wrote:
> Does it make more sense to have the port available as a long here to avoid
> callers having to parse the string?

Yes, probably. I've broken it out.

https://chromiumcodereview.appspot.com/10790137/diff/2001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:190: GetInfoCallback
callback);
On 2012/07/24 08:05:33, benwells wrote:
> nit: indentation

Done.

[benwells, 2012-07-26 04:18:37]

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket_api.cc:473:
result->Set(kSocketTypeKey, Value::CreateStringValue(type));
I think there is a nicer way to set this return value.

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/common/exten...
File chrome/common/extensions/api/experimental_socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:60: // The type of the
passed socket. This will be code <code>tcp</code> or
In "This will be code <code>tcp..." is the first code intentional?

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:70: // Optionally contains
the port of the connected peer.
As this is a return value optional is unclear. You should say when it is present
/ when not.

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:76: // Optionally contains
the local port.
Same re optional

[thorogood, 2012-07-26 06:19:50]

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/browser/exte...
chrome/browser/extensions/api/socket/socket_api.cc:473:
result->Set(kSocketTypeKey, Value::CreateStringValue(type));
On 2012/07/26 04:18:37, benwells wrote:
> I think there is a nicer way to set this return value.

I've had a go at using the generated type. How I reset the scoped_ptr feels
verbose though - I'm not sure if there's a neater way. Makes sense though:
dynamically create copy (in heap) of variable returned to me normally on the
stack, give ownership to scoped_ptr.

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/common/exten...
File chrome/common/extensions/api/experimental_socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:60: // The type of the
passed socket. This will be code <code>tcp</code> or
On 2012/07/26 04:18:37, benwells wrote:
> In "This will be code <code>tcp..." is the first code intentional?

Typo, thanks.

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:70: // Optionally contains
the port of the connected peer.
On 2012/07/26 04:18:37, benwells wrote:
> As this is a return value optional is unclear. You should say when it is
present
> / when not.

Done.

https://chromiumcodereview.appspot.com/10790137/diff/7001/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:76: // Optionally contains
the local port.
On 2012/07/26 04:18:37, benwells wrote:
> Same re optional

Done.

[thorogood, 2012-07-27 03:46:50]

https://chromiumcodereview.appspot.com/10790137/diff/13001/chrome/common/exte...
File chrome/common/extensions/api/experimental_socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/13001/chrome/common/exte...
chrome/common/extensions/api/experimental_socket.idl:64: // Whether this socket
is currently connected.
Ben, I've updated the comment here. I still think this value holds some utility,
because devs are still going to call connect()/disconnect() as normal and they
may want to know what state the socket thinks it is in.

However I suspect it's still contrived. Thoughts?

[benwells, 2012-07-30 00:47:58]

lgtm

https://chromiumcodereview.appspot.com/10790137/diff/15002/chrome/browser/ext...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://chromiumcodereview.appspot.com/10790137/diff/15002/chrome/browser/ext...
chrome/browser/extensions/api/socket/socket_api.cc:451: : params_(NULL) {
Nit: can use {} on the one line.

[thorogood, 2012-07-30 07:22:18]

ping miket@.

I've also updated the comments slightly _again_ because this morning I finally
got around to writing some C sockets code just to verify some behaviour. Turns
out even if the remote end disconnects you, the get...addr methods still work
until you locally shut down the socket.

https://chromiumcodereview.appspot.com/10790137/diff/15002/chrome/browser/ext...
File chrome/browser/extensions/api/socket/socket_api.cc (right):

https://chromiumcodereview.appspot.com/10790137/diff/15002/chrome/browser/ext...
chrome/browser/extensions/api/socket/socket_api.cc:451: : params_(NULL) {
On 2012/07/30 00:47:58, benwells wrote:
> Nit: can use {} on the one line.

Done, I've left the ": params_(NULL) {}" on the next line though (even though I
could fit it on the previous). Woo, nits.

[miket_OOO, 2012-07-30 16:53:10]

I've added a few comments about the documentation. You might want to consider
adding penghuang, who is in the midst of changes to these files, too.

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/common/exten...
File chrome/common/extensions/api/experimental_socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:69: //
<code>disconnect()</code>.
We should say something here about what "connected" means in the UDP case.
(Unfortunately, the documentation for our connect API itself is a little too
concise to be helpful.) It doesn't actually connect or cause anything to go out
over the wire. It just associates a destination address with the socket. So a
"connected" UDP socket never gets disconnected, either.

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:81: /// IPv4/6 address.
Remove extra slash.

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:197: // Retrieve the state
of the given socket.
"Retrieves" to match the voice of earlier docs. (Would you please update
setNoDelay and setKeepAlive, which are using the wrong voice, too?)

[thorogood, 2012-07-31 01:21:09]

+penghuang

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/common/exten...
File chrome/common/extensions/api/experimental_socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:69: //
<code>disconnect()</code>.
On 2012/07/30 16:53:10, miket wrote:
> We should say something here about what "connected" means in the UDP case.
> (Unfortunately, the documentation for our connect API itself is a little too
> concise to be helpful.) It doesn't actually connect or cause anything to go
out
> over the wire. It just associates a destination address with the socket. So a
> "connected" UDP socket never gets disconnected, either.

I've added a section for UDP, as well as updating the documentation for
connect().

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:81: /// IPv4/6 address.
On 2012/07/30 16:53:10, miket wrote:
> Remove extra slash.

Done.

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/common/exten...
chrome/common/extensions/api/experimental_socket.idl:197: // Retrieve the state
of the given socket.
On 2012/07/30 16:53:10, miket wrote:
> "Retrieves" to match the voice of earlier docs. (Would you please update
> setNoDelay and setKeepAlive, which are using the wrong voice, too?)

Done. I've also updated the setNoDelay comment just a touch, since I feel it was
difficult to grok.

[Peng, 2012-07-31 16:15:37]

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/browser/exte...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/browser/exte...
chrome/browser/extensions/api/socket/tcp_socket.cc:148: return
!socket_->GetPeerAddress(address);
I am not sure if socket is always non-Null. Could please add a browser test
which calls getInfo() before connect() is completed?

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/browser/exte...
chrome/browser/extensions/api/socket/tcp_socket.cc:152: return
!socket_->GetLocalAddress(address);
ditto

[thorogood, 2012-08-03 03:24:12]

Thanks for noticing the NPE. Fix added, API test modified to check default state
of socket pre-connection.

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/browser/exte...
File chrome/browser/extensions/api/socket/tcp_socket.cc (right):

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/browser/exte...
chrome/browser/extensions/api/socket/tcp_socket.cc:148: return
!socket_->GetPeerAddress(address);
On 2012/07/31 16:15:37, Peng wrote:
> I am not sure if socket is always non-Null. Could please add a browser test
> which calls getInfo() before connect() is completed?

Done.

https://chromiumcodereview.appspot.com/10790137/diff/9007/chrome/browser/exte...
chrome/browser/extensions/api/socket/tcp_socket.cc:152: return
!socket_->GetLocalAddress(address);
On 2012/07/31 16:15:37, Peng wrote:
> ditto

Done.

[Peng, 2012-08-03 14:13:36]

lgtm with one comment.

https://chromiumcodereview.appspot.com/10790137/diff/20013/chrome/common/exte...
File chrome/common/extensions/api/socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/20013/chrome/common/exte...
chrome/common/extensions/api/socket.idl:204: static void getInfo(long socketId,
I am not sure if new added function should be experimental first. Please confirm
it with mike.

[thorogood, 2012-08-05 02:22:57]

Ping miket@ again. :)

https://chromiumcodereview.appspot.com/10790137/diff/20013/chrome/common/exte...
File chrome/common/extensions/api/socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/20013/chrome/common/exte...
chrome/common/extensions/api/socket.idl:204: static void getInfo(long socketId,
On 2012/08/03 14:13:36, Peng wrote:
> I am not sure if new added function should be experimental first. Please
confirm
> it with mike.

Since the whole API is now out of experimental, I'm fairly sure it is safe to
add this. I'll wait for another LGTM from Mike, or from someone else locally in
Sydney.

[miket_OOO, 2012-08-06 21:47:53]

lgtm

https://chromiumcodereview.appspot.com/10790137/diff/20013/chrome/common/exte...
File chrome/common/extensions/api/socket.idl (right):

https://chromiumcodereview.appspot.com/10790137/diff/20013/chrome/common/exte...
chrome/common/extensions/api/socket.idl:204: static void getInfo(long socketId,
On 2012/08/05 02:22:57, thorogood wrote:
> On 2012/08/03 14:13:36, Peng wrote:
> > I am not sure if new added function should be experimental first. Please
> confirm
> > it with mike.
> 
> Since the whole API is now out of experimental, I'm fairly sure it is safe to
> add this. I'll wait for another LGTM from Mike, or from someone else locally
in
> Sydney.

Yes, that's fine; it's additive and backward-compatible. It's of course less
mature than the other APIs in this family, but I don't think it's worth
inventing a new doc-only designation of "not experimental but not guaranteed
stable yet, either." My answer might change if a many apps were already using
the socket API.

[commit-bot: I haz the power, 2012-08-06 23:15:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thorogood@chromium.org/10790137/20013

[commit-bot: I haz the power, 2012-08-07 00:30:01]

Change committed as 150190