sandwich: Refactor to use more existing code.

tools/android/loading/run_sandwich.py

 #! /usr/bin/env python
 # Copyright 2016 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Instructs Chrome to load series of web pages and reports results.
 
 When running Chrome is sandwiched between preprocessed disk caches and
 WepPageReplay serving all connections.
 
@@ skipping 17 matching lines @@
 
 sys.path.append(os.path.join(_SRC_DIR, 'third_party', 'catapult', 'devil'))
 from devil.android import device_utils
 
 sys.path.append(os.path.join(_SRC_DIR, 'build', 'android'))
 from pylib import constants
 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
 
 
 _JOB_SEARCH_PATH = 'sandwich_jobs'

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

It's idiomatic to create a global here

OPTIONS = options.OPTIONS

to make future references easier.

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

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.

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

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.

 
 # Directory name under --output to save the cache from the device.
 _CACHE_DIRECTORY_NAME = 'cache'
 
 # Name of cache subdirectory on the device where the cache index is stored.
 _INDEX_DIRECTORY_NAME = 'index-dir'
 
 # Name of the file containing the cache index. This file is stored on the device
 # in the cache directory under _INDEX_DIRECTORY_NAME.
 _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.
 _TIME_TO_DEVICE_IDLE_SECONDS = 2
 
 # Cache directory's path on the device.
 _REMOTE_CACHE_DIRECTORY = '/data/data/' + _CHROME_PACKAGE + '/cache/Cache'
 
 
 def _ReadUrlsFromJobDescription(job_name):
@@ skipping 10 matching lines @@
       json_data = json.load(f)
 
   key = 'urls'
   if json_data and key in json_data:
     url_list = json_data[key]
     if isinstance(url_list, list) and len(url_list) > 0:
       return url_list
   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))
 
 
 def _AdbShell(adb, cmd):
   adb.Shell(subprocess.list2cmdline(cmd))
 
 
 def _AdbUtime(adb, filename, timestamp):
   """Adb equivalent of os.utime(filename, (timestamp, timestamp))
@@ skipping 154 matching lines @@
     try:
       int(dirname)
     except ValueError:
       continue
     shutil.rmtree(directory_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,
                       help='JSON file with job description.')
   parser.add_argument('--output', required=True,
                       help='Name of output directory to create.')
   parser.add_argument('--repeat', default=1, type=int,
                       help='How many times to run the job')
   parser.add_argument('--cache-op',
                       choices=['clear', 'save', 'push', 'reload'],
@@ skipping 34 matching lines @@
     assert os.path.isfile(local_cache_archive_path)
     local_cache_directory_path = tempfile.mkdtemp(suffix='.cache')
     _UnzipDirectoryContent(local_cache_archive_path, local_cache_directory_path)
 
   with device_setup.WprHost(device, args.wpr_archive, args.wpr_record,
       args.disable_wpr_script_injection) as additional_flags:
     def _RunNavigation(url, clear_cache, trace_id):
       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:
         clear_cache = False
         if args.cache_op == 'clear':
           clear_cache = True
         elif args.cache_op == 'push':
           device.KillAll(_CHROME_PACKAGE, quiet=True)
           _PushBrowserCache(device, local_cache_directory_path)
         elif args.cache_op == 'reload':
@@ skipping 17 matching lines @@
     cache_directory_path = _PullBrowserCache(device)
     _ZipDirectoryContent(cache_directory_path, local_cache_archive_path)
     shutil.rmtree(cache_directory_path)
 
   with open(os.path.join(args.output, 'run_infos.json'), 'w') as file_output:
     json.dump(run_infos, file_output, indent=2)
 
 
 if __name__ == '__main__':
   sys.exit(main())