[Android] Switch to DeviceUtils versions of GetPid, TakeScreenshot, and GetIoStats.

[Android] Switch to DeviceUtils versions of GetPid, TakeScreenshot, and GetIoStats.

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

[tonyg, 2014-07-11 20:16:32]

lgtm

[frankf, 2014-07-11 20:22:53]

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

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:604: def GetPid(self, process_name,
timeout=None, retries=None):
this should be plural to match the return value

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:611: timeout: Same as for |IsOnline|.
I'd change all these to:

timeout: timeout in <some unit>
retries: number of retries

same amount of duplication plus it's self-contained

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:614: A list containing the PIDs of
processes with the given name.
are there any assumptions on the ordering here? For example for Chrome, does it
list the browser process first?

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:615: """
you should add raises section to all methods

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:624: screenshot to. If None, a file
name will be generated.
is this a temp file?

[frankf, 2014-07-11 20:29:07]

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

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:607: Note that the |process_name| is
often the package name.
Also isn't this the app package name and not the process name?

[jbudorick, 2014-07-14 16:15:11]

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

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:604: def GetPid(self, process_name,
timeout=None, retries=None):
On 2014/07/11 20:22:53, frankf wrote:
> this should be plural to match the return value

Done.

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:607: Note that the |process_name| is
often the package name.
On 2014/07/11 20:29:07, frankf wrote:
> Also isn't this the app package name and not the process name?

Not necessarily, though it can be, e.g. these can both appear in the name field
of a ps line:

  /data/local/tmp/forwarder/device_forwarder
  com.google.android.apps.chrome

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:611: timeout: Same as for |IsOnline|.
On 2014/07/11 20:22:53, frankf wrote:
> I'd change all these to:
> 
> timeout: timeout in <some unit>
> retries: number of retries
> 
> same amount of duplication plus it's self-contained

I don't really have an objection to this, but...

https://codereview.chromium.org/285143002/#msg14

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:614: A list containing the PIDs of
processes with the given name.
On 2014/07/11 20:22:53, frankf wrote:
> are there any assumptions on the ordering here? For example for Chrome, does
it
> list the browser process first?

I don't think that the interface should guarantee ordering, but I'm open to
arguments to the contrary.

Currently, AndroidCommands puts exact matches at the front of the list. Other
matches are added in the order in which they appear in the output of ps, which
appears to be in numerical order by PID.

As for browser vs renderer processes, no, there are definitely no guarantees
there as far as the scripts are concerned.

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:615: """
On 2014/07/11 20:22:53, frankf wrote:
> you should add raises section to all methods

Done.

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:624: screenshot to. If None, a file
name will be generated.
On 2014/07/11 20:22:53, frankf wrote:
> is this a temp file?

No.

The AndroidCommands implementation uses ${PWD}/screenshot-${timestamp}.png when
it generates a file name. That is not guaranteed to stay the same in the impl
change, but it won't be a temp file.

[frankf, 2014-07-14 17:04:10]

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

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:611: timeout: Same as for |IsOnline|.
Yea, the spirit of that comment was to cut down on boilerplate, but you still
have boiler plate so you might as well have the actual arg description. If
someone is using generated pydoc, then it's easier for them.

On 2014/07/14 16:15:11, jbudorick wrote:
> On 2014/07/11 20:22:53, frankf wrote:
> > I'd change all these to:
> > 
> > timeout: timeout in <some unit>
> > retries: number of retries
> > 
> > same amount of duplication plus it's self-contained
> 
> I don't really have an objection to this, but...
> 
> https://codereview.chromium.org/285143002/#msg14

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:614: A list containing the PIDs of
processes with the given name.
For example, in monkey test runner, we take the first pid as the browser
process, and there might be other instances. So either provide the guarantee or
come up with a mechanism to get just the browser process and not renderers.

On 2014/07/14 16:15:11, jbudorick wrote:
> On 2014/07/11 20:22:53, frankf wrote:
> > are there any assumptions on the ordering here? For example for Chrome, does
> it
> > list the browser process first?
> 
> I don't think that the interface should guarantee ordering, but I'm open to
> arguments to the contrary.
> 
> Currently, AndroidCommands puts exact matches at the front of the list. Other
> matches are added in the order in which they appear in the output of ps, which
> appears to be in numerical order by PID.
> 
> As for browser vs renderer processes, no, there are definitely no guarantees
> there as far as the scripts are concerned.

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:624: screenshot to. If None, a file
name will be generated.
So my point was: is the user expected to clean this up? I guess in general it's
good to delete the file even if it's in /tmp...

On 2014/07/14 16:15:11, jbudorick wrote:
> On 2014/07/11 20:22:53, frankf wrote:
> > is this a temp file?
> 
> No.
> 
> The AndroidCommands implementation uses ${PWD}/screenshot-${timestamp}.png
when
> it generates a file name. That is not guaranteed to stay the same in the impl
> change, but it won't be a temp file.

