sandwich: Refactor to use more existing code.

tools/android/loading/pull_sandwich_metrics.py

Index: tools/android/loading/pull_sandwich_metrics.py
diff --git a/tools/android/loading/pull_sandwich_metrics.py b/tools/android/loading/pull_sandwich_metrics.py
index df24f1c36658a6d3d8893dd7ad5dc643ff0a1abc..c770a7d45889da0aef453276cac6bebc9617503f 100755
--- a/tools/android/loading/pull_sandwich_metrics.py
+++ b/tools/android/loading/pull_sandwich_metrics.py
@@ -15,6 +15,9 @@ import logging
 import os
 import sys
 
+import loading_trace as loading_trace_module
+import tracing
+
 
 CATEGORIES = ['blink.user_timing', 'disabled-by-default-memory-infra']
 
@@ -29,41 +32,43 @@ _CSV_FIELD_NAMES = [
 _TRACKED_EVENT_NAMES = set(['requestStart', 'loadEventStart', 'loadEventEnd'])
 
 
-def _GetBrowserPID(trace):
+def _GetBrowserPID(tracing_track):
   """Get the browser PID from a trace.
 
   Args:
-    trace: The cached trace.
+    tracing_track: The tracing.TracingTrack.
 
   Returns:
     The browser's PID as an integer.
   """
-  for event in trace['traceEvents']:
-    if event['cat'] != '__metadata' or event['name'] != 'process_name':
+  assert type(tracing_track) == tracing.TracingTrack
+  for event in tracing_track.GetEvents():
+    if event.category != '__metadata' or event.name != 'process_name':
       continue
-    if event['args']['name'] == 'Browser':
-      return event['pid']
+    if event.args['name'] == 'Browser':
+      return event.pid
   raise ValueError('couldn\'t find browser\'s PID')
 
 
-def _GetBrowserDumpEvents(trace):
-  """Get the browser memory dump events from a trace.
+def _GetBrowserDumpEvents(tracing_track):
+  """Get the browser memory dump events from a tracing track.
 
   Args:
-    trace: The cached trace.
+    tracing_track: The tracing.TracingTrack.
 
   Returns:
     List of memory dump events.
   """
-  browser_pid = _GetBrowserPID(trace)
+  assert type(tracing_track) == tracing.TracingTrack
+  browser_pid = _GetBrowserPID(tracing_track)
   browser_dumps_events = []
-  for event in trace['traceEvents']:
-    if event['cat'] != 'disabled-by-default-memory-infra':
+  for event in tracing_track.GetEvents():
+    if event.category != 'disabled-by-default-memory-infra':
       continue
-    if event['ph'] != 'v' or event['name'] != 'periodic_interval':
+    if event.type != 'v' or event.name != 'periodic_interval':
       continue
     # Ignore dump events for processes other than the browser process
-    if event['pid'] != browser_pid:
+    if event.pid != browser_pid:
       continue
     browser_dumps_events.append(event)
   if len(browser_dumps_events) == 0:
@@ -71,31 +76,32 @@ def _GetBrowserDumpEvents(trace):
   return browser_dumps_events
 
 
-def _GetWebPageTrackedEvents(trace):
-  """Get the web page's tracked events from a trace.
+def _GetWebPageTrackedEvents(tracing_track):
+  """Get the web page's tracked events from a tracing track.
 
   Args:
-    trace: The cached trace.
+    tracing_track: The tracing.TracingTrack.
 
   Returns:
     Dictionary all tracked events.
   """
+  assert type(tracing_track) == tracing.TracingTrack
   main_frame = None
   tracked_events = {}
-  for event in trace['traceEvents']:
-    if event['cat'] != 'blink.user_timing':
+  for event in tracing_track.GetEvents():
+    if event.category != 'blink.user_timing':
       continue
-    event_name = event['name']
+    event_name = event.name
     # Ignore events until about:blank's unloadEventEnd that give the main
     # frame id.
     if not main_frame:
       if event_name == 'unloadEventEnd':
-        main_frame = event['args']['frame']
+        main_frame = event.args['frame']
         logging.info('found about:blank\'s event \'unloadEventEnd\'')
       continue
     # Ignore sub-frames events. requestStart don't have the frame set but it
     # is fine since tracking the first one after about:blank's unloadEventEnd.
-    if 'frame' in event['args'] and event['args']['frame'] != main_frame:
+    if 'frame' in event.args and event.args['frame'] != main_frame:
       continue
     if event_name in _TRACKED_EVENT_NAMES and event_name not in tracked_events:
       logging.info('found url\'s event \'%s\'' % event_name)
@@ -104,32 +110,34 @@ def _GetWebPageTrackedEvents(trace):
   return tracked_events
 
 
-def _PullMetricsFromTrace(trace):
+def _PullMetricsFromLoadingTrace(loading_trace):
   """Pulls all the metrics from a given trace.
 
   Args:
-    trace: The cached trace.
+    loading_trace: loading_trace_module.LoadingTrace.
 
   Returns:
     Dictionary with all _CSV_FIELD_NAMES's field set (except the 'id').
   """
-  browser_dump_events = _GetBrowserDumpEvents(trace)
-  web_page_tracked_events = _GetWebPageTrackedEvents(trace)
+  assert type(loading_trace) == loading_trace_module.LoadingTrace

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

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.

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

Done.

+  browser_dump_events = _GetBrowserDumpEvents(loading_trace.tracing_track)
+  web_page_tracked_events = _GetWebPageTrackedEvents(
+      loading_trace.tracing_track)
 
   browser_malloc_sum = 0
   browser_malloc_max = 0
   for dump_event in browser_dump_events:
-    attr = dump_event['args']['dumps']['allocators']['malloc']['attrs']['size']
+    attr = dump_event.args['dumps']['allocators']['malloc']['attrs']['size']
     assert attr['units'] == 'bytes'
     size = int(attr['value'], 16)
     browser_malloc_sum += size
     browser_malloc_max = max(browser_malloc_max, size)
 
   return {
-    'total_load': (web_page_tracked_events['loadEventEnd']['ts'] -
-                   web_page_tracked_events['requestStart']['ts']),
-    'onload': (web_page_tracked_events['loadEventEnd']['ts'] -
-               web_page_tracked_events['loadEventStart']['ts']),
+    'total_load': (web_page_tracked_events['loadEventEnd'].start_msec -
+                   web_page_tracked_events['requestStart'].start_msec),
+    'onload': (web_page_tracked_events['loadEventEnd'].start_msec -
+               web_page_tracked_events['loadEventStart'].start_msec),
     'browser_malloc_avg': browser_malloc_sum / float(len(browser_dump_events)),
     'browser_malloc_max': browser_malloc_max
   }
@@ -162,12 +170,11 @@ def _PullMetricsFromOutputDirectory(output_directory_path):
     if not os.path.isfile(trace_path):
       continue
     logging.info('processing \'%s\'' % trace_path)
-    with open(trace_path) as trace_file:
-      trace = json.load(trace_file)
-      trace_metrics = _PullMetricsFromTrace(trace)
-      trace_metrics['id'] = page_id
-      trace_metrics['url'] = run_infos['urls'][page_id]
-      metrics.append(trace_metrics)
+    loading_trace = loading_trace_module.LoadingTrace.FromJsonFile(trace_path)
+    trace_metrics = _PullMetricsFromLoadingTrace(loading_trace)
+    trace_metrics['id'] = page_id
+    trace_metrics['url'] = run_infos['urls'][page_id]
+    metrics.append(trace_metrics)
   assert len(metrics) > 0, ('Looks like \'{}\' was not a sandwich ' +
                             'run directory.').format(output_directory_path)
   return metrics