[Android] Remove android_commands uses from build/android/.

build/android/pylib/device/adb_wrapper.py

Index: build/android/pylib/device/adb_wrapper.py
diff --git a/build/android/pylib/device/adb_wrapper.py b/build/android/pylib/device/adb_wrapper.py
index 36f8f484b00fbe2ec05c4c440ab13af10dde71f2..500e3a2eb332b6c334d17d0f43be653fc8d532d3 100644
--- a/build/android/pylib/device/adb_wrapper.py
+++ b/build/android/pylib/device/adb_wrapper.py
@@ -17,6 +17,7 @@ from pylib import cmd_helper
 from pylib import constants
 from pylib.device import decorators
 from pylib.device import device_errors
+from pylib.device import device_filter
 from pylib.utils import timeout_retry
 
 
@@ -158,22 +159,33 @@ class AdbWrapper(object):
     cls._RunAdbCmd(['start-server'], timeout=timeout, retries=retries,
                    cpu_affinity=0)
 
-  # TODO(craigdh): Determine the filter criteria that should be supported.
   @classmethod
-  def GetDevices(cls, timeout=_DEFAULT_TIMEOUT, retries=_DEFAULT_RETRIES):
+  def GetDevices(cls, filters=None, timeout=_DEFAULT_TIMEOUT,
+                 retries=_DEFAULT_RETRIES):
     """Get the list of active attached devices.
 
     Args:
+      filters: (optional) A list of binary functions that take an AdbWrapper
+          instance and a string description. Any device for which all provided
+          filter functions do not return True will not be included in the
+          returned list.

[perezju, 2015/04/14 12:39:10]

I feel this is a bit over-complicated, I would either:

1. take as filter a single function, f(adb, desc) -> bool, and filter using
that; or even better

2. do not provide the option at all, store the description string into the adb
object, and let the caller do the filtering itself.

[jbudorick, 2015/04/14 15:29:58]

after looking at this, I think we already have this info available via
AdbWrapper.GetState(), so I can eliminate the description.

I've gone back and forth on this. We _already_ filter on state == 'device', and
very, very few clients want blacklisted devices. Passing back a list of
everything is not what we want to do.

[perezju, 2015/04/15 09:25:44]

I'm not sure about this. AdbWrapper is meant to be "a thin wrapper around the
adb interface", and having knowledge about a blacklist file on some particular
location of a chrome checkout seems to go a bit too far.

Also, as implemented, it seems that this would silently skip devices not
matching the filter (which may or may not be what we want on different
scenarios).

Having some ready made filters sounds like a great idea, but I would pass the
responsibility of what filters to use to the caller. Also, when developing or
testing things locally, the blacklist is kind of annoying, I had to disable it
once when experimenting with provision devices for the android test.

Just my final 2 cents, but I'm ok with whatever you decide in the end.

(Also if you decide to keep this, the docstring should document the default
behavior.)

[jbudorick, 2015/04/15 13:06:59]

Yeah, I thought about this too and am still not sure. I'll try it with no
default filters and see how it looks.

I suspect the implementation of the blacklist will have to change when we
extract it anyway, so think of it less in terms of this knowing about the
current implementation of the blacklist and more about it knowing about the
blacklist at all.
It does, and that is bad.
wit the current implementation, just rm -rf out/bad_devices.json

but yes, I've hit the same thing.

       timeout: (optional) Timeout per try in seconds.
       retries: (optional) Number of retries to attempt.
 
     Yields:
       AdbWrapper instances.
     """
+    if filters is None:
+      filters = device_filter.DefaultFilters()
+
     output = cls._RunAdbCmd(['devices'], timeout=timeout, retries=retries)
     lines = [line.split() for line in output.splitlines()]
-    return [AdbWrapper(line[0]) for line in lines
-            if len(line) == 2 and line[1] == 'device']
+
+    devices_and_descriptions = [
+        (AdbWrapper(line[0]), line[1]) for line in lines if len(line) == 2]
+
+    return [dev for dev, desc in devices_and_descriptions
+            if all(f(dev, desc) for f in filters)]
 
   def GetDeviceSerial(self):
     """Gets the device serial number associated with this object.