[jbudorick, 2014-07-15 15:52:57]

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

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:611: timeout: Same as for |IsOnline|.
On 2014/07/14 17:04:10, frankf wrote:
> Yea, the spirit of that comment was to cut down on boilerplate, but you still
> have boiler plate so you might as well have the actual arg description. If
> someone is using generated pydoc, then it's easier for them.
> 
> On 2014/07/14 16:15:11, jbudorick wrote:
> > On 2014/07/11 20:22:53, frankf wrote:
> > > I'd change all these to:
> > > 
> > > timeout: timeout in <some unit>
> > > retries: number of retries
> > > 
> > > same amount of duplication plus it's self-contained
> > 
> > I don't really have an objection to this, but...
> > 
> > https://codereview.chromium.org/285143002/#msg14
> 

Makes sense.

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:614: A list containing the PIDs of
processes with the given name.
On 2014/07/14 17:04:10, frankf wrote:
> For example, in monkey test runner, we take the first pid as the browser
> process, and there might be other instances. So either provide the guarantee
or
> come up with a mechanism to get just the browser process and not renderers.
> 
> On 2014/07/14 16:15:11, jbudorick wrote:
> > On 2014/07/11 20:22:53, frankf wrote:
> > > are there any assumptions on the ordering here? For example for Chrome,
does
> > it
> > > list the browser process first?
> > 
> > I don't think that the interface should guarantee ordering, but I'm open to
> > arguments to the contrary.
> > 
> > Currently, AndroidCommands puts exact matches at the front of the list.
Other
> > matches are added in the order in which they appear in the output of ps,
which
> > appears to be in numerical order by PID.
> > 
> > As for browser vs renderer processes, no, there are definitely no guarantees
> > there as far as the scripts are concerned.
> 

Ah, ok.

Switched to having GetPids return a dict mapping process name to PID.

https://codereview.chromium.org/386053002/diff/20001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:624: screenshot to. If None, a file
name will be generated.
On 2014/07/14 17:04:10, frankf wrote:
> So my point was: is the user expected to clean this up? I guess in general
it's
> good to delete the file even if it's in /tmp...
> 
> On 2014/07/14 16:15:11, jbudorick wrote:
> > On 2014/07/11 20:22:53, frankf wrote:
> > > is this a temp file?
> > 
> > No.
> > 
> > The AndroidCommands implementation uses ${PWD}/screenshot-${timestamp}.png
> when
> > it generates a file name. That is not guaranteed to stay the same in the
impl
> > change, but it won't be a temp file.
> 

Yes, the user is expected to clean up the screenshot file on the host if they
don't want it (although if they don't want the screenshot file, there's really
no point in calling this function).

https://codereview.chromium.org/386053002/diff/60001/build/android/pylib/scre...
File build/android/pylib/screenshot.py (right):

https://codereview.chromium.org/386053002/diff/60001/build/android/pylib/scre...
build/android/pylib/screenshot.py:77: def Pull(self, host_file=None):
Changes in here are to fix issues introduced in
https://codereview.chromium.org/379443002/

[frankf, 2014-07-15 17:27:06]

lgtm after fix

https://codereview.chromium.org/386053002/diff/60001/build/android/pylib/monk...
File build/android/pylib/monkey/test_runner.py (right):

https://codereview.chromium.org/386053002/diff/60001/build/android/pylib/monk...
build/android/pylib/monkey/test_runner.py:70: if not before_pids or not
self._package in before_pids:
can't you shorten ths to:
if self_package not in before_pids

same below

[frankf, 2014-07-15 17:28:44]

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

https://codereview.chromium.org/386053002/diff/60001/build/android/pylib/devi...
build/android/pylib/device/device_utils.py:108: def _HasRootImpl(self):
fyi, you don't have to provide docstring for private methods (unless you really
want to).

[jbudorick, 2014-07-16 01:08:52]

https://codereview.chromium.org/386053002/diff/60001/build/android/pylib/monk...
File build/android/pylib/monkey/test_runner.py (right):

https://codereview.chromium.org/386053002/diff/60001/build/android/pylib/monk...
build/android/pylib/monkey/test_runner.py:70: if not before_pids or not
self._package in before_pids:
On 2014/07/15 17:27:05, frankf wrote:
> can't you shorten ths to:
> if self_package not in before_pids
> 
> same below

Done.

[jbudorick, 2014-07-16 01:08:58]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-07-16 01:11:49]

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

[commit-bot: I haz the power, 2014-07-16 09:39:49]

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

[commit-bot: I haz the power, 2014-07-16 09:44:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-16 09:44:25]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[jbudorick, 2014-07-16 14:25:19]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-07-16 14:29:03]

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

[commit-bot: I haz the power, 2014-07-16 18:01:31]

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

[commit-bot: I haz the power, 2014-07-16 18:11:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-16 18:11:20]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[jbudorick, 2014-07-16 18:12:11]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-07-16 18:16:09]

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

[commit-bot: I haz the power, 2014-07-17 09:54:11]

Change committed as 283731