[Android] Remove more references to and uses of AndroidCommands.

[Android] Remove more references to and uses of AndroidCommands.

This includes removing most direct references and uses from telemetry.

BUG=267773, 476709
Committed: https://crrev.com/ac496302b1d0ada0bbe8ef7beecfdff67d480c6b
Cr-Commit-Position: refs/heads/master@{#329963}

[jbudorick, 2015-05-12 17:25:12]

jbudorick@chromium.org changed reviewers:
+ perezju@chromium.org, skyostil@google.com, sullivan@chromium.org

[jbudorick, 2015-05-12 17:25:14]

I'm going to exorcise adb_commands in a separate CL.

[sullivan, 2015-05-12 18:12:30]

lgtm

Thanks so much for doing this!

Should probably give the perf sheriff a heads up after submitting.

[jbudorick, 2015-05-12 18:13:25]

On 2015/05/12 18:12:30, sullivan wrote:
> lgtm
> 
> Thanks so much for doing this!
> 
> Should probably give the perf sheriff a heads up after submitting.

Will do. It probably won't be today.

[Sami, 2015-05-12 18:37:09]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2015-05-12 18:37:10]

Great to see this going away :)

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/contro...
File tools/profile_chrome/controllers_unittest.py (right):

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/contro...
tools/profile_chrome/controllers_unittest.py:18: self.device =
device_utils.DeviceUtils(devices[0])
Should the DeviceUtils part be removed here?

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/main.py
File tools/profile_chrome/main.py (right):

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/main.p...
tools/profile_chrome/main.py:175: device = options.device
This is supposed to select a device based on serial # but I'm not sure how that
should work in the new system. Maybe we need to iterate over the available
devices and pick the matching one?

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
File tools/profile_chrome/perf_controller.py (right):

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
tools/profile_chrome/perf_controller.py:53: self._device.RunShellCommand(
This command should still be run asynchronously by the Popen below.

https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:108: def
__del__(self):
This seems a little risky since the finalizer can run at a pretty arbitrary file
-- including when we're already doing something completely different with the
same device.

Maybe we should just save the copy script with constant name and leave it on the
device?

[nednguyen, 2015-05-12 19:52:21]

On 2015/05/12 18:37:10, Sami wrote:
> Great to see this going away :)
> 
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/contro...
> File tools/profile_chrome/controllers_unittest.py (right):
> 
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/contro...
> tools/profile_chrome/controllers_unittest.py:18: self.device =
> device_utils.DeviceUtils(devices[0])
> Should the DeviceUtils part be removed here?
> 
> https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/main.py
> File tools/profile_chrome/main.py (right):
> 
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/main.p...
> tools/profile_chrome/main.py:175: device = options.device
> This is supposed to select a device based on serial # but I'm not sure how
that
> should work in the new system. Maybe we need to iterate over the available
> devices and pick the matching one?
> 
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
> File tools/profile_chrome/perf_controller.py (right):
> 
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
> tools/profile_chrome/perf_controller.py:53: self._device.RunShellCommand(
> This command should still be run asynchronously by the Popen below.
> 
>
https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
> File tools/telemetry/telemetry/core/platform/android_platform_backend.py
> (right):
> 
>
https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
> tools/telemetry/telemetry/core/platform/android_platform_backend.py:108: def
> __del__(self):
> This seems a little risky since the finalizer can run at a pretty arbitrary
file
> -- including when we're already doing something completely different with the
> same device.
> 
> Maybe we should just save the copy script with constant name and leave it on
the
> device?

Thanks so much John!

Does thisalso  allow us to shrink down few dependencies in
telemetry/telemetry/TELEMETRY_DEPS?

[jbudorick, 2015-05-12 20:30:44]

On 2015/05/12 19:52:21, nednguyen (slow review) wrote:
> On 2015/05/12 18:37:10, Sami wrote:
> > Great to see this going away :)
> > 
> >
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/contro...
> > File tools/profile_chrome/controllers_unittest.py (right):
> > 
> >
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/contro...
> > tools/profile_chrome/controllers_unittest.py:18: self.device =
> > device_utils.DeviceUtils(devices[0])
> > Should the DeviceUtils part be removed here?
> > 
> >
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/main.py
> > File tools/profile_chrome/main.py (right):
> > 
> >
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/main.p...
> > tools/profile_chrome/main.py:175: device = options.device
> > This is supposed to select a device based on serial # but I'm not sure how
> that
> > should work in the new system. Maybe we need to iterate over the available
> > devices and pick the matching one?
> > 
> >
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
> > File tools/profile_chrome/perf_controller.py (right):
> > 
> >
>
https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
> > tools/profile_chrome/perf_controller.py:53: self._device.RunShellCommand(
> > This command should still be run asynchronously by the Popen below.
> > 
> >
>
https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
> > File tools/telemetry/telemetry/core/platform/android_platform_backend.py
> > (right):
> > 
> >
>
https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
> > tools/telemetry/telemetry/core/platform/android_platform_backend.py:108: def
> > __del__(self):
> > This seems a little risky since the finalizer can run at a pretty arbitrary
> file
> > -- including when we're already doing something completely different with
the
> > same device.
> > 
> > Maybe we should just save the copy script with constant name and leave it on
> the
> > device?
> 
> Thanks so much John!
> 
> Does thisalso  allow us to shrink down few dependencies in
> telemetry/telemetry/TELEMETRY_DEPS?

not yet, there are still some things to resolve with android_commands and
third_party/android_testrunner, but that's what this is working towards.

[jbudorick, 2015-05-13 03:21:22]

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/contro...
File tools/profile_chrome/controllers_unittest.py (right):

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/contro...
tools/profile_chrome/controllers_unittest.py:18: self.device =
device_utils.DeviceUtils(devices[0])
On 2015/05/12 18:37:09, Sami wrote:
> Should the DeviceUtils part be removed here?

Yes, done.

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/main.py
File tools/profile_chrome/main.py (right):

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/main.p...
tools/profile_chrome/main.py:175: device = options.device
On 2015/05/12 18:37:09, Sami wrote:
> This is supposed to select a device based on serial # but I'm not sure how
that
> should work in the new system. Maybe we need to iterate over the available
> devices and pick the matching one?

Yeah, this is actually a mistake I've made elsewhere, too. Fixed here and in the
other locations.

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
File tools/profile_chrome/perf_controller.py (right):

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
tools/profile_chrome/perf_controller.py:53: self._device.RunShellCommand(
On 2015/05/12 18:37:10, Sami wrote:
> This command should still be run asynchronously by the Popen below.

Ah, that's the part I missed. Reverted.

Also, this is gross, though fixing it is (thankfully) noit required to ditch
android_commands.

https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:108: def
__del__(self):
On 2015/05/12 18:37:10, Sami wrote:
> This seems a little risky since the finalizer can run at a pretty arbitrary
file
> -- including when we're already doing something completely different with the
> same device.

I'm not sure what you mean by this. Can you elaborate?

> 
> Maybe we should just save the copy script with constant name and leave it on
the
> device?

I guess we could, though I don't think this will be that risky -- each instance
of AndroidPlatformBackend will have its own instance of the temp file, so
removing it shouldn't affect other instances of AndroidPlatformBackend.

[Sami, 2015-05-13 12:58:51]

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
File tools/profile_chrome/perf_controller.py (right):

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
tools/profile_chrome/perf_controller.py:53: self._device.RunShellCommand(
On 2015/05/13 at 03:21:22, jbudorick wrote:
> On 2015/05/12 18:37:10, Sami wrote:
> > This command should still be run asynchronously by the Popen below.
> 
> Ah, that's the part I missed. Reverted.
> 
> Also, this is gross, though fixing it is (thankfully) noit required to ditch
android_commands.

Yeah this is pretty bad. Maybe there's a cleaner API we could add to DeviceUtils
(in another patch)?

https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:108: def
__del__(self):
On 2015/05/13 at 03:21:22, jbudorick wrote:
> On 2015/05/12 18:37:10, Sami wrote:
> > This seems a little risky since the finalizer can run at a pretty arbitrary
file
> > -- including when we're already doing something completely different with
the
> > same device.
> 
> I'm not sure what you mean by this. Can you elaborate?

I meant that it's not at all deterministic if or when the finalizer will run. We
may have killed the adb server by that time or the device may have gone away.
That could lead to random breakage if the closing fails for some reason.

If we do decide to do this in the destructor, I think it should be
DeviceTempFile's __del__ that closes the file like with regular files.

> > 
> > Maybe we should just save the copy script with constant name and leave it on
the
> > device?
> 
> I guess we could, though I don't think this will be that risky -- each
instance of AndroidPlatformBackend will have its own instance of the temp file,
so removing it shouldn't affect other instances of AndroidPlatformBackend.

Right, my main concern was about the close() function failing in unpredictable
ways. I think having a static name would still be cleaner.

https://codereview.chromium.org/1132993004/diff/40001/build/android/adb_rever...
File build/android/adb_reverse_forwarder.py (right):

https://codereview.chromium.org/1132993004/diff/40001/build/android/adb_rever...
build/android/adb_reverse_forwarder.py:57: devices =
device_utils.DeviceUtils.HealthyDevices()
Just a thought: what if HealthyDevices (or some new method) took an optional
serial # as a parameter to reduce the duplication a bit? I'm happy with the
current approach too.

https://codereview.chromium.org/1132993004/diff/40001/tools/profile_chrome/pe...
File tools/profile_chrome/perf_controller.py (right):

https://codereview.chromium.org/1132993004/diff/40001/tools/profile_chrome/pe...
tools/profile_chrome/perf_controller.py:50: self._output_file =
device_temp_file.DeviceTempFile(
Missing import?

[jbudorick, 2015-05-13 21:49:29]

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
File tools/profile_chrome/perf_controller.py (right):

https://codereview.chromium.org/1132993004/diff/1/tools/profile_chrome/perf_c...
tools/profile_chrome/perf_controller.py:53: self._device.RunShellCommand(
On 2015/05/13 12:58:50, Sami wrote:
> On 2015/05/13 at 03:21:22, jbudorick wrote:
> > On 2015/05/12 18:37:10, Sami wrote:
> > > This command should still be run asynchronously by the Popen below.
> > 
> > Ah, that's the part I missed. Reverted.
> > 
> > Also, this is gross, though fixing it is (thankfully) noit required to ditch
> android_commands.
> 
> Yeah this is pretty bad. Maybe there's a cleaner API we could add to
DeviceUtils
> (in another patch)?

Perhaps. Both telemetry and the video recorder both do this handrolled adb +
Popen shenanigans. That would certainly be in a subsequent patch.

https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/1132993004/diff/1/tools/telemetry/telemetry/c...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:108: def
__del__(self):
On 2015/05/13 12:58:51, Sami wrote:
> On 2015/05/13 at 03:21:22, jbudorick wrote:
> > On 2015/05/12 18:37:10, Sami wrote:
> > > This seems a little risky since the finalizer can run at a pretty
arbitrary
> file
> > > -- including when we're already doing something completely different with
> the
> > > same device.
> > 
> > I'm not sure what you mean by this. Can you elaborate?
> 
> I meant that it's not at all deterministic if or when the finalizer will run.
We
> may have killed the adb server by that time or the device may have gone away.
> That could lead to random breakage if the closing fails for some reason.

Sometimes I miss c++.

> 
> If we do decide to do this in the destructor, I think it should be
> DeviceTempFile's __del__ that closes the file like with regular files.
> 
> > > 
> > > Maybe we should just save the copy script with constant name and leave it
on
> the
> > > device?
> > 
> > I guess we could, though I don't think this will be that risky -- each
> instance of AndroidPlatformBackend will have its own instance of the temp
file,
> so removing it shouldn't affect other instances of AndroidPlatformBackend.
> 
> Right, my main concern was about the close() function failing in unpredictable
> ways. I think having a static name would still be cleaner.

Switching to a file that we leave on the device for now.

The other thing we could do is just have this eat any exceptions that get
thrown.

https://codereview.chromium.org/1132993004/diff/40001/build/android/adb_rever...
File build/android/adb_reverse_forwarder.py (right):

https://codereview.chromium.org/1132993004/diff/40001/build/android/adb_rever...
build/android/adb_reverse_forwarder.py:57: devices =
device_utils.DeviceUtils.HealthyDevices()
On 2015/05/13 12:58:51, Sami wrote:
> Just a thought: what if HealthyDevices (or some new method) took an optional
> serial # as a parameter to reduce the duplication a bit? I'm happy with the
> current approach too.

I tried that out, but I wasn't crazy about the interface.

https://codereview.chromium.org/1132993004/diff/40001/tools/profile_chrome/pe...
File tools/profile_chrome/perf_controller.py (right):

https://codereview.chromium.org/1132993004/diff/40001/tools/profile_chrome/pe...
tools/profile_chrome/perf_controller.py:50: self._output_file =
device_temp_file.DeviceTempFile(
On 2015/05/13 12:58:51, Sami wrote:
> Missing import?

Done.

[Sami, 2015-05-14 10:38:52]

Thanks! lgtm.

https://codereview.chromium.org/1132993004/diff/60001/chrome/test/chromedrive...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/1132993004/diff/60001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:1071: for x in
device_utils.DeviceUtils.HealthyDevices()]
nit: _ instead of x.

[jbudorick, 2015-05-14 20:17:10]

jbudorick@chromium.org changed reviewers:
+ samuong@chromium.org

[jbudorick, 2015-05-14 20:17:11]

+Sam for chrome/test/chromedriver owners

https://codereview.chromium.org/1132993004/diff/60001/chrome/test/chromedrive...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/1132993004/diff/60001/chrome/test/chromedrive...
chrome/test/chromedriver/test/run_py_tests.py:1071: for x in
device_utils.DeviceUtils.HealthyDevices()]
On 2015/05/14 10:38:51, Sami wrote:
> nit: _ instead of x.

Done.

[samuong, 2015-05-14 20:18:48]

lgtm for chromedriver

[jbudorick, 2015-05-14 21:58:53]

The CQ bit was checked by jbudorick@chromium.org

[jbudorick, 2015-05-14 21:58:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
sullivan@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1132993004/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-05-14 21:59:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1132993004/80001

[commit-bot: I haz the power, 2015-05-14 22:52:35]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-14 22:53:29]

Patchset 5 (id:??) landed as
https://crrev.com/ac496302b1d0ada0bbe8ef7beecfdff67d480c6b
Cr-Commit-Position: refs/heads/master@{#329963}