Add GetResponses() and related funcionality

tools/chrome_proxy/webdriver/common.py

Index: tools/chrome_proxy/webdriver/common.py
diff --git a/tools/chrome_proxy/webdriver/common.py b/tools/chrome_proxy/webdriver/common.py
index 29c7e58a6ccd1201162126d8092990618ea9b4f8..46d37908f222cedbc419ee2cd1ca1a28de7f7cfd 100644
--- a/tools/chrome_proxy/webdriver/common.py
+++ b/tools/chrome_proxy/webdriver/common.py
@@ -5,6 +5,7 @@
 import argparse
 import json
 import os
+import re
 import shlex
 import sys
 import time
@@ -34,8 +35,7 @@ def ParseFlags():
   parser.add_argument('chrome_driver', nargs=1, type=str, help='The path to '
     'the ChromeDriver executable. If not given, the default system chrome '
     'will be used.')
-  # TODO(robertogden) make this a logging statement
-  print 'DEBUG: Args=', json.dumps(vars(parser.parse_args(sys.argv[1:])))
+  # TODO(robertogden) make a logging statement here

[sclittle, 2016/12/06 00:09:49]

style nit: You should have a colon after the "TODO(robertogden)" and all
comments should be complete sentences.

Also, could you clarify what you mean by "make a logging statement here" in the
comment?

[Robert Ogden, 2016/12/06 16:57:16]

Done.

   return parser.parse_args(sys.argv[1:])
 
 def HandleException(test_name=None):
@@ -191,6 +191,106 @@ class TestDriver:
       self._StartDriver()
     return self._driver.execute_script("return " + script)
 
+  def GetPerformanceLogs(self, method_filter=r'Network\.responseReceived'):

[sclittle, 2016/12/06 00:09:49]

For all of these class and function definitions, you should be following the
Google Python style guide, e.g. starting with a single summary line.

See the style guide for more details:
https://google.github.io/styleguide/pyguide.html?showone=Comments#Comments

You can fix the style of the existing code here in a separate CL if you'd like,
but eventually it should follow the style guide.

[Robert Ogden, 2016/12/06 16:57:16]

Done.

+    """
+    Returns all logged Performance events from Chrome as a list of dicts, since
+    the last time this function was called. method_filter accepts a regex to
+    match against the method type of the events to return.
+    """
+    all_messages = []
+    for log in self._driver.execute('getLog', {'type': 'performance'})['value']:
+      message = json.loads(log['message'])['message']
+      if re.match(method_filter, message['method']):
+        all_messages.append(message)
+    return all_messages

[sclittle, 2016/12/06 00:09:49]

nit: This is minor, but if you don't need the entire list of logs at once, you
might consider making this function a generator (e.g. using "yield" instead of
"return"), so that each yielded value will be iterated over.

Same with the GetHTTPResponses() below.

[Robert Ogden, 2016/12/06 16:57:16]

Thought about that too. Kept it like this so that the len() function would still
work, which seems like it could be useful with all the counting and checking we
did in Telemetry.

+
+  def GetHTTPResponses(self, include_favicon=False):
+    """
+    Parses the Performance Logs and returns a list of HTTPResponse objects, each
+    representing a single completed HTTP transaction by Chrome. Optionally
+    allows favicon requests to be included in the result as well.
+    This function should be called exactly once after every page load.
+    """
+    def MakeHTTPResponse(log_dict):
+      params = log_dict['params']
+      response_dict = params['response']
+      response_headers = response_dict['headers']
+      request_headers = response_dict['requestHeaders']
+      url = response_dict['url']
+      protocol = request_headers[':scheme']
+      status = response_dict['status']
+      request_type = params['type']
+      return HTTPResponse(response_headers, request_headers, url, protocol,

[sclittle, 2016/12/06 00:09:49]

nit: Instead of making so many local variables, could you just pass in the dict
lookups directly to the constructor here?

e.g. return HTTPResponse(response_dict['headers'], ...)

[Robert Ogden, 2016/12/06 16:57:16]

Done.

+        status, request_type)
+    all_responses = []
+    for message in self.GetPerformanceLogs():
+      response = MakeHTTPResponse(message)
+      is_favicon = response.url.endswith('favicon.ico')
+      if not is_favicon or include_favicon:
+        all_responses.append(response)
+    return all_responses
+
+class HTTPResponse:
+  """
+  This class represents a single HTTP transaction (request and response) by
+  Chrome. This class also includes several convenience functions for ChromeProxy
+  specific assertions.
+  """
+
+  def __init__(self, response_headers, request_headers, url, protocol,
+      status, request_type):
+    self._response_headers = response_headers
+    self._request_headers = request_headers
+    self._url = url
+    self._protocol = protocol
+    self._status = status
+    self._request_type = request_type
+    self._flags = ParseFlags()
+
+  def __str__(self):
+    self_dict = {
+      'response_headers': self._response_headers,
+      'request_headers': self._request_headers,
+      'url': self._url,
+      'protocol': self._protocol,
+      'status': self._status,
+      'request_type': self._request_type
+    }
+    return json.dumps(self_dict)
+
+  @property
+  def response_headers(self):
+    return self._response_headers
+
+  @property
+  def request_headers(self):
+    return self._request_headers
+
+  @property
+  def url(self):
+    return self._url
+
+  @property
+  def protocol(self):
+    return self._protocol
+
+  @property
+  def status(self):
+    return self._status
+
+  @property
+  def request_type(self):
+    return self._request_type
+
+  def UsedHTTPS(self):
+    return self._protocol == 'https'

[sclittle, 2016/12/06 00:09:49]

What do you intent |UsedHTTPS| to be used for? I haven't confirmed this, but I'd
guess that it checks if the fetched URL used HTTPS, regardless of whether or not
the secure proxy was used.

IIRC we have a of couple tests (e.g. HTTPFallbackToDirect or something) that
check that the secure or insecure proxy is specifically being used.

If those are what you want to use |UsedHTTPS| for, then could you confirm that
it does what you want it to do?

[Robert Ogden, 2016/12/06 16:57:15]

Correct, there are a number of tests where we need to check for HTTPS vs HTTP.
and yes, this is working as intended.

+
+  def ResponseHasViaHeader(self):
+    return 'via' in self._response_headers and (self._response_headers['via'] ==
+      self._flags.via_header_value)
+
+  def WasXHR(self):
+    return self.request_type == 'XHR'
 
 class IntegrationTest:
   """