tools/android/loading: ContentClassificationLens, ads and tracking requests.

tools/android/loading/loading_model.py

Index: tools/android/loading/loading_model.py
diff --git a/tools/android/loading/loading_model.py b/tools/android/loading/loading_model.py
index 155da1ddf8ef201e5e732a8abe6e93c148dc2fd8..0c4ffda04cf4bdbf2f194a4318f18f499bbd4df1 100644
--- a/tools/android/loading/loading_model.py
+++ b/tools/android/loading/loading_model.py
@@ -30,14 +30,17 @@ class ResourceGraph(object):
   Set parameters:
     cache_all: if true, assume zero loading time for all resources.
   """
-  def __init__(self, trace):
+  def __init__(self, trace, content_lens=None):
     """Create from a LoadingTrace (or json of a trace).
 
     Args:
       trace: (LoadingTrace/JSON) Loading trace or JSON of a trace.
+      content_lens: (ContentClassificationLens) Lens used to annotate the
+                    nodes, or None.
     """
     if type(trace) == dict:
       trace = loading_trace.LoadingTrace.FromJsonDict(trace)
+    self._content_lens = content_lens
     self._BuildDag(trace)
     self._global_start = min([n.StartTime() for n in self._node_info])
     # Sort before splitting children so that we can correctly dectect if a
@@ -331,14 +334,18 @@ class ResourceGraph(object):
     We also store the request on the node, and expose request-derived
     information like content type.
     """
-    def __init__(self, node, request):
+    def __init__(self, node, request, is_ad, is_tracking):
       """Create a new node info.
 
       Args:
         node: The node to augment.
         request: The request associated with this node.
+        is_ad: (bool) Whether the request is an Ad.
+        is_tracking: (bool) Whether the request is related to tracking.
       """
       self._request = request
+      self._is_ad = is_ad
+      self._is_tracking = is_tracking
       self._node = node
       self._edge_costs = {}
       self._edge_annotations = {}
@@ -357,6 +364,12 @@ class ResourceGraph(object):
     def Index(self):
       return self._node.Index()
 
+    def IsAd(self):
+      return self._is_ad
+
+    def IsTracking(self):
+      return self._is_tracking
+
     def Request(self):
       return self._request
 
@@ -480,7 +493,11 @@ class ResourceGraph(object):
       assert request not in index_by_request
       index_by_request[request] = next_index
       node = dag.Node(next_index)
-      node_info = self._NodeInfo(node, request)
+      is_ad = (self._content_lens.IsAdRequest(request)
+               if self._content_lens else False)
+      is_tracking = (self._content_lens.IsTrackingRequest(request)
+               if self._content_lens else False)
+      node_info = self._NodeInfo(node, request, is_ad, is_tracking)

[mattcary, 2016/01/25 16:08:58]

I think it would be nicer to add setters to NodeInfo for tracking (or maybe even
a single SetRequestContent(is_ad, is_tracking) method, have NodeInfo default
those members to false, and then conditionally set them if we have a
content_lens.

IMHO the pattern of infinitely extending constructor parameters gets
unmaintainable, particularly in tests (true, that doesn't matter here, but
what's a code review if not a chance to preach general principles that don't
apply to the code in question?).

[Benoit L, 2016/01/26 13:31:13]

Absolutely, thanks for the suggestion.
Done.

       self._nodes.append(node)
       self._node_info.append(node_info)
 
@@ -606,6 +623,8 @@ class ResourceGraph(object):
         if fragment in node_info.Url():
           styles.append('dotted')
           break
+    if node_info.IsAd() or node_info.IsTracking():
+      styles += ['bold', 'diagonals']
     return ('%d [label = "%s\\n%.2f->%.2f (%.2f)"; style = "%s"; '
             'fillcolor = %s; shape = %s];\n'
             % (index, node_info.ShortName(),