sandwich: Refactor to use more existing code.

sandwich: Refactor to use more existing code.

Also fixes a failures caused by 23bd22480abd2209f258d1dd8d3a8572ee17954a:
"Refactor options in analyze.py to global structure"

BUG=582080
Committed: https://crrev.com/51f679843972a6f6592d871fea887b6435b7fe75
Cr-Commit-Position: refs/heads/master@{#376728}

[gabadie, 2016-02-17 17:57:50]

gabadie@chromium.org changed reviewers:
+ lizeb@chromium.org, mattcary@chromium.org

[gabadie, 2016-02-17 17:57:51]

lizeb@chromium.org, mattcary@chromium.org: PTAL.

[mattcary, 2016-02-19 14:41:06]

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/pull_...
File tools/android/loading/pull_sandwich_metrics.py (right):

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/pull_...
tools/android/loading/pull_sandwich_metrics.py:122: assert type(loading_trace)
== loading_trace_module.LoadingTrace
This sort of check seems overly fussy. Worse, it prevents passing in a subclass
(ie, for tests, see https://codereview.chromium.org/1708223005/). 

The following line, where you access the tracing_track, will catch any incorrect
arguments.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/pull_...
File tools/android/loading/pull_sandwich_metrics_unittest.py (right):

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/pull_...
tools/android/loading/pull_sandwich_metrics_unittest.py:52:
page_track.PageTrack(None),
Can just pass in None instead of a *Track created from None. That will help
reduce dependencies generally.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:45: _JOB_SEARCH_PATH = 'sandwich_jobs'
It's idiomatic to create a global here

OPTIONS = options.OPTIONS

to make future references easier.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:58: _CHROME_PACKAGE =
constants.PACKAGE_INFO['chrome'].package
_CHROME_PACKAGE should be taken from options.OPTIONS.ChromePackage() instead.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/traci...
File tools/android/loading/tracing.py (right):

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/traci...
tools/android/loading/tracing.py:15: DEFAULT_CATEGORIES = None
What's the point of creating this? Seems just to be unnecessary indirection.

[gabadie, 2016-02-19 15:04:35]

Thanks Matt. New revision uploaded. PTAL.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/pull_...
File tools/android/loading/pull_sandwich_metrics.py (right):

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/pull_...
tools/android/loading/pull_sandwich_metrics.py:122: assert type(loading_trace)
== loading_trace_module.LoadingTrace
On 2016/02/19 14:41:05, mattcary wrote:
> This sort of check seems overly fussy. Worse, it prevents passing in a
subclass
> (ie, for tests, see https://codereview.chromium.org/1708223005/). 
> 
> The following line, where you access the tracing_track, will catch any
incorrect
> arguments.

Done.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/pull_...
File tools/android/loading/pull_sandwich_metrics_unittest.py (right):

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/pull_...
tools/android/loading/pull_sandwich_metrics_unittest.py:52:
page_track.PageTrack(None),
On 2016/02/19 14:41:05, mattcary wrote:
> Can just pass in None instead of a *Track created from None. That will help
> reduce dependencies generally.

Already tried and I can't otherwise the SaveToJsonFile() fails.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:45: _JOB_SEARCH_PATH = 'sandwich_jobs'
On 2016/02/19 14:41:05, mattcary wrote:
> It's idiomatic to create a global here
> 
> OPTIONS = options.OPTIONS
> 
> to make future references easier.

I have to say I am not a fan options.OPTIONS. It is more a global rather than a
constant since it needs to be initialized before we can access it's variables.
If it is not initialized, it is just unusable. All I want is to have constants.

It may be added if we end-up using it. But I have my doubts.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:58: _CHROME_PACKAGE =
constants.PACKAGE_INFO['chrome'].package
On 2016/02/19 14:41:05, mattcary wrote:
> _CHROME_PACKAGE should be taken from options.OPTIONS.ChromePackage() instead.

But options.OPTIONS has not be initialized yet. And this is what I actually hate
in it. It is not a constant but a global. All I want is a constant.

Additional, what I don't like in options.OPTIONS is it is meant to be configured
with command line flags. But I would rather not have this because it may expose
flags in this command line tools that are just unsupported in this layer. This
why I would prefer the TraceRecorder pattern because he have object out of the
box that works and we can just configured at very high level in the different
places and keep control of what the script are doing and allow to do in the
command line.

Here and the added options.OPTIONS.ParseArgs([]) at the top of the main is more
damage control to fix the breakage.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/traci...
File tools/android/loading/tracing.py (right):

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/traci...
tools/android/loading/tracing.py:15: DEFAULT_CATEGORIES = None
On 2016/02/19 14:41:05, mattcary wrote:
> What's the point of creating this? Seems just to be unnecessary indirection.

Ensure consistency with the callers that can optionally override the given
categories parameter in TracingTrack.

[gabadie, 2016-02-19 15:05:15]

Description was changed from

==========
sandwich: Refactorises to use more existing code.

Also fixes a failures caused by 23bd22480abd2209f258d1dd8d3a8572ee17954a:
"Refactor options in analyze.py to global structure"

BUG=582080
==========

to

==========
sandwich: Refactor to use more existing code.

Also fixes a failures caused by 23bd22480abd2209f258d1dd8d3a8572ee17954a:
"Refactor options in analyze.py to global structure"

BUG=582080
==========

[mattcary, 2016-02-19 15:29:00]

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:45: _JOB_SEARCH_PATH = 'sandwich_jobs'
On 2016/02/19 15:04:35, gabadie wrote:
> On 2016/02/19 14:41:05, mattcary wrote:
> > It's idiomatic to create a global here
> > 
> > OPTIONS = options.OPTIONS
> > 
> > to make future references easier.
> 
> I have to say I am not a fan options.OPTIONS. It is more a global rather than
a
> constant since it needs to be initialized before we can access it's variables.
> If it is not initialized, it is just unusable. All I want is to have
constants.
> 
> It may be added if we end-up using it. But I have my doubts.

The rest of our library depends on the options being configured a certain way.
There are a lot of the problem as with global variables, but the alternative of
having to re-capitulate all configuring with each front-end script is worse. For
example, the devtools hostname and port in the device connection is configured
through OPTIONS flags, and you've skipped that here which makes your script run
inconsistently with everything else.

It is true that the way help text is configured is not optimal, maybe that can
be cleaned up.

My use of OPTIONS is modeled on google3's FLAGS which seems to work well.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:58: _CHROME_PACKAGE =
constants.PACKAGE_INFO['chrome'].package
On 2016/02/19 15:04:35, gabadie wrote:
> On 2016/02/19 14:41:05, mattcary wrote:
> > _CHROME_PACKAGE should be taken from options.OPTIONS.ChromePackage()
instead.
> 
> But options.OPTIONS has not be initialized yet. And this is what I actually
hate
> in it. It is not a constant but a global. All I want is a constant.
> 
> Additional, what I don't like in options.OPTIONS is it is meant to be
configured
> with command line flags. But I would rather not have this because it may
expose
> flags in this command line tools that are just unsupported in this layer. This
> why I would prefer the TraceRecorder pattern because he have object out of the
> box that works and we can just configured at very high level in the different
> places and keep control of what the script are doing and allow to do in the
> command line.
> 
> Here and the added options.OPTIONS.ParseArgs([]) at the top of the main is
more
> damage control to fix the breakage.

But then you may be using a different chrome package than other parts of our
libraries use? That's probably broken (see
device_setup._DevToolsConnectionOnDevice).

I don't think there are any flags that are unsupported in this layer. Some may
not be used by this layer, but I don't think that's a reason to not use the
options layer at all. 

The point of OPTIONS is actually to make it easier to have objects that work out
of the box, because configuration that's not interesting to a particular script
is handled in a uniform and transparent way.

Perhaps we should meet in person on this, but I think we should mandate use of
OPTIONS.

https://codereview.chromium.org/1707793002/diff/1/tools/android/loading/run_s...
tools/android/loading/run_sandwich.py:263: options.OPTIONS.ParseArgs([])
You should pass argv, and add your own arguments as extra args.

An alternative can be seen in analyze.py, where you use your own ArgumentParser
with and argparse.REMAINDER argument, and pass that into ParseArgs.

[mattcary, 2016-02-22 10:07:21]

lgtm

After offline discussion, we've decided that the options parsing needs some
reworking, as caching and device emulation need to be moved out of flag-defined
options so that they can be manipulated grammatically. So Guillaume will land
this without using options in the recommended way, then he & I will work
together to fix the API so that everything will work correctly.

[Benoit L, 2016-02-22 10:26:13]

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
File tools/android/loading/loading_trace.py (right):

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
tools/android/loading/loading_trace.py:45: def SaveToJsonFile(self, json_path):
nit: ToJsonFile to be more consistent with the other methods.

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
tools/android/loading/loading_trace.py:67: """Returns an instance from a json
file saved by SaveToJsonFile()."""
nit: don't forget to update this docstring as well.

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
tools/android/loading/loading_trace.py:70: assert False
Is this really necessary?

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/r...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/r...
tools/android/loading/run_sandwich.py:45: # Use options layer to access
contants.
nit: s/contants/constants/

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
File tools/android/loading/trace_recorder.py (right):

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
tools/android/loading/trace_recorder.py:40: clear_cache: boolean indicating if
cache should be cleared before loading.
nit: missing `categories` arg.

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
File tools/android/loading/tracing.py (right):

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
tools/android/loading/tracing.py:316: def name(self):
Notice: Rebase before landing, as I believe that this might conflict.

[gabadie, 2016-02-22 11:05:07]

Thanks Benoit! New revision uploaded.

lizeb@chromium.org: PTAL. :)

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
File tools/android/loading/loading_trace.py (right):

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
tools/android/loading/loading_trace.py:45: def SaveToJsonFile(self, json_path):
On 2016/02/22 10:26:12, Benoit L wrote:
> nit: ToJsonFile to be more consistent with the other methods.

Done.

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
tools/android/loading/loading_trace.py:67: """Returns an instance from a json
file saved by SaveToJsonFile()."""
On 2016/02/22 10:26:13, Benoit L wrote:
> nit: don't forget to update this docstring as well.

Done.

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
tools/android/loading/loading_trace.py:70: assert False
On 2016/02/22 10:26:13, Benoit L wrote:
> Is this really necessary?

Removed the assert. But yes this method is handy to make one liners.

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/r...
File tools/android/loading/run_sandwich.py (right):

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/r...
tools/android/loading/run_sandwich.py:45: # Use options layer to access
contants.
On 2016/02/22 10:26:13, Benoit L wrote:
> nit: s/contants/constants/

Done.

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
File tools/android/loading/trace_recorder.py (right):

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
tools/android/loading/trace_recorder.py:40: clear_cache: boolean indicating if
cache should be cleared before loading.
On 2016/02/22 10:26:13, Benoit L wrote:
> nit: missing `categories` arg.

Done.

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
File tools/android/loading/tracing.py (right):

https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
tools/android/loading/tracing.py:316: def name(self):
On 2016/02/22 10:26:13, Benoit L wrote:
> Notice: Rebase before landing, as I believe that this might conflict.

Done.

[Benoit L, 2016-02-22 14:05:20]

On 2016/02/22 11:05:07, gabadie wrote:
> Thanks Benoit! New revision uploaded.
> 
> mailto:lizeb@chromium.org: PTAL. :)
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
> File tools/android/loading/loading_trace.py (right):
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
> tools/android/loading/loading_trace.py:45: def SaveToJsonFile(self,
json_path):
> On 2016/02/22 10:26:12, Benoit L wrote:
> > nit: ToJsonFile to be more consistent with the other methods.
> 
> Done.
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
> tools/android/loading/loading_trace.py:67: """Returns an instance from a json
> file saved by SaveToJsonFile()."""
> On 2016/02/22 10:26:13, Benoit L wrote:
> > nit: don't forget to update this docstring as well.
> 
> Done.
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/l...
> tools/android/loading/loading_trace.py:70: assert False
> On 2016/02/22 10:26:13, Benoit L wrote:
> > Is this really necessary?
> 
> Removed the assert. But yes this method is handy to make one liners.
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/r...
> File tools/android/loading/run_sandwich.py (right):
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/r...
> tools/android/loading/run_sandwich.py:45: # Use options layer to access
> contants.
> On 2016/02/22 10:26:13, Benoit L wrote:
> > nit: s/contants/constants/
> 
> Done.
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
> File tools/android/loading/trace_recorder.py (right):
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
> tools/android/loading/trace_recorder.py:40: clear_cache: boolean indicating if
> cache should be cleared before loading.
> On 2016/02/22 10:26:13, Benoit L wrote:
> > nit: missing `categories` arg.
> 
> Done.
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
> File tools/android/loading/tracing.py (right):
> 
>
https://codereview.chromium.org/1707793002/diff/40001/tools/android/loading/t...
> tools/android/loading/tracing.py:316: def name(self):
> On 2016/02/22 10:26:13, Benoit L wrote:
> > Notice: Rebase before landing, as I believe that this might conflict.
> 
> Done.

