sandwich: Refactor to use more existing code.

tools/android/loading/run_sandwich.py

Index: tools/android/loading/run_sandwich.py
diff --git a/tools/android/loading/run_sandwich.py b/tools/android/loading/run_sandwich.py
index ef7fca8b2391036f73a2abc4c11b678ab8c38582..422134fb154664ff17e3ea85a9b0585a94d2e448 100755
--- a/tools/android/loading/run_sandwich.py
+++ b/tools/android/loading/run_sandwich.py
@@ -35,9 +35,10 @@ import devil_chromium
 
 import device_setup
 import devtools_monitor
-import json
+import options
 import page_track
 import pull_sandwich_metrics
+import trace_recorder
 import tracing
 
 
@@ -54,8 +55,7 @@ _INDEX_DIRECTORY_NAME = 'index-dir'
 _REAL_INDEX_FILE_NAME = 'the-real-index'
 
 # Name of the chrome package.
-_CHROME_PACKAGE = (
-    constants.PACKAGE_INFO[device_setup.DEFAULT_CHROME_PACKAGE].package)
+_CHROME_PACKAGE = constants.PACKAGE_INFO['chrome'].package

[mattcary, 2016/02/19 14:41:05]

_CHROME_PACKAGE should be taken from options.OPTIONS.ChromePackage() instead.

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

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.

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

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.

 
 # An estimate of time to wait for the device to become idle after expensive
 # operations, such as opening the launcher activity.
@@ -86,23 +86,6 @@ def _ReadUrlsFromJobDescription(job_name):
   raise Exception('Job description does not define a list named "urls"')
 
 
-def _SaveChromeTrace(events, target_directory):
-  """Saves the trace events, ignores IO errors.
-
-  Args:
-    events: a dict as returned by TracingTrack.ToJsonDict()
-    target_directory: Directory path where trace is created.
-  """
-  filename = os.path.join(target_directory, 'trace.json')
-  try:
-    os.makedirs(target_directory)
-    with open(filename, 'w') as f:
-      json.dump({'traceEvents': events['events'], 'metadata': {}}, f, indent=2)
-  except IOError:
-    logging.warning('Could not save a trace: %s' % filename)
-    # Swallow the exception.
-
-
 def _UpdateTimestampFromAdbStat(filename, stat):
   os.utime(filename, (stat.st_time, stat.st_time))
 
@@ -277,6 +260,7 @@ def _CleanPreviousTraces(output_directories_path):
 def main():
   logging.basicConfig(level=logging.INFO)
   devil_chromium.Initialize()
+  options.OPTIONS.ParseArgs([])

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

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.

 
   parser = argparse.ArgumentParser()
   parser.add_argument('--job', required=True,
@@ -331,17 +315,15 @@ def main():
       with device_setup.DeviceConnection(
           device=device,
           additional_flags=additional_flags) as connection:
-        if clear_cache:
-          connection.ClearCache()
-        page_track.PageTrack(connection)
-        tracing_track = tracing.TracingTrack(connection,
+        loading_trace = trace_recorder.MonitorUrl(
+            connection, url,
+            clear_cache=clear_cache,
             categories=pull_sandwich_metrics.CATEGORIES)
-        connection.SetUpMonitoring()
-        connection.SendAndIgnoreResponse('Page.navigate', {'url': url})
-        connection.StartMonitoring()
         if trace_id != None:
-          trace_target_directory = os.path.join(args.output, str(trace_id))
-          _SaveChromeTrace(tracing_track.ToJsonDict(), trace_target_directory)
+          loading_trace_path = os.path.join(
+              args.output, str(trace_id), 'trace.json')
+          os.makedirs(os.path.dirname(loading_trace_path))
+          loading_trace.SaveToJsonFile(loading_trace_path)
 
     for _ in xrange(args.repeat):
       for url in job_urls: