Add DefaultListenSocket.

net/base/default_listen_socket.h

 // 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.
 
-// TCP/IP server that handles IO asynchronously in the specified MessageLoop.
-// These objects are NOT thread safe.  They use WSAEVENT handles to monitor
-// activity in a given MessageLoop.  This means that callbacks will
-// happen in that loop's thread always and that all other methods (including
-// constructors and destructors) should also be called from the same thread.
+// Default (abstract) listen socket implementation that can be used by clients

[mmenke, 2012/04/25 17:25:31]

Default isn't really an accurate description, or at least isn't very clear. 
Suggest you mention that this is a stream-based socket implementation, that this
handles reading and writing to the socket, but does not handle creating the
listen socket or connected sockets, which are handled by subclasses on creation
and in Accept, respectively.

[Philippe, 2012/04/30 06:55:21]

Indeed.

+// to implement more specific classes like TCPListenSocket and UnixDomainSocket.
 
-#ifndef NET_BASE_TCP_LISTEN_SOCKET_H_
-#define NET_BASE_TCP_LISTEN_SOCKET_H_
+// Server that handles IO asynchronously in the specified MessageLoop. These
+// objects are NOT thread safe. They use WSAEVENT handles to monitor activity
+// in a given MessageLoop. This means that callbacks will happen in that loop's
+// thread always and that all other methods (including constructors and
+// destructors) should also be called from the same thread.
+
+#ifndef NET_BASE_DEFAULT_LISTEN_SOCKET_H_
+#define NET_BASE_DEFAULT_LISTEN_SOCKET_H_
 #pragma once
 
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
 #include <winsock2.h>
 #endif
 #include <string>
 #if defined(OS_WIN)
 #include "base/win/object_watcher.h"
 #elif defined(OS_POSIX)
 #include "base/message_loop.h"
 #endif
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "net/base/listen_socket.h"
 #include "net/base/net_export.h"
 
 #if defined(OS_POSIX)
 typedef int SOCKET;
 #endif
 
 namespace net {
 
-// Implements a TCP socket interface. Note that this is ref counted.
-class NET_EXPORT TCPListenSocket : public ListenSocket,
+class NET_EXPORT DefaultListenSocket

[mmenke, 2012/04/25 17:25:31]

Suggest you call this ListenSocketBase.  It has two advantages over the current
name:

1)  In alphabetical sorting of files, it's right next to listen_socket.

2)  "default" isn't a terribly accurate description.

Other options would be ListenSocketStream, or StreamListenSocket, which would
perhaps be even more accurate.

[mmenke, 2012/04/25 17:25:31]

Are there any plans to make any other direct subclass of ListenSocket?

[Philippe, 2012/04/30 06:55:21]

I liked StreamListenSocket.

[Philippe, 2012/04/30 06:55:21]

I don't think so. I guess that your underlying suggestion was to merge
ListenSocket into this class. I made the change.

[mmenke, 2012/04/30 19:11:44]

Yea, I was wondering if there was any need for it.

+    : public ListenSocket,
 #if defined(OS_WIN)
