[Android] Switch KillAll, StartActivity, and BroadcastIntent to DeviceUtils.

[Android] Switch KillAll, StartActivity, and BroadcastIntent to DeviceUtils.

BUG=267773
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279599

[Victor Starodub, 2014-06-17 18:43:42]

lgtm w/couple of comments

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:348: force_stop=False, timeout=None,
retries=None):
Why do we need both this and previous method? Can't we just ignore the return
value?

https://codereview.chromium.org/338353004/diff/20001/tools/android/adb_profil...
File tools/android/adb_profile_chrome/chrome_controller.py (right):

https://codereview.chromium.org/338353004/diff/20001/tools/android/adb_profil...
tools/android/adb_profile_chrome/chrome_controller.py:66: 'continuous' if
self._ring_buffer else '': None
This one looks a bit ugly.  It's not exactly equivalent to the old code and
relying on an edge case of an empty key IMO is not quite justified here.

[frankf, 2014-06-17 18:45:44]

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:359: The start time of the activity.
can you clarify whether this is device or host time and in wihch format

[Sami, 2014-06-17 20:39:11]

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/andr...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/andr...
build/android/pylib/android_commands.py:781: def KillAllBlocking(self, process,
timeout_sec, signum=9, with_su=False):
nit: use signal.SIGKILL instead of 9.

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:302: def KillAll(self, process_name,
signum=9, root=False, blocking=False,
ditto about signal.SIGKILL.

https://codereview.chromium.org/338353004/diff/20001/tools/android/adb_profil...
File tools/android/adb_profile_chrome/chrome_controller.py (right):

https://codereview.chromium.org/338353004/diff/20001/tools/android/adb_profil...
tools/android/adb_profile_chrome/chrome_controller.py:66: 'continuous' if
self._ring_buffer else '': None
On 2014/06/17 18:43:42, Victor Starodub wrote:
> This one looks a bit ugly.  It's not exactly equivalent to the old code and
> relying on an edge case of an empty key IMO is not quite justified here.

+1. It might be better to construct the Intent first instead of doing everything
inline.

[jbudorick, 2014-06-17 21:09:27]

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/andr...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/andr...
build/android/pylib/android_commands.py:781: def KillAllBlocking(self, process,
timeout_sec, signum=9, with_su=False):
On 2014/06/17 20:39:11, Sami wrote:
> nit: use signal.SIGKILL instead of 9.

Done. (and above in KillAll)

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:302: def KillAll(self, process_name,
signum=9, root=False, blocking=False,
On 2014/06/17 20:39:11, Sami wrote:
> ditto about signal.SIGKILL.

Done.

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:348: force_stop=False, timeout=None,
retries=None):
On 2014/06/17 18:43:42, Victor Starodub wrote:
> Why do we need both this and previous method? Can't we just ignore the return
> value?

This is a good question, and one to which I don't have a good answer. I suspect
that it might be because the clients who don't care about the start time don't
want to do the extra work of determining the start time. I'm not sure that's a
reason to separate the two, though.

That said, _nothing_ calls StartActivityTimed, so I'm just going to remove it
altogether.

https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:359: The start time of the activity.
On 2014/06/17 18:45:44, frankf wrote:
> can you clarify whether this is device or host time and in wihch format

removing this function as noted above

https://codereview.chromium.org/338353004/diff/20001/tools/android/adb_profil...
File tools/android/adb_profile_chrome/chrome_controller.py (right):

https://codereview.chromium.org/338353004/diff/20001/tools/android/adb_profil...
tools/android/adb_profile_chrome/chrome_controller.py:66: 'continuous' if
self._ring_buffer else '': None
On 2014/06/17 20:39:11, Sami wrote:
> On 2014/06/17 18:43:42, Victor Starodub wrote:
> > This one looks a bit ugly.  It's not exactly equivalent to the old code and
> > relying on an edge case of an empty key IMO is not quite justified here.
> 
> +1. It might be better to construct the Intent first instead of doing
everything
> inline.

I'll at least move the creation of the extras dict out.

[frankf, 2014-06-19 17:54:06]

https://codereview.chromium.org/338353004/diff/60001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/60001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:305: """Kill all processes with the
given name on the device.
A general note: if it's not too expensive these methods should indicate whether
an operation was successful (boolean or raising exception as appropriate). In
this case, following linux killall behaviour seems appropriate (if at least one
process is killed, it's successful).

https://codereview.chromium.org/338353004/diff/60001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:311: root: A boolean indicating
whether the kill should be executed with root
I'd rename "root" to "as_root" here and above

https://codereview.chromium.org/338353004/diff/60001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:341:
self.old_interface.StartActivity(
what happens if you fail to start an activity?

[jbudorick, 2014-06-20 00:36:44]

+tonyg for telemetry

https://codereview.chromium.org/338353004/diff/60001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/60001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:305: """Kill all processes with the
given name on the device.
On 2014/06/19 17:54:05, frankf wrote:
> A general note: if it's not too expensive these methods should indicate
whether
> an operation was successful (boolean or raising exception as appropriate). In
> this case, following linux killall behaviour seems appropriate (if at least
one
> process is killed, it's successful).

KillAll now raises a CommandFailedError if no processes were killed.

The couple of places that use it, though, both eat the exception at the moment,
as they both use it as a way of ensuring that no process with the given name
exists.

https://codereview.chromium.org/338353004/diff/60001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:311: root: A boolean indicating
whether the kill should be executed with root
On 2014/06/19 17:54:05, frankf wrote:
> I'd rename "root" to "as_root" here and above

Done.

https://codereview.chromium.org/338353004/diff/60001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:341:
self.old_interface.StartActivity(
On 2014/06/19 17:54:05, frankf wrote:
> what happens if you fail to start an activity?

It was doing the same thing as AndroidCommands.StartActivity, which is to say,
nothing.

It now raises a CommandFailedError if it sees a line start with "Error: ", which
appears to be the only consistent way to tell that 'am start' failed.

[frankf, 2014-06-20 00:56:56]

https://codereview.chromium.org/338353004/diff/80001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/80001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:309: 
Update the docstring to include the exception

https://codereview.chromium.org/338353004/diff/80001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:324: ['adb', 'shell', 'kill'], 'No
process "%s"' % process_name)
Why does this include such a command list? You haven't run adb shell kill at
this point. Can you use another exception type that doesn't require it?

https://codereview.chromium.org/338353004/diff/80001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:361: ['adb', 'shell', 'am', 'start'],
l)
same here

[tonyg, 2014-06-20 16:52:47]

lgtm

[jbudorick, 2014-06-23 16:44:03]

https://codereview.chromium.org/338353004/diff/80001/build/android/pylib/devi...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/80001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:309: 
On 2014/06/20 00:56:56, frankf wrote:
> Update the docstring to include the exception

Done.

https://codereview.chromium.org/338353004/diff/80001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:324: ['adb', 'shell', 'kill'], 'No
process "%s"' % process_name)
On 2014/06/20 00:56:55, frankf wrote:
> Why does this include such a command list? You haven't run adb shell kill at
> this point. Can you use another exception type that doesn't require it?

Created a new AdbCommandFailedError that derives from CommandFailedError.
AdbCommandFailedError requires the command list and is now largely raised in
adb_wrapper.py, while CommandFailedError does not require the command list and
is largely raised in device_utils.py.

https://codereview.chromium.org/338353004/diff/80001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:361: ['adb', 'shell', 'am', 'start'],
l)
On 2014/06/20 00:56:55, frankf wrote:
> same here

using the reworked CommandFailedError, as described above.

[frankf, 2014-06-23 18:13:36]

device_utils.py lgtm

https://codereview.chromium.org/338353004/diff/100001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/100001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:354: retries: Same as for |IsOnline|.
add Raises

[jbudorick, 2014-06-24 19:38:26]

https://codereview.chromium.org/338353004/diff/100001/build/android/pylib/dev...
File build/android/pylib/device/device_utils.py (right):

https://codereview.chromium.org/338353004/diff/100001/build/android/pylib/dev...
build/android/pylib/device/device_utils.py:354: retries: Same as for |IsOnline|.
On 2014/06/23 18:13:36, frankf wrote:
> add Raises

Done.

[jbudorick, 2014-06-24 22:12:22]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-06-24 22:13:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/338353004/140001

[jbudorick, 2014-06-24 23:26:43]

The CQ bit was unchecked by jbudorick@chromium.org

[jbudorick, 2014-06-24 23:36:56]

On 2014/06/17 21:09:27, jbudorick wrote:
>
https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/andr...
> File build/android/pylib/android_commands.py (right):
> 
>
https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/andr...
> build/android/pylib/android_commands.py:781: def KillAllBlocking(self,
process,
> timeout_sec, signum=9, with_su=False):
> On 2014/06/17 20:39:11, Sami wrote:
> > nit: use signal.SIGKILL instead of 9.
> 
> Done. (and above in KillAll)
> 

signal.SIGKILL does not exist on Windows, so using it broke telemetry on the
windows bots. Reverted to 9.

>
https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
> File build/android/pylib/device/device_utils.py (right):
> 
>
https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
> build/android/pylib/device/device_utils.py:302: def KillAll(self,
process_name,
> signum=9, root=False, blocking=False,
> On 2014/06/17 20:39:11, Sami wrote:
> > ditto about signal.SIGKILL.
> 
> Done.
> 
>
https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
> build/android/pylib/device/device_utils.py:348: force_stop=False,
timeout=None,
> retries=None):
> On 2014/06/17 18:43:42, Victor Starodub wrote:
> > Why do we need both this and previous method? Can't we just ignore the
return
> > value?
> 
> This is a good question, and one to which I don't have a good answer. I
suspect
> that it might be because the clients who don't care about the start time don't
> want to do the extra work of determining the start time. I'm not sure that's a
> reason to separate the two, though.
> 
> That said, _nothing_ calls StartActivityTimed, so I'm just going to remove it
> altogether.
> 
>
https://codereview.chromium.org/338353004/diff/20001/build/android/pylib/devi...
> build/android/pylib/device/device_utils.py:359: The start time of the
activity.
> On 2014/06/17 18:45:44, frankf wrote:
> > can you clarify whether this is device or host time and in wihch format
> 
> removing this function as noted above
> 
>
https://codereview.chromium.org/338353004/diff/20001/tools/android/adb_profil...
> File tools/android/adb_profile_chrome/chrome_controller.py (right):
> 
>
https://codereview.chromium.org/338353004/diff/20001/tools/android/adb_profil...
> tools/android/adb_profile_chrome/chrome_controller.py:66: 'continuous' if
> self._ring_buffer else '': None
> On 2014/06/17 20:39:11, Sami wrote:
> > On 2014/06/17 18:43:42, Victor Starodub wrote:
> > > This one looks a bit ugly.  It's not exactly equivalent to the old code
and
> > > relying on an edge case of an empty key IMO is not quite justified here.
> > 
> > +1. It might be better to construct the Intent first instead of doing
> everything
> > inline.
> 
> I'll at least move the creation of the extras dict out.

[jbudorick, 2014-06-24 23:37:44]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-06-24 23:39:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jbudorick@chromium.org/338353004/160001

[commit-bot: I haz the power, 2014-06-25 05:09:12]

Change committed as 279599