Miscellaneous changes (mostly cleanup) from my code review.

R=darin@google.com

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@354 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 90 insertions, 61 deletions

diff --git a/net/base/ssl_client_socket.h b/net/base/ssl_client_socket.h
index 599b488..3e1779f 100644
--- a/net/base/ssl_client_socket.h
+++ b/net/base/ssl_client_socket.h
@@ -35,12 +35,16 @@
 #include <windows.h>
 #include <security.h>
 
+#include <string>
+
 #include "base/scoped_ptr.h"
 #include "net/base/client_socket.h"
 #include "net/base/completion_callback.h"
 
 namespace net {
 
+// A client socket that uses SSL as the transport layer.
+//
 // NOTE: The SSL handshake occurs within the Connect method after a TCP
 // connection is established.  If a SSL error occurs during the handshake,
 // Connect will fail.  The consumer may choose to ignore certain SSL errors,
diff --git a/net/base/ssl_client_socket_unittest.cc b/net/base/ssl_client_socket_unittest.cc
index a465563..ebcce65 100644
--- a/net/base/ssl_client_socket_unittest.cc
+++ b/net/base/ssl_client_socket_unittest.cc
@@ -28,8 +28,8 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include "net/base/address_list.h"
-#include "net/base/net_errors.h"
 #include "net/base/host_resolver.h"
+#include "net/base/net_errors.h"
 #include "net/base/ssl_client_socket.h"
 #include "net/base/tcp_client_socket.h"
 #include "net/base/test_completion_callback.h"
@@ -60,10 +60,12 @@ TEST_F(SSLClientSocketTest, Connect) {
   EXPECT_FALSE(sock.IsConnected());
 
   rv = sock.Connect(&callback);
-  ASSERT_EQ(net::ERR_IO_PENDING, rv);
+  if (rv != net::OK) {
+    ASSERT_EQ(net::ERR_IO_PENDING, rv);
 
-  rv = callback.WaitForResult();
-  EXPECT_EQ(net::OK, rv);
+    rv = callback.WaitForResult();
+    EXPECT_EQ(net::OK, rv);
+  }
 
   EXPECT_TRUE(sock.IsConnected());
 
@@ -87,18 +89,20 @@ TEST_F(SSLClientSocketTest, Read) {
   net::SSLClientSocket sock(new net::TCPClientSocket(addr), hostname);
 
   rv = sock.Connect(&callback);
-  ASSERT_EQ(rv, net::ERR_IO_PENDING);
+  if (rv != net::OK) {
+    ASSERT_EQ(rv, net::ERR_IO_PENDING);
 
-  rv = callback.WaitForResult();
-  EXPECT_EQ(rv, net::OK);
+    rv = callback.WaitForResult();
+    EXPECT_EQ(rv, net::OK);
+  }
 
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
-  rv = sock.Write(request_text, arraysize(request_text)-1, &callback);
+  rv = sock.Write(request_text, arraysize(request_text) - 1, &callback);
   EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
 
   if (rv == net::ERR_IO_PENDING) {
     rv = callback.WaitForResult();
-    EXPECT_EQ(rv, arraysize(request_text)-1);
+    EXPECT_EQ(rv, arraysize(request_text) - 1);
   }
 
   char buf[4096];
@@ -109,12 +113,13 @@ TEST_F(SSLClientSocketTest, Read) {
     if (rv == net::ERR_IO_PENDING)
       rv = callback.WaitForResult();
 
-    if (rv == 0)
+    EXPECT_GE(rv, 0);
+    if (rv <= 0)
       break;
   }
 }
 
-TEST_F(TCPClientSocketTest, Read_SmallChunks) {
+TEST_F(SSLClientSocketTest, Read_SmallChunks) {
   net::AddressList addr;
   net::HostResolver resolver;
   TestCompletionCallback callback;
@@ -125,18 +130,20 @@ TEST_F(TCPClientSocketTest, Read_SmallChunks) {
   net::TCPClientSocket sock(addr);
 
   rv = sock.Connect(&callback);
-  ASSERT_EQ(rv, net::ERR_IO_PENDING);
+  if (rv != net::OK) {
+    ASSERT_EQ(rv, net::ERR_IO_PENDING);
 
-  rv = callback.WaitForResult();
-  EXPECT_EQ(rv, net::OK);
+    rv = callback.WaitForResult();
+    EXPECT_EQ(rv, net::OK);
+  }
 
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
-  rv = sock.Write(request_text, arraysize(request_text)-1, &callback);
+  rv = sock.Write(request_text, arraysize(request_text) - 1, &callback);
   EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
 
   if (rv == net::ERR_IO_PENDING) {
     rv = callback.WaitForResult();
-    EXPECT_EQ(rv, arraysize(request_text)-1);
+    EXPECT_EQ(rv, arraysize(request_text) - 1);
   }
 
   char buf[1];
@@ -147,12 +154,13 @@ TEST_F(TCPClientSocketTest, Read_SmallChunks) {
     if (rv == net::ERR_IO_PENDING)
       rv = callback.WaitForResult();
 
-    if (rv == 0)
+    EXPECT_GE(rv, 0);
+    if (rv <= 0)
       break;
   }
 }
 
-TEST_F(TCPClientSocketTest, Read_Interrupted) {
+TEST_F(SSLClientSocketTest, Read_Interrupted) {
   net::AddressList addr;
   net::HostResolver resolver;
   TestCompletionCallback callback;
@@ -163,18 +171,20 @@ TEST_F(TCPClientSocketTest, Read_Interrupted) {
   net::TCPClientSocket sock(addr);
 
   rv = sock.Connect(&callback);
-  ASSERT_EQ(rv, net::ERR_IO_PENDING);
+  if (rv != net::OK) {
+    ASSERT_EQ(rv, net::ERR_IO_PENDING);
 
-  rv = callback.WaitForResult();
-  EXPECT_EQ(rv, net::OK);
+    rv = callback.WaitForResult();
+    EXPECT_EQ(rv, net::OK);
+  }
 
   const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
-  rv = sock.Write(request_text, arraysize(request_text)-1, &callback);
+  rv = sock.Write(request_text, arraysize(request_text) - 1, &callback);
   EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING);
 
   if (rv == net::ERR_IO_PENDING) {
     rv = callback.WaitForResult();
-    EXPECT_EQ(rv, arraysize(request_text)-1);
+    EXPECT_EQ(rv, arraysize(request_text) - 1);
   }
 
   // Do a partial read and then exit.  This test should not crash!
@@ -185,6 +195,6 @@ TEST_F(TCPClientSocketTest, Read_Interrupted) {
   if (rv == net::ERR_IO_PENDING)
     rv = callback.WaitForResult();
 
-  EXPECT_TRUE(rv != 0);
+  EXPECT_NE(rv, 0);
 }
 #endif
diff --git a/net/http/http_chunked_decoder_unittest.cc b/net/http/http_chunked_decoder_unittest.cc
index 602ba6a..377f1b1 100644
--- a/net/http/http_chunked_decoder_unittest.cc
+++ b/net/http/http_chunked_decoder_unittest.cc
@@ -46,7 +46,7 @@ void RunTest(const char* inputs[], size_t num_inputs,
   for (size_t i = 0; i < num_inputs; ++i) {
     std::string input = inputs[i];
     int n = decoder.FilterBuf(&input[0], static_cast<int>(input.size()));
-    EXPECT_TRUE(n >= 0);
+    EXPECT_GE(n, 0);
     if (n > 0)
       result.append(input.data(), n);
   }
@@ -64,6 +64,13 @@ TEST(HttpChunkedDecoderTest, Basic) {
   RunTest(inputs, arraysize(inputs), "hello", true);
 }
 
+TEST(HttpChunkedDecoderTest, OneChunk) {
+  const char* inputs[] = {
+    "5\r\nhello\r\n"
+  };
+  RunTest(inputs, arraysize(inputs), "hello", false);
+}
+
 TEST(HttpChunkedDecoderTest, Typical) {
   const char* inputs[] = {
     "5\r\nhello\r\n",
diff --git a/net/http/http_connection.h b/net/http/http_connection.h
index ada8468..e3a6496 100644
--- a/net/http/http_connection.h
+++ b/net/http/http_connection.h
@@ -30,6 +30,8 @@
 #ifndef NET_HTTP_HTTP_CONNECTION_H_
 #define NET_HTTP_HTTP_CONNECTION_H_
 
+#include <string>
+
 #include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
 #include "net/base/client_socket.h"
@@ -50,11 +52,12 @@ class HttpConnectionManager;
 //
 class HttpConnection {
  public:
-  HttpConnection(HttpConnectionManager* mgr);
+  explicit HttpConnection(HttpConnectionManager* mgr);
   ~HttpConnection();
 
   // Initializes a HttpConnection object, which involves talking to the
-  // HttpConnectionManager to locate a socket to possibly reuse.
+  // HttpConnectionManager to locate a socket to possibly reuse.  This method
+  // returns either OK or ERR_IO_PENDING.
   //
   // If this method succeeds, then the socket member will be set to an existing
   // socket if an existing socket was available to reuse.  Otherwise, the
@@ -69,9 +72,9 @@ class HttpConnection {
 
   // An initialized connection can be reset, which causes it to return to the
   // un-initialized state.  This releases the underlying socket, which in the
-  // case of a socket that is not closed, indicates that the socket may be kept
-  // alive for use by a subsequent HttpConnection.  NOTE: To prevent the socket
-  // from being kept alive, be sure to call its Close method.
+  // case of a socket that is not disconnected, indicates that the socket may
+  // be kept alive for use by a subsequent HttpConnection.  NOTE: To prevent
+  // the socket from being kept alive, be sure to call its Disconnect method.
   void Reset();
 
   // Returns true when Init has completed successfully.
diff --git a/net/http/http_connection_manager.cc b/net/http/http_connection_manager.cc
index a7edd92..7064c4f 100644
--- a/net/http/http_connection_manager.cc
+++ b/net/http/http_connection_manager.cc
@@ -33,18 +33,25 @@
 #include "net/base/client_socket.h"
 #include "net/base/net_errors.h"
 
+namespace {
+
+// The timeout value, in seconds, used to clean up disconnected idle sockets.
+const int kCleanupInterval = 5;
+
+}  // namespace
+
 namespace net {
 
 HttpConnectionManager::HttpConnectionManager()
-    : timer_(TimeDelta::FromSeconds(5)),
-      idle_count_(0) {
+    : timer_(TimeDelta::FromSeconds(kCleanupInterval)),
+      idle_socket_count_(0) {
   timer_.set_task(this);
 }
 
 HttpConnectionManager::~HttpConnectionManager() {
   timer_.set_task(NULL);
 
-  // Cleanup any idle sockets.  Assert that we have no remaining active sockets
+  // Clean up any idle sockets.  Assert that we have no remaining active sockets
   // or pending requests.  They should have all been cleaned up prior to the
   // manager being destroyed.
 
@@ -55,13 +62,13 @@ HttpConnectionManager::~HttpConnectionManager() {
 int HttpConnectionManager::RequestSocket(const std::string& group_name,
                                          SocketHandle** handle,
                                          CompletionCallback* callback) {
-  Group& group =
-      group_map_.insert(std::make_pair(group_name, Group())).first->second;
+  Group& group = group_map_[group_name];
 
   // Can we make another active socket now?
   if (group.active_socket_count == kMaxSocketsPerGroup) {
     Request r;
     r.result = handle;
+    DCHECK(callback);
     r.callback = callback;
     group.pending_requests.push_back(r);
     return ERR_IO_PENDING;
@@ -70,18 +77,18 @@ int HttpConnectionManager::RequestSocket(const std::string& group_name,
   // OK, we are going to activate one.
   group.active_socket_count++;
 
-  // Use idle sockets in LIFO order.
+  // Use idle sockets in LIFO order because they're more likely to be
+  // still connected.
   while (!group.idle_sockets.empty()) {
     SocketHandle* h = group.idle_sockets.back();
     group.idle_sockets.pop_back();
     DecrementIdleCount();
-    if (!h->get()->IsConnected()) {
-      delete h;
-    } else {
+    if (h->get()->IsConnected()) {
       // We found one we can reuse!
       *handle = h;
       return OK;
     }
+    delete h;
   }
 
   *handle = new SocketHandle();
@@ -93,7 +100,8 @@ void HttpConnectionManager::CancelRequest(const std::string& group_name,
   Group& group = group_map_[group_name];
 
   // In order for us to be canceling a pending request, we must have active
-  // sockets equaling the limit.
+  // sockets equaling the limit.  NOTE: The correctness of the code doesn't
+  // require this assertion.
   DCHECK(group.active_socket_count == kMaxSocketsPerGroup);
 
   // Search pending_requests for matching handle.
@@ -120,7 +128,7 @@ void HttpConnectionManager::CloseIdleSockets() {
 
 void HttpConnectionManager::MaybeCloseIdleSockets(
     bool only_if_disconnected) {
-  if (idle_count_ == 0)
+  if (idle_socket_count_ == 0)
     return;
 
   GroupMap::iterator i = group_map_.begin();
@@ -149,12 +157,12 @@ void HttpConnectionManager::MaybeCloseIdleSockets(
 }
 
 void HttpConnectionManager::IncrementIdleCount() {
-  if (++idle_count_ == 1)
+  if (++idle_socket_count_ == 1)
     timer_.Start();
 }
 
 void HttpConnectionManager::DecrementIdleCount() {
-  if (--idle_count_ == 0)
+  if (--idle_socket_count_ == 0)
     timer_.Stop();
 }
 
@@ -180,8 +188,9 @@ void HttpConnectionManager::DoReleaseSocket(const std::string& group_name,
   if (!group.pending_requests.empty()) {
     Request r = group.pending_requests.front();
     group.pending_requests.pop_front();
-    RequestSocket(i->first, r.result, NULL);
-    r.callback->Run(OK);
+    int rv = RequestSocket(i->first, r.result, NULL);
+    DCHECK(rv == OK);
+    r.callback->Run(rv);
     return;
   }
 
diff --git a/net/http/http_connection_manager.h b/net/http/http_connection_manager.h
index 22080a3..fc5402c 100644
--- a/net/http/http_connection_manager.h
+++ b/net/http/http_connection_manager.h
@@ -32,6 +32,7 @@
 
 #include <deque>
 #include <map>
+#include <string>
 
 #include "base/ref_counted.h"
 #include "base/timer.h"
@@ -45,7 +46,7 @@ class ClientSocket;
 // open at a time.  It also maintains a list of idle persistent sockets.
 //
 // The HttpConnectionManager allocates SocketHandle objects, but it is not
-// responsible for allocating the associated ClientSocket object.  The
+// responsible for allocating the associated ClientSocket objects.  The
 // consumer must do so if it gets a SocketHandle with a null ClientSocket.
 //
 class HttpConnectionManager : public base::RefCounted<HttpConnectionManager>,
@@ -62,11 +63,10 @@ class HttpConnectionManager : public base::RefCounted<HttpConnectionManager>,
   //
   // If this function returns OK, then |*handle| will reference a SocketHandle
   // object.  If ERR_IO_PENDING is returned, then the completion callback will
-  // be called when |*handle| has been populated.  Otherwise, an error code is
-  // returned.
+  // be called when |*handle| has been populated.
   //
   // If the resultant SocketHandle object has a null member, then it is the
-  // callers job to create a ClientSocket and associate it with the handle.
+  // caller's job to create a ClientSocket and associate it with the handle.
   //
   int RequestSocket(const std::string& group_name, SocketHandle** handle,
                     CompletionCallback* callback);
@@ -91,7 +91,9 @@ class HttpConnectionManager : public base::RefCounted<HttpConnectionManager>,
   ~HttpConnectionManager();
 
   // Closes all idle sockets if |only_if_disconnected| is false.  Else, only
-  // idle sockets that are disconnected get closed.
+  // idle sockets that are disconnected get closed.  "Maybe" refers to the
+  // fact that it doesn't close all idle sockets if |only_if_disconnected| is
+  // true.
   void MaybeCloseIdleSockets(bool only_if_disconnected);
 
   // Called when the number of idle sockets changes.
@@ -115,10 +117,10 @@ class HttpConnectionManager : public base::RefCounted<HttpConnectionManager>,
   // A Group is allocated per group_name when there are idle sockets or pending
   // requests.  Otherwise, the Group object is removed from the map.
   struct Group {
+    Group() : active_socket_count(0) {}
     std::deque<SocketHandle*> idle_sockets;
     std::deque<Request> pending_requests;
     int active_socket_count;
-    Group() : active_socket_count(0) {}
   };
 
   typedef std::map<std::string, Group> GroupMap;
@@ -128,7 +130,7 @@ class HttpConnectionManager : public base::RefCounted<HttpConnectionManager>,
   RepeatingTimer timer_;
 
   // The total number of idle sockets in the system.
-  int idle_count_;
+  int idle_socket_count_;
 };
 
 }  // namespace net
diff --git a/net/http/http_connection_manager_unittest.cc b/net/http/http_connection_manager_unittest.cc
index 9eade18..503097b 100644
--- a/net/http/http_connection_manager_unittest.cc
+++ b/net/http/http_connection_manager_unittest.cc
@@ -67,12 +67,6 @@ class MockClientSocket : public net::ClientSocket {
                     net::CompletionCallback* callback) {
     return net::ERR_FAILED;
   }
-  virtual int GetProperty(int property, void* buf, int buf_len) const {
-    return net::ERR_FAILED;
-  }
-  virtual int SetProperty(int property, const void* buf, int buf_len) {
-    return net::ERR_FAILED;
-  }
 
   static int allocation_count;
 
diff --git a/net/http/http_network_layer.h b/net/http/http_network_layer.h
index 72fbca3..db7d0d9 100644
--- a/net/http/http_network_layer.h
+++ b/net/http/http_network_layer.h
@@ -40,6 +40,9 @@ class HttpProxyInfo;
 
 class HttpNetworkLayer : public HttpTransactionFactory {
  public:
+  explicit HttpNetworkLayer(const HttpProxyInfo* pi);
+  ~HttpNetworkLayer();
+
   // This function hides the details of how a network layer gets instantiated
   // and allows other implementations to be substituted.
   static HttpTransactionFactory* CreateFactory(const HttpProxyInfo* pi);
@@ -47,9 +50,6 @@ class HttpNetworkLayer : public HttpTransactionFactory {
   // If value is true, then WinHTTP will be used.
   static void UseWinHttp(bool value);
 
-  HttpNetworkLayer(const HttpProxyInfo* pi);
-  ~HttpNetworkLayer();
-
   // HttpTransactionFactory methods:
   virtual HttpTransaction* CreateTransaction();
   virtual HttpCache* GetCache();