[Telemetry] Implement CanLaunchApplication and InstallApplication for Android.

[Telemetry] Implement CanLaunchApplication and InstallApplication for Android.

This is in preparation for implementing the Trepn profiler.

BUG=

Committed: https://crrev.com/d52c4a886bbb02a6be4d6009eedacdaf1f84043a
Cr-Commit-Position: refs/heads/master@{#292543}

[tonyg, 2014-08-27 19:29:54]

tonyg@chromium.org changed reviewers:
+ dtu@chromium.org

[tonyg, 2014-08-27 21:52:04]

tonyg@chromium.org changed reviewers:
+ nednguyen@google.com

[nednguyen, 2014-08-28 01:43:03]

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:261:
backend_settings(adb, package),
With this change, can we remove adb from backend_settings? Even better if we can
bury the details of finding devices and adb is used to control it in
android_platform_backend. I think android_platform_backend should be the only
thing that knows about adb.

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:263: if
b.platform.CanLaunchApplication(package) or b.HaveLocalAPK():
Should we do the check "if android_platform.CanLaunchApplication(package):"
before we even create the PossibleAndroidBrowser object?

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

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:189:
parameters = ''
Should we assert self.CanLaunchApplication() here?

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:190:
self._device.RunShellCommand('am start ' + parameters + ' ' + application)
Use 'am start %s %s' % (parameters, application) instead. 

https://wiki.python.org/moin/PythonSpeed/PerformanceTips#String_Concatenation

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:195: def
CanLaunchApplication(self, application):
It's not obvious that application is a string. Can you add some docstring
explains that it's full apk's name?

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:203:
self._device.Install(application)
Does this have a return value? If yes, I think it is nice to return False if
installation fails.

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/profiler/oomkiller_profiler.py
(right):

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/oomkiller_profiler.py:42:
self._platform_backend.InstallApplication(mem_consumer_path)
Should we clean up the installation in WillCloseBrowser? Do we have any policy
about telemtry test leave extra packages on target machines?

[tonyg, 2014-08-28 18:59:14]

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py
(right):

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:261:
backend_settings(adb, package),
On 2014/08/28 01:43:02, nednguyen wrote:
> With this change, can we remove adb from backend_settings? Even better if we
can
> bury the details of finding devices and adb is used to control it in
> android_platform_backend. I think android_platform_backend should be the only
> thing that knows about adb.

Yes, but that'll require some more refactoring. I'd rather keep CLs as small as
possible and this is a step in that direction. Do you think I should add that to
this patch or defer to a subsequent patch?

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/backends/chrome/android_browser_finder.py:263: if
b.platform.CanLaunchApplication(package) or b.HaveLocalAPK():
On 2014/08/28 01:43:02, nednguyen wrote:
> Should we do the check "if android_platform.CanLaunchApplication(package):"
> before we even create the PossibleAndroidBrowser object?

The bit about having the local apk is the wrinkle there. And the PossibleBrowser
conceals a lot of that. I agree it'd be cleaner, but am not sure it is necessary
for now.

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

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:189:
parameters = ''
On 2014/08/28 01:43:02, nednguyen wrote:
> Should we assert self.CanLaunchApplication() here?

Instead of the extra call, I added an exception if it didn't work.

ValueError: Failed to start "does.not.exist" with error
  Error: Activity class {does.not.exist} does not exist.

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:190:
self._device.RunShellCommand('am start ' + parameters + ' ' + application)
On 2014/08/28 01:43:02, nednguyen wrote:
> Use 'am start %s %s' % (parameters, application) instead. 
> 
> https://wiki.python.org/moin/PythonSpeed/PerformanceTips#String_Concatenation

Done.

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:195: def
CanLaunchApplication(self, application):
On 2014/08/28 01:43:02, nednguyen wrote:
> It's not obvious that application is a string. Can you add some docstring
> explains that it's full apk's name?

Done.

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:203:
self._device.Install(application)
On 2014/08/28 01:43:02, nednguyen wrote:
> Does this have a return value? If yes, I think it is nice to return False if
> installation fails.

It raises if installation fails.

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/profiler/oomkiller_profiler.py
(right):

https://codereview.chromium.org/510943002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/oomkiller_profiler.py:42:
self._platform_backend.InstallApplication(mem_consumer_path)
On 2014/08/28 01:43:03, nednguyen wrote:
> Should we clean up the installation in WillCloseBrowser? Do we have any policy
> about telemtry test leave extra packages on target machines?

We typically leave installations around to make subsequent runs faster. Any
settings we change, OTOH, we clean up.

[nednguyen, 2014-08-28 19:38:45]

We can defer the adb refactoring to subsequent patch.

https://codereview.chromium.org/510943002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/510943002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:284: def
ParseCStateSample(sample):
Why is this function added in this patch?

[tonyg, 2014-08-28 21:14:34]

https://codereview.chromium.org/510943002/diff/20001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/510943002/diff/20001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:284: def
ParseCStateSample(sample):
On 2014/08/28 19:38:45, nednguyen wrote:
> Why is this function added in this patch?

It isn't. You're diffing the wrong thing.

It was added in https://codereview.chromium.org/509043002/ . Thus it shows up in
the diff between the first and subsequent version of the patch. But won't show
up if you just diff against ToT (ie. Click the "view" link instead of the number
link).

[nednguyen, 2014-08-28 21:25:39]

On 2014/08/28 21:14:34, tonyg wrote:
>
https://codereview.chromium.org/510943002/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/platform/android_platform_backend.py
> (right):
> 
>
https://codereview.chromium.org/510943002/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/platform/android_platform_backend.py:284: def
> ParseCStateSample(sample):
> On 2014/08/28 19:38:45, nednguyen wrote:
> > Why is this function added in this patch?
> 
> It isn't. You're diffing the wrong thing.
> 
> It was added in https://codereview.chromium.org/509043002/ . Thus it shows up
in
> the diff between the first and subsequent version of the patch. But won't show
> up if you just diff against ToT (ie. Click the "view" link instead of the
number
> link).

Ah, sorry :p

lgtm though

[nednguyen, 2014-08-28 21:25:40]

On 2014/08/28 21:14:34, tonyg wrote:
>
https://codereview.chromium.org/510943002/diff/20001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/platform/android_platform_backend.py
> (right):
> 
>
https://codereview.chromium.org/510943002/diff/20001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/platform/android_platform_backend.py:284: def
> ParseCStateSample(sample):
> On 2014/08/28 19:38:45, nednguyen wrote:
> > Why is this function added in this patch?
> 
> It isn't. You're diffing the wrong thing.
> 
> It was added in https://codereview.chromium.org/509043002/ . Thus it shows up
in
> the diff between the first and subsequent version of the patch. But won't show
> up if you just diff against ToT (ie. Click the "view" link instead of the
number
> link).

Ah, sorry :p

lgtm though

[nednguyen, 2014-08-28 21:55:20]

The CQ bit was checked by nednguyen@google.com

[nednguyen, 2014-08-28 21:55:31]

The CQ bit was unchecked by nednguyen@google.com

[tonyg, 2014-08-28 22:35:38]

The CQ bit was checked by tonyg@chromium.org

[commit-bot: I haz the power, 2014-08-28 22:37:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tonyg@chromium.org/510943002/40001

[commit-bot: I haz the power, 2014-08-29 00:17:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-29 01:47:24]

Committed patchset #3 (id:40001) as 70c332f2911d6b4acc52e9edc0cd887755e09439

[commit-bot: I haz the power, 2014-09-10 03:05:05]

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