Create AndroidCommandLineBackend to handle setting up cmdline args

Create AndroidCommandLineBackend to handle setting up cmdline args

BUG=448204, 441959
Committed: https://crrev.com/057c7a7fff9cc00a6a70567512a336ff9f854944
Cr-Commit-Position: refs/heads/master@{#311564}

[ariblue, 2015-01-13 01:52:51]

Patchset #1 (id:1) has been deleted

[ariblue, 2015-01-13 02:21:06]

Patchset #1 (id:20001) has been deleted

[ariblue, 2015-01-13 02:27:02]

ariblue@google.com changed reviewers:
+ chrishenry@google.com, dtu@chromium.org, nednguyen@google.com

[ariblue, 2015-01-13 02:27:02]

Just an FYI, with this patch (along with
https://codereview.chromium.org/842303002/ and the self._tbm modifications to
shared_app_state), I was able to successfully see smoothness metrics for the
agsa benchmark.

[nednguyen, 2015-01-13 16:03:04]

Looks solid to me.

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:29:
webview_startup_args = self.GetWebviewStartupArgs()
Calling a method in the constructor can be dangerous sometimes. How about make
GetWebviewStartupArgs() a classmethod?

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:49:
self._android_command_line_backend.SetUpCommandLine()
Instead of try & finally as below, can we use the __enter__ & __exit__ paradigm?

If self._adb.device().StartActivity(self._start_intent, blocking=False) crashes
for example, the cleanup will fail. The "with" paradigm is a pretty nice way to
guarantee the resource cleanup.

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_browser_backend_settings.py
(right):

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_browser_backend_settings.py:16:
self._cmdline_file = cmdline_file
Can you make the 3 properties below "immutable"?

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_command_line_backend.py
(right):

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_command_line_backend.py:15: def
QuoteIfNeeded(arg):
Please make this private & add unittest for it.

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_command_line_backend.py:32: 
Can you add pydoc for this class?

[ariblue, 2015-01-13 20:23:37]

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:29:
webview_startup_args = self.GetWebviewStartupArgs()
On 2015/01/13 16:03:04, nednguyen wrote:
> Calling a method in the constructor can be dangerous sometimes. How about make
> GetWebviewStartupArgs() a classmethod?

I've moved it to the __enter__ function in part of the switch to the
__enter__/__exit__ paradigm.

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:49:
self._android_command_line_backend.SetUpCommandLine()
On 2015/01/13 16:03:04, nednguyen wrote:
> Instead of try & finally as below, can we use the __enter__ & __exit__
paradigm?
> 
> If self._adb.device().StartActivity(self._start_intent, blocking=False)
crashes
> for example, the cleanup will fail. The "with" paradigm is a pretty nice way
to
> guarantee the resource cleanup.

Yeah, that looks cleaner too. Good call, updated here and in
android_browser_backend

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_browser_backend_settings.py
(right):

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_browser_backend_settings.py:16:
self._cmdline_file = cmdline_file
On 2015/01/13 16:03:04, nednguyen wrote:
> Can you make the 3 properties below "immutable"?

Done (added immutable property for self.activity, as well).

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_command_line_backend.py
(right):

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_command_line_backend.py:15: def
QuoteIfNeeded(arg):
On 2015/01/13 16:03:04, nednguyen wrote:
> Please make this private & add unittest for it.

Done.

https://codereview.chromium.org/811703007/diff/20002/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_command_line_backend.py:32: 
On 2015/01/13 16:03:04, nednguyen wrote:
> Can you add pydoc for this class?

Done.

[ariblue, 2015-01-13 22:53:04]

Patchset #4 (id:80001) has been deleted

[nednguyen, 2015-01-14 17:05:58]

https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:45: with
android_command_line_backend.AndroidCommandLineBackend(
This syntax is a little weird. Can we make a factory method that returns
AndroidCommandLineBackend so you can write it like:

with android_command_line_backend.SetupCommandLineArgs(..):
   ...

We can then make _AndroidCommandLineBackend a private class.

https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:91:
self._adb, self._backend_settings, browser_startup_args):
I think we can move this to right before we start activity and clean up right
after we started the activity, s.t like:

# ....
with ...
  self._adb.device().StartActivity(...)
# the rest

[ariblue, 2015-01-14 21:19:26]

https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):

https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/android_app_backend.py:45: with
android_command_line_backend.AndroidCommandLineBackend(
On 2015/01/14 17:05:58, nednguyen wrote:
> This syntax is a little weird. Can we make a factory method that returns
> AndroidCommandLineBackend so you can write it like:
> 
> with android_command_line_backend.SetupCommandLineArgs(..):
>    ...
> 
> We can then make _AndroidCommandLineBackend a private class. 

Done.

https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
(right):

https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:91:
self._adb, self._backend_settings, browser_startup_args):
On 2015/01/14 17:05:58, nednguyen wrote:
> I think we can move this to right before we start activity and clean up right
> after we started the activity, s.t like:
> 
> # ....
> with ...
>   self._adb.device().StartActivity(...)
> # the rest 

Shouldn't we wait until after _WaitForBrowserToComeUp has completed (just to
make sure we aren't reverting the flags before the browser has actually
launched? I've moved it to start right before the StartActivity() and include
the rest.

[nednguyen, 2015-01-14 21:20:58]

On 2015/01/14 21:19:26, ariblue wrote:
>
https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/backends/android_app_backend.py (right):
> 
>
https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/backends/android_app_backend.py:45: with
> android_command_line_backend.AndroidCommandLineBackend(
> On 2015/01/14 17:05:58, nednguyen wrote:
> > This syntax is a little weird. Can we make a factory method that returns
> > AndroidCommandLineBackend so you can write it like:
> > 
> > with android_command_line_backend.SetupCommandLineArgs(..):
> >    ...
> > 
> > We can then make _AndroidCommandLineBackend a private class. 
> 
> Done.
> 
>
https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
> File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py
> (right):
> 
>
https://codereview.chromium.org/811703007/diff/60001/tools/telemetry/telemetr...
> tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:91:
> self._adb, self._backend_settings, browser_startup_args):
> On 2015/01/14 17:05:58, nednguyen wrote:
> > I think we can move this to right before we start activity and clean up
right
> > after we started the activity, s.t like:
> > 
> > # ....
> > with ...
> >   self._adb.device().StartActivity(...)
> > # the rest 
> 
> Shouldn't we wait until after _WaitForBrowserToComeUp has completed (just to
> make sure we aren't reverting the flags before the browser has actually
> launched? I've moved it to start right before the StartActivity() and include
> the rest.

lgtm

[ariblue, 2015-01-14 21:23:01]

The CQ bit was checked by ariblue@google.com

[commit-bot: I haz the power, 2015-01-14 21:23:54]

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

[commit-bot: I haz the power, 2015-01-14 22:43:44]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-01-14 22:44:50]

Patchset 4 (id:??) landed as
https://crrev.com/057c7a7fff9cc00a6a70567512a336ff9f854944
Cr-Commit-Position: refs/heads/master@{#311564}