Move Python setup/tear down logic into Forwarder itself.

build/android/pylib/base/base_test_runner.py

Index: build/android/pylib/base/base_test_runner.py
diff --git a/build/android/pylib/base/base_test_runner.py b/build/android/pylib/base/base_test_runner.py
index 3fb6c269c787b9888748890ab556ed77dc3349be..159f9a7ee65d4a2588f0c652f3c8119ab750c423 100644
--- a/build/android/pylib/base/base_test_runner.py
+++ b/build/android/pylib/base/base_test_runner.py
@@ -46,14 +46,12 @@ class BaseTestRunner(object):
     self.adb = android_commands.AndroidCommands(device=device)
     self.tool = CreateTool(tool, self.adb)
     self._http_server = None
-    self._forwarder = None
     self._forwarder_device_port = 8000
     self.forwarder_base_url = ('http://localhost:%d' %
         self._forwarder_device_port)
     self.flags = FlagChanger(self.adb)
     self.flags.AddFlags(['--disable-fre'])
     self._spawning_server = None
-    self._spawner_forwarder = None
     # We will allocate port for test server spawner when calling method
     # LaunchChromeTestServerSpawner and allocate port for test server when
     # starting it in TestServerThread.
@@ -91,7 +89,6 @@ class BaseTestRunner(object):
 
   def SetUp(self):
     """Run once before all tests are run."""
-    Forwarder.KillDevice(self.adb, self.tool)
     self.InstallTestPackage()
     push_size_before = self.adb.GetPushSizeInfo()
     if self._push_deps:
@@ -141,49 +138,38 @@ class BaseTestRunner(object):
                    self._http_server.port)
     else:
       logging.critical('Failed to start http server')
-    self.StartForwarderForHttpServer()
+    self.ForwardPortsForHttpServer()
     return (self._forwarder_device_port, self._http_server.port)
 
-  def _ForwardPort(self, port_pairs):
-    """Creates a forwarder instance if needed and forward a port."""
-    if not self._forwarder:
-      self._forwarder = Forwarder(self.adb, self.build_type)
-    self._forwarder.Run(port_pairs, self.tool)
+  def ForwardPorts(self, port_pairs):
+    """Forwards a port."""
+    Forwarder.Map(port_pairs, self.adb, self.build_type, self.tool)

[frankf, 2013/07/10 18:15:38]

Just inline ForwardPorts, UnmapPorts, ForwardPortsForHttpServer?

[Philippe, 2013/07/11 09:34:06]

These methods are exposed to clients and proved to be quite valuable (as an
intermediate layer). For instance we have some code downstream using them (or
StartForwarder() below to be exact) rather than Forwarder directly which makes
any change in Forwarder painless.

What do you think?

 
+  def UnmapPorts(self, port_pairs):
+    """Unmap previously forwarded ports."""
+    for (device_port, _) in port_pairs:
+      Forwarder.UnmapDevicePort(device_port, self.adb)
+
+  # Deprecated: Use ForwardPorts instead.
   def StartForwarder(self, port_pairs):
     """Starts TCP traffic forwarding for the given |port_pairs|.
 
     Args:
       host_port_pairs: A list of (device_port, local_port) tuples to forward.
     """
-    self._ForwardPort(port_pairs)
+    self.ForwardPorts(port_pairs)
 
-  def StartForwarderForHttpServer(self):
+  def ForwardPortsForHttpServer(self):
     """Starts a forwarder for the HTTP server.
 
     The forwarder forwards HTTP requests and responses between host and device.
     """
-    self._ForwardPort([(self._forwarder_device_port, self._http_server.port)])
-
-  def RestartHttpServerForwarderIfNecessary(self):
-    """Restarts the forwarder if it's not open."""
-    # Checks to see if the http server port is being used.  If not forwards the
-    # request.
-    # TODO(dtrainor): This is not always reliable because sometimes the port
-    # will be left open even after the forwarder has been killed.
-    if not ports.IsDevicePortUsed(self.adb,
-        self._forwarder_device_port):
-      self.StartForwarderForHttpServer()
+    self.ForwardPorts([(self._forwarder_device_port, self._http_server.port)])
 
   def ShutdownHelperToolsForTestSuite(self):
     """Shuts down the server and the forwarder."""
-    # Forwarders should be killed before the actual servers they're forwarding
-    # to as they are clients potentially with open connections and to allow for
-    # proper hand-shake/shutdown.
-    Forwarder.KillDevice(self.adb, self.tool)
-    if self._forwarder:
-      self._forwarder.Close()
     if self._http_server:
+      self.UnmapPorts([(self._forwarder_device_port, self._http_server.port)])
       self._http_server.ShutdownHttpServer()
     if self._spawning_server:
       self._spawning_server.Stop()
@@ -208,12 +194,11 @@ class BaseTestRunner(object):
     # Try 3 times to launch test spawner server.
     for i in xrange(0, 3):
       self.test_server_spawner_port = ports.AllocateTestServerPort()
-      self._ForwardPort(
+      self.ForwardPorts(
           [(self.test_server_spawner_port, self.test_server_spawner_port)])
       self._spawning_server = SpawningServer(self.test_server_spawner_port,
                                              self.adb,
                                              self.tool,
-                                             self._forwarder,
                                              self.build_type)
       self._spawning_server.Start()
       server_ready, error_msg = ports.IsHttpServerConnectable(