android: Reimplement adb_profile_chrome in Python

android: Reimplement adb_profile_chrome in Python

adb_profile_chrome is used to capture and download traces from Chrome on
Android. To prepare for future feature improvements and reduce the
maintenance burden, rewrite this bash script in Python.

This reimplementation keeps all the features and command line options of
the original and adds these new ones:

  --trace-cc          Enable extra trace categories for compositor frame
                      viewer data.
  --trace-gpu         Enable extra trace categories for GPU data.
  -z, --compress      Compress the resulting trace with gzip.

BUG=299822, 294883
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226259

[bulach, 2013-10-01 10:07:06]

lgtm, nice!!

just nits and small suggestions:

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
File build/android/adb_profile_chrome.py (right):

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:20: SUPPORTED_VERSIONS = {
nit: could use a "_" prefix for everything except main() to indicate nothing is
"exported"..

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:85: with open(host_file) as input_file:
nit: maybe open(host_file, 'rb')

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:102: print 'done'
nit: s/print/PrintHeading all over?

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:111: PrintHeading('Capturing %d second
trace...' % interval)
ultra nit: second(s)

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:113: time.sleep(interval)
how about something like:

try:
  PrintHeading('Ctrl+C to capture now...')
  time.sleep(interval)
except KeyboardInterrupt:
  PrintHeading('Capturing shorter trace...')

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:138: manual_options =
optparse.OptionGroup(parser, "Manual tracing")
nit:s/"/'/g

(on 147 and 153 too)

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:169: parser.add_option('-v', '--version',
help='Select among installed browsers. '
since it's a new script, can we break old habits? :)

-b/--browser would be more compatible with telemetry, and less problematic with
--verbose, wdyt?

https://codereview.chromium.org/25044004/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/25044004/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:735: cmd = 'am broadcast -a %s.%s ' %
(package, intent)
nit: add a %s and , ' '.join(args) right here?

[Sami, 2013-10-01 13:32:32]

Thanks, all nits addressed.

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
File build/android/adb_profile_chrome.py (right):

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:20: SUPPORTED_VERSIONS = {
On 2013/10/01 10:07:06, bulach wrote:
> nit: could use a "_" prefix for everything except main() to indicate nothing
is
> "exported"..

Good idea, done.

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:102: print 'done'
On 2013/10/01 10:07:06, bulach wrote:
> nit: s/print/PrintHeading all over?

Done.

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:111: PrintHeading('Capturing %d second
trace...' % interval)
On 2013/10/01 10:07:06, bulach wrote:
> ultra nit: second(s)

Mr. Tom "native speaker" Hudson says my version is right but needs a hyphen
between the number and "second" :)

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:113: time.sleep(interval)
On 2013/10/01 10:07:06, bulach wrote:
> how about something like:
> 
> try:
>   PrintHeading('Ctrl+C to capture now...')
>   time.sleep(interval)
> except KeyboardInterrupt:
>   PrintHeading('Capturing shorter trace...')

Ooh, I like that idea.

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:138: manual_options =
optparse.OptionGroup(parser, "Manual tracing")
On 2013/10/01 10:07:06, bulach wrote:
> nit:s/"/'/g
> 
> (on 147 and 153 too)

Some day I will learn...

https://codereview.chromium.org/25044004/diff/1/build/android/adb_profile_chr...
build/android/adb_profile_chrome.py:169: parser.add_option('-v', '--version',
help='Select among installed browsers. '
On 2013/10/01 10:07:06, bulach wrote:
> since it's a new script, can we break old habits? :)
> 
> -b/--browser would be more compatible with telemetry, and less problematic
with
> --verbose, wdyt?

Yeah, I don't like --version much but wanted to keep compatibility. Since you
get a nice error message from optparser if pass in old arguments I say let's fix
this now.

https://codereview.chromium.org/25044004/diff/1/build/android/pylib/android_c...
File build/android/pylib/android_commands.py (right):

https://codereview.chromium.org/25044004/diff/1/build/android/pylib/android_c...
build/android/pylib/android_commands.py:735: cmd = 'am broadcast -a %s.%s ' %
(package, intent)
On 2013/10/01 10:07:06, bulach wrote:
> nit: add a %s and , ' '.join(args) right here?

Done.

[commit-bot: I haz the power, 2013-10-01 13:33:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/25044004/7001

[commit-bot: I haz the power, 2013-10-01 17:45:23]

Change committed as 226259