lgtm, thanks.

[gabadie, 2016-02-22 14:38:35]

The CQ bit was checked by gabadie@chromium.org

[gabadie, 2016-02-22 14:38:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
mattcary@chromium.org
Link to the patchset: https://codereview.chromium.org/1707793002/#ps60001
(title: "Addresses benoit's comments")

[commit-bot: I haz the power, 2016-02-22 14:40:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1707793002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1707793002/60001

[commit-bot: I haz the power, 2016-02-22 15:10:58]

Description was changed from

==========
sandwich: Refactor to use more existing code.

Also fixes a failures caused by 23bd22480abd2209f258d1dd8d3a8572ee17954a:
"Refactor options in analyze.py to global structure"

BUG=582080
==========

to

==========
sandwich: Refactor to use more existing code.

Also fixes a failures caused by 23bd22480abd2209f258d1dd8d3a8572ee17954a:
"Refactor options in analyze.py to global structure"

BUG=582080
==========

[commit-bot: I haz the power, 2016-02-22 15:10:59]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-22 15:13:15]

Description was changed from

==========
sandwich: Refactor to use more existing code.

Also fixes a failures caused by 23bd22480abd2209f258d1dd8d3a8572ee17954a:
"Refactor options in analyze.py to global structure"

BUG=582080
==========

to

==========
sandwich: Refactor to use more existing code.

Also fixes a failures caused by 23bd22480abd2209f258d1dd8d3a8572ee17954a:
"Refactor options in analyze.py to global structure"

BUG=582080

Committed: https://crrev.com/51f679843972a6f6592d871fea887b6435b7fe75
Cr-Commit-Position: refs/heads/master@{#376728}
==========

[commit-bot: I haz the power, 2016-02-22 15:13:15]

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