-                                   public base::win::ObjectWatcher::Delegate {
+      public base::win::ObjectWatcher::Delegate {
 #elif defined(OS_POSIX)
-                                   public MessageLoopForIO::Watcher {
+      public MessageLoopForIO::Watcher {
 #endif
- public:
-  // Listen on port for the specified IP address.  Use 127.0.0.1 to only
-  // accept local connections.
-  static TCPListenSocket* CreateAndListen(std::string ip, int port,
-                                          ListenSocketDelegate* del);
-
-  // NOTE: This is for unit test use only!
-  // Pause/Resume calling Read().  Note that ResumeReads() will also call
-  // Read() if there is anything to read.
-  void PauseReads();
-  void ResumeReads();
-
  protected:

[mmenke, 2012/04/25 17:25:31]

nit:  Please add a blank line above the "protected:"

[Philippe, 2012/04/30 06:55:21]

Done.

   enum WaitState {
     NOT_WAITING      = 0,
     WAITING_ACCEPT   = 1,
     WAITING_READ     = 2
   };
 
   static const SOCKET kInvalidSocket;
   static const int kSocketError;
 
-  TCPListenSocket(SOCKET s, ListenSocketDelegate* del);
-  virtual ~TCPListenSocket();
-  static SOCKET CreateAndBind(std::string ip, int port);
-  // if valid, returned SOCKET is non-blocking
+  DefaultListenSocket(SOCKET s, ListenSocketDelegate* del);
+  virtual ~DefaultListenSocket();
+
   static SOCKET Accept(SOCKET s);

[mmenke, 2012/04/25 17:25:31]

There doesn't seem to be any reason to make the current Accept function static,
that I'm aware of.  I suggest just a normal protected function, and then it
looks like we could make socket_ private, unless it's needed by unit tests.

[mmenke, 2012/04/25 17:27:20]

Just to clarify, as a non-static function, it wouldn't take an argument, but
rather just use our private socket_.

[Philippe, 2012/04/30 06:55:21]

Good point.

 
   // Implements ListenSocket::SendInternal.
   virtual void SendInternal(const char* bytes, int len) OVERRIDE;
 
   virtual void Listen();

[mmenke, 2012/04/25 17:25:31]

It looks to me like Listen, Read, Close, and CloseSocket don't need to be
virtual (Even before you get rid of one HttpListenSocket::Listen and rename the
other, actually, but afterwards, too).

[Philippe, 2012/04/30 06:55:21]

Done.

-  virtual void Accept();
+  virtual void AcceptInternal() = 0;

[mmenke, 2012/04/25 17:25:31]

This naming seems a little weird.  SendInternal is how things generally work:  A
public Send function does stuff and then call the private child class's Send
function.

With AcceptInternal, not only which one calls the other reversed, but Accept is
also a private function.

My suggestion is to rename AcceptInternal to Accept, and Accept to AcceptSocket.

[Philippe, 2012/04/30 06:55:21]

Done.

   virtual void Read();
   virtual void Close();
   virtual void CloseSocket(SOCKET s);
 
   // Pass any value in case of Windows, because in Windows
   // we are not using state.
   void WatchSocket(WaitState state);
   void UnwatchSocket();
 
 #if defined(OS_WIN)
   // ObjectWatcher delegate
   virtual void OnObjectSignaled(HANDLE object);
   base::win::ObjectWatcher watcher_;
   HANDLE socket_event_;
 #elif defined(OS_POSIX)
   // Called by MessagePumpLibevent when the socket is ready to do I/O
   virtual void OnFileCanReadWithoutBlocking(int fd) OVERRIDE;
   virtual void OnFileCanWriteWithoutBlocking(int fd) OVERRIDE;
   WaitState wait_state_;
   // The socket's libevent wrapper
   MessageLoopForIO::FileDescriptorWatcher watcher_;
 #endif
 
-  SOCKET socket_;
+  const SOCKET socket_;
 
  private:
+  // NOTE: This is for unit test use only!
+  // Pause/Resume calling Read().  Note that ResumeReads() will also call
+  // Read() if there is anything to read.
+  friend class TransportClientSocketTest;

[mmenke, 2012/04/25 17:25:31]

nit:  Please add a linebreak between the friend declaration and the first
function.

[Philippe, 2012/04/30 06:55:21]

Done.

+  void PauseReads();
+  void ResumeReads();
+
   bool reads_paused_;
   bool has_pending_reads_;
 
-  DISALLOW_COPY_AND_ASSIGN(TCPListenSocket);
+  DISALLOW_COPY_AND_ASSIGN(DefaultListenSocket);
 };
 
 }  // namespace net
 
-#endif  // NET_BASE_TCP_LISTEN_SOCKET_H_
+#endif  // NET_BASE_DEFAULT_LISTEN_SOCKET_H_