Refactored adopting a socket for devtools into cleaner, more idiomatic code.

headless/lib/browser/headless_devtools.cc

 // Copyright 2015 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 "headless/lib/browser/headless_devtools.h"
 
 #include <string>
 #include <utility>
 
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/devtools_socket_factory.h"
 #include "content/public/browser/navigation_entry.h"
 #include "headless/grit/headless_lib_resources.h"
 #include "headless/public/headless_browser.h"
 #include "net/base/net_errors.h"
 #include "net/log/net_log_source.h"
 #include "net/socket/tcp_server_socket.h"
 #include "ui/base/resource/resource_bundle.h"
 
 namespace headless {
 
 namespace {
 
 const int kBackLog = 10;
 
 class TCPServerSocketFactory : public content::DevToolsSocketFactory {
+ private:
+  std::unique_ptr<net::ServerSocket> CreateForTethering(

[mcgreevy, 2017/05/23 04:31:07]

I'm not a fan of implementation inheritance, and I don't think there's really a
need for it here (having said that, I'm not familiar with the codebase
conventions).  If you just duplicate this method in
TCPEndpointServerSocketFactory and TCPAdoptServerSocketFactory, then you can
remove this whole type. You would end up with a pure interface
(DevToolsSocketFactory) and two implementations (in this file, at least) of that
interface, rather than an inheritance hierarchy with implementation sprinkled
through it.

[Raul Vera, 2017/05/24 00:22:30]

What you suggest is what I did first. I added the inheritance to avoid
duplication. What's wrong with implementation inheritance if all the subclasses
do the exact same thing for that method? Particularly when the whole hierarchy
is right here in the one file. You would really prefer duplication? Admittedly
the method here is small, so I'll do it, but still.

Done.

[mcgreevy, 2017/05/24 01:23:19]

In general I find it harder to reason about code where logic is spread through
the inheritance hierarchy (this is especially the case where one method calls
another method from a different level in the hierarchy, sometimes even bouncing
up and down the hierarchy. This is not the case here, obviously).

I think that the extra boilerplate (whether it's duplication of straightforward
logic, such as here, or a small method which makes a call to some code that
you've factored out into another class or function) is worth it for the simpler
structure.  Concretely, if you have a DevToolsSocketFactory that is of dynamic
type TCPEndpointServerSocketFactory, you can find all of its implementation in
the TCPEndpointServerSocketFactory class.  If CreateForTethering were to require
real logic, you could factor that out and call it explicitly from each of the
leaf classes. A future reader could then still start understanding the
implementation of TCPEndpointServerSocketFactory by looking at its code, and
then following the explicit call to the factored out implementation, rather than
needing to figure out which override in the hierarchy provides the
implementation.

Obviously the code in this CL is a simple case, where it wouldn't really hurt to
go either way, but it think it's better to start with a flatter structure,
because code tends to have a tendency to grow over time and no longer be so
simple.
The fact that they are all in the same file does help mitigate my concern, and
the total amount of code here is small, so it relatively easy to understand. 
But if this code grew, the extra inheritance would IMO be a drag on readability
and simplicity.

+      std::string* out_name) override {
+    return nullptr;
+  }
+};
+
+class TCPEndpointServerSocketFactory : public TCPServerSocketFactory {
  public:
-  explicit TCPServerSocketFactory(const net::IPEndPoint& endpoint)
-      : endpoint_(endpoint), socket_fd_(0) {
+  explicit TCPEndpointServerSocketFactory(const net::IPEndPoint& endpoint)
+      : endpoint_(endpoint) {
     DCHECK(endpoint_.address().IsValid());

[mcgreevy, 2017/05/23 04:31:07]

It's odd that CreateForHttpServer and CreateForTethering were (and still are)
private when they are declared as public in DevToolsSocketFactory.  I suppose
this *works* because once one of these factories is created, it's passed around
as a DevToolsSocketFactory (where these members are public), but it feels a bit
odd that you wouldn't be able to call these methods on the derived types.

[Raul Vera, 2017/05/24 00:22:30]

There was a long thread about this on chromium-dev a few weeks ago, becasue I
gather it's pretty common in the codebase. It's an odd pattern, but it makes
some sense when the subclasses should never be called directly. Note that they
are also declared in an unnamed namespace so they can't be accessed outside this
file. Defence in depth I guess. All of this was already here, and I don't want
to change the relationship of this code to the rest of devtools if I don't need
to.

[mcgreevy, 2017/05/24 01:23:19]

Fair enough.

   }
 
-  explicit TCPServerSocketFactory(const size_t socket_fd)
+ private:
+  // content::DevToolsSocketFactory implementation:

[mcgreevy, 2017/05/23 04:31:07]

These comments seem unnecessary.

[Raul Vera, 2017/05/24 00:22:30]

They were there before. I'll remove them.

Done.

+  std::unique_ptr<net::ServerSocket> CreateForHttpServer() override {
+    std::unique_ptr<net::ServerSocket> socket(
+        new net::TCPServerSocket(nullptr, net::NetLogSource()));
+    if (socket->Listen(endpoint_, kBackLog) != net::OK)
+      return std::unique_ptr<net::ServerSocket>();
+    return socket;
+  }
+
+  net::IPEndPoint endpoint_;
+
+  DISALLOW_COPY_AND_ASSIGN(TCPEndpointServerSocketFactory);
+};
+
+#if defined(OS_POSIX)

[mcgreevy, 2017/05/23 04:31:07]

Does this really need to be conditionally compiled?  I don't know what the
typical style is in this codebase, but AFAICT this will slightly reduce the
binary size at the cost of readability. I don't think it's needed for
correctness, as the conditional compilation on line 91 should take care of that.
 And I don't see anything here that would prevent this code from actually
compiling on other platforms.  But maybe I'm missing something.

[Raul Vera, 2017/05/24 00:22:30]

It won't compile as is on Windows, where a socket is a SOCKET instead of a
size_t, and I can't pass one in on the command line nor call AdoptSocket with a
size_t. In /net SocketSescriptor is defined conditionally to make the network
code cross platform (compile-time polymorphism ftw :-), so I would have to
include that and rewrite this stuff to match, even though it won't ever get
called. I'm not sure it would be any more readable, either. 

[mcgreevy, 2017/05/24 01:23:19]

OK.

+class TCPAdoptServerSocketFactory : public TCPServerSocketFactory {
+ public:
+  explicit TCPAdoptServerSocketFactory(const size_t socket_fd)
       : socket_fd_(socket_fd) {}
 
  private:
   // content::DevToolsSocketFactory implementation:
   std::unique_ptr<net::ServerSocket> CreateForHttpServer() override {
-    if (!socket_fd_) {
-      std::unique_ptr<net::ServerSocket> socket(
-          new net::TCPServerSocket(nullptr, net::NetLogSource()));
-      if (socket->Listen(endpoint_, kBackLog) != net::OK)
-        return std::unique_ptr<net::ServerSocket>();
-      return socket;
-    }
-#if defined(OS_POSIX)
     std::unique_ptr<net::TCPServerSocket> tsock(
         new net::TCPServerSocket(nullptr, net::NetLogSource()));
     if (tsock->AdoptSocket(socket_fd_) != net::OK) {
       LOG(ERROR) << "Failed to adopt open socket";
       return std::unique_ptr<net::ServerSocket>();
     }
-    return std::unique_ptr<net::ServerSocket>(tsock.release());
-#else
-    LOG(ERROR) << "Can't inherit an open socket on non-Posix systems";
-    return std::unique_ptr<net::ServerSocket>();
-#endif
+    // Note that we assume that the socket is already listening, so we don't

[mcgreevy, 2017/05/23 04:31:07]

You should probably document that assumption on the TCPAdoptServerSocketFactory
constructor.

[Raul Vera, 2017/05/24 00:22:30]

Done. And clarified this comment, too.

+    // call Listen.
+    return std::unique_ptr<net::ServerSocket>(std::move(tsock));
   }
 
-  std::unique_ptr<net::ServerSocket> CreateForTethering(
-      std::string* out_name) override {
-    return nullptr;
-  }
-
-  net::IPEndPoint endpoint_;
   size_t socket_fd_;
 
-  DISALLOW_COPY_AND_ASSIGN(TCPServerSocketFactory);
+  DISALLOW_COPY_AND_ASSIGN(TCPAdoptServerSocketFactory);
 };
-
+#endif  // defined(OS_POSIX)
 }  // namespace
 
 void StartLocalDevToolsHttpHandler(HeadlessBrowser::Options* options) {
   std::unique_ptr<content::DevToolsSocketFactory> socket_factory;
   if (options->devtools_socket_fd == 0) {
     const net::IPEndPoint& endpoint = options->devtools_endpoint;
-    socket_factory.reset(new TCPServerSocketFactory(endpoint));
+    socket_factory.reset(new TCPEndpointServerSocketFactory(endpoint));
   } else {
+#if defined(OS_POSIX)
     const uint16_t socket_fd = options->devtools_socket_fd;
-    socket_factory.reset(new TCPServerSocketFactory(socket_fd));
+    socket_factory.reset(new TCPAdoptServerSocketFactory(socket_fd));
+#else
+    LOG(ERROR) << "Can't inherit an open socket on non-Posix systems";

[mcgreevy, 2017/05/23 04:31:07]

In this case, socket_factory will be a null pointer, which you then pass into
StartRemoteDebuggingServer.  I don't think you want to do this.  I'd suggest
creating another class which derives from content::DevToolsSocketFactory and
just returns nullptr from its CreateForHttpServer method, and instantiate one of
those here.

[Raul Vera, 2017/05/24 00:22:30]

Good point. Done.

+#endif
   }
   content::DevToolsAgentHost::StartRemoteDebuggingServer(
       std::move(socket_factory), std::string(),
       options->user_data_dir,  // TODO(altimin): Figure a proper value for this.
       base::FilePath(), options->product_name_and_version, options->user_agent);
 }
 
 void StopLocalDevToolsHttpHandler() {
   content::DevToolsAgentHost::StopRemoteDebuggingServer();
 }
 
 }  // namespace headless