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

chrome/browser/extensions/api/socket/tcp_socket.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/socket/tcp_socket.h"
 
 #include "chrome/browser/extensions/api/api_resource.h"
 #include "chrome/browser/extensions/api/api_resource_event_notifier.h"
 #include "net/base/address_list.h"
 #include "net/base/ip_endpoint.h"
@@ skipping 20 matching lines @@
     APIResourceEventNotifier* event_notifier) {
   return new TCPSocket(tcp_client_socket, event_notifier);
 }
 
 TCPSocket::~TCPSocket() {
   if (is_connected_) {
     Disconnect();
   }
 }
 
-int TCPSocket::Connect(const std::string& address, int port) {
-  if (is_connected_)
-    return net::ERR_CONNECTION_FAILED;
+void TCPSocket::Connect(const std::string& address,
+                        int port,
+                        const CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
 
-  net::AddressList address_list;
-  if (!StringAndPortToAddressList(address, port, &address_list))
-    return net::ERR_INVALID_ARGUMENT;
+  if (!connect_callback_.is_null()) {
+    callback.Run(net::ERR_IO_PENDING);

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

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.

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

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

+    return;
+  }
+  connect_callback_ = callback;
 
-  socket_.reset(new net::TCPClientSocket(address_list, NULL,
-                                         net::NetLog::Source()));
+  int result = net::ERR_CONNECTION_FAILED;
+  do {
+    if (is_connected_) {
+      result = net::ERR_CONNECTION_FAILED;

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

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).

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

Done.

+      break;
+    }
 
-  int result = socket_->Connect(base::Bind(
-      &TCPSocket::OnConnect, base::Unretained(this)));
-  if (result == net::OK) {
-    is_connected_ = true;
-  }
-  return result;
+    net::AddressList address_list;
+    if (!StringAndPortToAddressList(address, port, &address_list)) {
+      result = net::ERR_INVALID_ARGUMENT;
+      break;
+    }
+
+    socket_.reset(new net::TCPClientSocket(address_list, NULL,
+                                           net::NetLog::Source()));
+    connect_callback_ = callback;
+    result = socket_->Connect(base::Bind(
+        &TCPSocket::OnConnectComplete, base::Unretained(this)));
+  } while (0);
+
+  if (result != net::ERR_IO_PENDING)
+    OnConnectComplete(result);
 }
 
 void TCPSocket::Disconnect() {
   is_connected_ = false;
   socket_->Disconnect();
 }
 
 int TCPSocket::Bind(const std::string& address, int port) {
   // TODO(penghuang): Supports bind for tcp?
   return net::ERR_FAILED;
 }
 
-int TCPSocket::Read(scoped_refptr<net::IOBuffer> io_buffer, int io_buffer_len) {
-  if (!socket_.get() || !socket_->IsConnected())
-    return net::ERR_SOCKET_NOT_CONNECTED;
+void TCPSocket::Read(int count,
+                     const ReadCompletionCallback& callback) {
+  DCHECK(!callback.is_null());
 
-  return socket_->Read(
-      io_buffer.get(),
-      io_buffer_len,
-      base::Bind(&Socket::OnDataRead, base::Unretained(this), io_buffer,
-          static_cast<net::IPEndPoint*>(NULL)));
+  if (!read_callback_.is_null()) {
+    callback.Run(net::ERR_IO_PENDING, NULL);

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

Same comment as earlier.

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

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.

[jeremya, 2012/05/04 00:38:00]

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.

+    return;
+  } else {
+    read_callback_ = callback;
+  }
+
+  int result = net::ERR_FAILED;
+  scoped_refptr<net::IOBuffer> io_buffer;
+  do {
+    if (count < 0) {
+      result = net::ERR_INVALID_ARGUMENT;
+      break;
+    }
+
+    if (!socket_.get() || !socket_->IsConnected()) {
+        result = net::ERR_SOCKET_NOT_CONNECTED;
+        break;
+    }
+
+    io_buffer = new net::IOBuffer(count);
+    result = socket_->Read(io_buffer.get(), count,
+        base::Bind(&TCPSocket::OnReadComplete, base::Unretained(this),
+            io_buffer));
+  } while (0);
+
+  if (result != net::ERR_IO_PENDING)
+    OnReadComplete(io_buffer, result);
 }
 
-int TCPSocket::Write(scoped_refptr<net::IOBuffer> io_buffer, int byte_count) {
-  if (!socket_.get() || !socket_->IsConnected())
-    return net::ERR_SOCKET_NOT_CONNECTED;
+void TCPSocket::Write(scoped_refptr<net::IOBuffer> io_buffer,
+                      int byte_count,
+                      const CompletionCallback& callback) {
+  DCHECK(!callback.is_null());
 
-  return socket_->Write(
-      io_buffer.get(), byte_count,
-      base::Bind(&Socket::OnWriteComplete, base::Unretained(this)));
+  if (!write_callback_.is_null()) {
+    // TODO(penghuang): Put requests in a pending queue to support multiple
+    // write calls.
+    callback.Run(net::ERR_IO_PENDING);
+    return;
+  } else {
+    write_callback_ = callback;
+  }
+
+  int result = net::ERR_FAILED;
+  do {
+    if (!socket_.get() || !socket_->IsConnected()) {
+      result = net::ERR_SOCKET_NOT_CONNECTED;
+      break;
+    }
+
+    result = socket_->Write(
+        io_buffer.get(), byte_count,
+        base::Bind(&TCPSocket::OnWriteComplete, base::Unretained(this)));
+  } while (0);
+
+  if (result != net::ERR_IO_PENDING)
+    OnWriteComplete(result);
 }
 
-int TCPSocket::RecvFrom(scoped_refptr<net::IOBuffer> io_buffer,
-                        int io_buffer_len,
-                        net::IPEndPoint* address) {
-  return net::ERR_FAILED;
+void TCPSocket::RecvFrom(int count,
+                         const RecvFromCompletionCallback& callback) {
+  callback.Run(net::ERR_FAILED, NULL, NULL, 0);
 }
 
-int TCPSocket::SendTo(scoped_refptr<net::IOBuffer> io_buffer,
-                      int byte_count,
-                      const std::string& address,
-                      int port) {
-  return net::ERR_FAILED;
+void TCPSocket::SendTo(scoped_refptr<net::IOBuffer> io_buffer,
+                       int byte_count,
+                       const std::string& address,
+                       int port,
+                       const CompletionCallback& callback) {
+  callback.Run(net::ERR_FAILED);
 }
 
-void TCPSocket::OnConnect(int result) {
+void TCPSocket::OnConnectComplete(int result) {
+  DCHECK(!connect_callback_.is_null());
+  DCHECK(!is_connected_);
   is_connected_ = result == net::OK;
-  event_notifier()->OnConnectComplete(result);
+  connect_callback_.Run(result);
+  connect_callback_.Reset();
+}
+
+void TCPSocket::OnReadComplete(scoped_refptr<net::IOBuffer> io_buffer,
+                               int result) {
+  DCHECK(!read_callback_.is_null());
+  read_callback_.Run(result, io_buffer);
+  read_callback_.Reset();
+}
+
+void TCPSocket::OnWriteComplete(int result) {
+  DCHECK(!write_callback_.is_null());
+  write_callback_.Run(result);
+  write_callback_.Reset();
 }
 
 }  // namespace extensions