Refactor the socket API to remove onEvent callback in socket.create() function.

Refactor the socket API to remove onEvent callback in socket.create() function.


BUG=None
TEST=browser_tests --gtest_filter=SocketApiTest.*
TEST=unit_tests --gtest_filter=SocketTest.*

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

[Peng, 2012-05-02 18:37:36]

Hi Mike and Jeremy,
Could you please review this CL? Thank you.

[miket_OOO, 2012-05-03 17:08:13]

LGTM. A few nits and one interesting question about ERR_IO_PENDING.

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

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:47:
callback.Run(net::ERR_IO_PENDING);
Is there something like an "another IO operation already in progress" result
code that would be more appropriate here? If I understand the context correctly,
then reaching this point indicates a likely programming error, but
ERR_IO_PENDING is a result code that can also indicate a legitimate outcome from
this call. A more specific error code would give the developer a clue what's
actually wrong.

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:55: result =
net::ERR_CONNECTION_FAILED;
Might as well take advantage of the initialization of result and leave this out,
or else copy the idiom you used below (init to a general "ERR_FAILED" code).

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:91:
callback.Run(net::ERR_IO_PENDING, NULL);
Same comment as earlier.

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/udp_socket.cc (right):

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/udp_socket.cc:46: } while (0);
I think it reads better to say while (false), at least it does in C++.

http://codereview.chromium.org/10273016/diff/25002/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental.socket.idl (right):

http://codereview.chromium.org/10273016/diff/25002/chrome/common/extensions/a...
chrome/common/extensions/api/experimental.socket.idl:12: [nodoc] long?
dummyValue;
This might have been fixed. Can you check?

[Peng, 2012-05-03 19:21:50]

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

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:47:
callback.Run(net::ERR_IO_PENDING);
On 2012/05/03 17:08:13, miket wrote:
> Is there something like an "another IO operation already in progress" result
> code that would be more appropriate here? If I understand the context
correctly,
> then reaching this point indicates a likely programming error, but
> ERR_IO_PENDING is a result code that can also indicate a legitimate outcome
from
> this call. A more specific error code would give the developer a clue what's
> actually wrong.

Linux returns -EALREADY for sock is connecting and not finished. 
-ISCONN for socket is connected.
I can not find similar errors in chrome. I have to use ERR_CONNECTION_FAILED.

Done

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:55: result =
net::ERR_CONNECTION_FAILED;
On 2012/05/03 17:08:13, miket wrote:
> Might as well take advantage of the initialization of result and leave this
out,
> or else copy the idiom you used below (init to a general "ERR_FAILED" code).

Done.

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:91:
callback.Run(net::ERR_IO_PENDING, NULL);
On 2012/05/03 17:08:13, miket wrote:
> Same comment as earlier.

I can not find any better errors than IO_PENDING in net/base/net_error_list.h.
Do you know any better error? Or add a new error for it. Any good error name? 
FYI, Linux uses EAGAIN or EWOULDBLOCK for - The  file  descriptor  fd refers to
a socket and has been marked nonblocking (O_NONBLOCK), and the read would block.

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
File chrome/browser/extensions/api/socket/udp_socket.cc (right):

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/udp_socket.cc:46: } while (0);
On 2012/05/03 17:08:13, miket wrote:
> I think it reads better to say while (false), at least it does in C++.

Done. Also fixed other do {} while (0);

http://codereview.chromium.org/10273016/diff/25002/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental.socket.idl (right):

http://codereview.chromium.org/10273016/diff/25002/chrome/common/extensions/a...
chrome/common/extensions/api/experimental.socket.idl:12: [nodoc] long?
dummyValue;
On 2012/05/03 17:08:13, miket wrote:
> This might have been fixed. Can you check?

I removed onEvent in CreateOptions. So it become empty. I tried remove
dummyValue, it doesn't work

With CL, CreateOptions becomes useless. But maybe we will need it to pass some
options. Do you think it is better to remove it or keep it?

[miket_OOO, 2012-05-03 20:03:31]

> I can not find any better errors than IO_PENDING in net/base/net_error_list.h.
> Do you know any better error? Or add a new error for it. Any good error name? 
> FYI, Linux uses EAGAIN or EWOULDBLOCK for - The  file  descriptor  fd refers
to
> a socket and has been marked nonblocking (O_NONBLOCK), and the read would
block.

On the error name, I'd ask one of the owners of net/, who will have to review
that change anyway.

> With CL, CreateOptions becomes useless. But maybe we will need it to pass some
> options. Do you think it is better to remove it or keep it?

Keep.

[Peng, 2012-05-03 20:34:26]

Hi agl, Could you please review changes in net/ folder? Thanks.

On 2012/05/03 20:03:31, miket wrote:
> > I can not find any better errors than IO_PENDING in
net/base/net_error_list.h.
> > Do you know any better error? Or add a new error for it. Any good error
name? 
> > FYI, Linux uses EAGAIN or EWOULDBLOCK for - The  file  descriptor  fd refers
> to
> > a socket and has been marked nonblocking (O_NONBLOCK), and the read would
> block.
> 
> On the error name, I'd ask one of the owners of net/, who will have to review
> that change anyway.

