Allow missing socketType in SocketInfo if socket is not connected

Allow missing socketType in SocketInfo if socket is not connected

BUG=242261
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203036

[Bei Zhang, 2013-05-24 18:05:39]

Hi Antony,

Please Review.

Thanks!
Bei

[asargent_no_longer_on_chrome, 2013-05-29 16:58:14]

The chrome.socket.create method requires a SocketType parameter, so it isn't
possible to create a socket without a type. 

This is just to fix the problem of what to return if an invalid socket id is
passed to chrome.socket.getInfo, right? Why not just change the signature of the
callback to return null if a socket wasn't found for the given id?

I.e. change

  callback GetInfoCallback = void (SocketInfo result);

to

  callback GetInfoCallback = void (optional SocketInfo result);

[Bei Zhang, 2013-05-29 17:04:53]

I believe SocketInfo is not related to chrome.socket.create method?

This change hurts the compatibility least because direct use of result.connected
will not throw an error.

On 2013/05/29 16:58:14, Antony Sargent wrote:
> The chrome.socket.create method requires a SocketType parameter, so it isn't
> possible to create a socket without a type. 
> 
> This is just to fix the problem of what to return if an invalid socket id is
> passed to chrome.socket.getInfo, right? Why not just change the signature of
the
> callback to return null if a socket wasn't found for the given id?
> 
> I.e. change
> 
>   callback GetInfoCallback = void (SocketInfo result);
> 
> to
> 
>   callback GetInfoCallback = void (optional SocketInfo result);

[asargent_no_longer_on_chrome, 2013-05-29 18:00:09]

Actually I think the approach that is most consistent with the rest of the
platform's APIs would be to leave it as-is. 

For example, in an extension with the tabs permission, try this:

chrome.tabs.get(99999, function(tab) {
  if (chrome.runtime.lastError) {
    console.log("error: " + chrome.runtime.lastError.message);
  } else {
    console.dir(tab);
  }
});

Note that we also throw an exception during the callback dispatch. 

While I sympathize with your point that in this particular instance it might be
more convenient for developers to be able to write:

chrome.socket.getInfo(id, function(info) {
  if (!info.connected) {
    // handle disconnection
  }
});

that road eventually leads both to inconsistency in the APIs, and to making all
enum fields in dictionaries optional even when it's not really a consistent
state for those data members to be null.

[Bei Zhang, 2013-05-29 21:25:00]

This make much more sense than my change!
Fixed according to your comment.

Thanks for review!

[asargent_no_longer_on_chrome, 2013-05-29 21:31:07]

https://codereview.chromium.org/15359007/diff/10001/chrome/common/extensions/...
File chrome/common/extensions/api/socket.idl (right):

https://codereview.chromium.org/15359007/diff/10001/chrome/common/extensions/...
chrome/common/extensions/api/socket.idl:113: callback GetInfoCallback = void
(optional SocketInfo result);
You can actually leave this as non-optional, since for callbacks we guarantee
the arguments will be sent to the callback as specified *unless* there is an
error. So for example in the chrome.tabs.get method I mentioned before, the
"Tab" argument to the callback is not optional but is omitted when an error
occurs. 

We could probably do a better job of documenting our error handling strategy,
because this is how most of our APIs work.

[Bei Zhang, 2013-05-29 22:14:51]

https://codereview.chromium.org/15359007/diff/10001/chrome/common/extensions/...
File chrome/common/extensions/api/socket.idl (right):

https://codereview.chromium.org/15359007/diff/10001/chrome/common/extensions/...
chrome/common/extensions/api/socket.idl:113: callback GetInfoCallback = void
(optional SocketInfo result);
On 2013/05/29 21:31:07, Antony Sargent wrote:
> You can actually leave this as non-optional, since for callbacks we guarantee
> the arguments will be sent to the callback as specified *unless* there is an
> error. So for example in the chrome.tabs.get method I mentioned before, the
> "Tab" argument to the callback is not optional but is omitted when an error
> occurs. 
> 
> We could probably do a better job of documenting our error handling strategy,
> because this is how most of our APIs work.

Done.

[asargent_no_longer_on_chrome, 2013-05-29 22:34:02]

lgtm

[commit-bot: I haz the power, 2013-05-29 22:35:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ikarienator@chromium.org/15359007/15001

[commit-bot: I haz the power, 2013-05-30 03:24:08]

Change committed as 203036