Added a new error net::ERR_TRY_AGAIN

> 
> > With CL, CreateOptions becomes useless. But maybe we will need it to pass
some
> > options. Do you think it is better to remove it or keep it?
> 
> Keep.

[jeremya, 2012-05-04 00:37:59]

Why did we decide not to go the 'data' event route?

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

http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:91:
callback.Run(net::ERR_IO_PENDING, NULL);
On 2012/05/03 19:21:50, Peng wrote:
> On 2012/05/03 17:08:13, miket wrote:
> > Same comment as earlier.
> 
> I can not find any better errors than IO_PENDING in net/base/net_error_list.h.
> Do you know any better error? Or add a new error for it. Any good error name? 
> FYI, Linux uses EAGAIN or EWOULDBLOCK for - The  file  descriptor  fd refers
to
> a socket and has been marked nonblocking (O_NONBLOCK), and the read would
block.

I don't think this system can ever return ERR_IO_PENDING legitimately to the
application any more. Still, this is repurposing the error code to mean
something other than what it does everywhere else.

[agl, 2012-05-04 16:52:17]

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

http://codereview.chromium.org/10273016/diff/36001/chrome/browser/extensions/...
chrome/browser/extensions/api/socket/tcp_socket.cc:89:
callback.Run(net::ERR_TRY_AGAIN, NULL);
In net/ there is a convention that functions that take a callback either perform
their action without blocking (and return a status), or return ERR_IO_PENDING.
ERR_IO_PENDING means that the callback will be made at a later time.

Here, it appears that the callback is always called. This means that the
callback will hairpin, which is typically error-prone and avoiding hairpins is
the reason for the ERR_IO_PENDING pattern in net/.

But since this code is outside of net/, and written in a different discipline, I
don't feel that adding a new error code is appropriate. In net/ we would DCHECK
this condition. If that's not appropriate here then I think ERR_IO_PENDING is
probably the best, small change.

However, I would urge you to consider following the net/ calling discipline if
possible. It will likely save you headaches in the future.

[Peng, 2012-05-07 15:27:42]

On 2012/05/04 16:52:17, agl wrote:
>
http://codereview.chromium.org/10273016/diff/36001/chrome/browser/extensions/...
> File chrome/browser/extensions/api/socket/tcp_socket.cc (right):
> 
>
http://codereview.chromium.org/10273016/diff/36001/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/tcp_socket.cc:89:
> callback.Run(net::ERR_TRY_AGAIN, NULL);
> In net/ there is a convention that functions that take a callback either
perform
> their action without blocking (and return a status), or return ERR_IO_PENDING.
> ERR_IO_PENDING means that the callback will be made at a later time.
> 
> Here, it appears that the callback is always called. This means that the
> callback will hairpin, which is typically error-prone and avoiding hairpins is
> the reason for the ERR_IO_PENDING pattern in net/.
> 
> But since this code is outside of net/, and written in a different discipline,
I
> don't feel that adding a new error code is appropriate. In net/ we would
DCHECK
> this condition. If that's not appropriate here then I think ERR_IO_PENDING is
> probably the best, small change.

changed it back to ERR_IO_PENDING.

> 
> However, I would urge you to consider following the net/ calling discipline if
> possible. It will likely save you headaches in the future.

This classes are glue layer between chrome socket classes and chrome extension
JS API.
In JS side, we can only use callback to deliver IO results (including errors).
So I think
current CL implementation can may code a little simpler. We don't need handle
errors in
two places (after IO call, and callback function).

[Peng, 2012-05-07 15:28:41]

On 2012/05/04 00:37:59, jeremya wrote:
> Why did we decide not to go the 'data' event route?

Discussed it in mail.

> 
>
http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
> File chrome/browser/extensions/api/socket/tcp_socket.cc (right):
> 
>
http://codereview.chromium.org/10273016/diff/25002/chrome/browser/extensions/...
> chrome/browser/extensions/api/socket/tcp_socket.cc:91:
> callback.Run(net::ERR_IO_PENDING, NULL);
> On 2012/05/03 19:21:50, Peng wrote:
> > On 2012/05/03 17:08:13, miket wrote:
> > > Same comment as earlier.
> > 
> > I can not find any better errors than IO_PENDING in
net/base/net_error_list.h.
> > Do you know any better error? Or add a new error for it. Any good error
name? 
> > FYI, Linux uses EAGAIN or EWOULDBLOCK for - The  file  descriptor  fd refers
> to
> > a socket and has been marked nonblocking (O_NONBLOCK), and the read would
> block.
> 
> I don't think this system can ever return ERR_IO_PENDING legitimately to the
> application any more. Still, this is repurposing the error code to mean
> something other than what it does everywhere else.

Changed it back.

[agl, 2012-05-07 15:30:54]

Thanks. As this CL no longer has any changes in net/, I don't believe that I
need to review it.

[commit-bot: I haz the power, 2012-05-07 16:00:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/penghuang@chromium.org/10273016/41001

[commit-bot: I haz the power, 2012-05-07 20:10:09]

Change committed as 135709