[Telemetry refactor] Migrate clients to new JavaScript API (batch 3)

tools/android/loading/devtools_monitor.py

 # Copyright (c) 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.
 
 """Library handling DevTools websocket interaction.
 """
 
 import datetime
 import httplib
 import json
 import logging
 import os
 import sys
 import time
 
 file_dir = os.path.dirname(__file__)
 sys.path.append(os.path.join(file_dir, '..', '..', 'perf'))
 from chrome_telemetry_build import chromium_config
 sys.path.append(chromium_config.GetTelemetryDir())
 
 from telemetry.internal.backends.chrome_inspector import inspector_websocket
 from telemetry.internal.backends.chrome_inspector import websocket
+from telemetry.util import js_template

[pasko, 2017/02/06 21:50:51]

we try to avoid depending on libraries exported from Telemetry (except the
absolutely essential low-level ones).

 
 import common_util
 
 
 DEFAULT_TIMEOUT_SECONDS = 10
 
 _WEBSOCKET_TIMEOUT_SECONDS = 10
 
 
 class DevToolsConnectionException(Exception):
@@ skipping 239 matching lines @@
       assert self._monitoring_start_timestamp is not None
       current_time = datetime.datetime.now()
       stop_delay_duration_seconds = self._stop_delay_multiplier * (
           current_time - self._monitoring_start_timestamp).seconds
       logging.info('Delaying monitoring stop for %.1fs',
                    stop_delay_duration_seconds)
       self._monitoring_stop_timestamp = (
           current_time + datetime.timedelta(
               seconds=stop_delay_duration_seconds))
 
-  def ExecuteJavaScript(self, expression):
+  def ExecuteJavaScript2(self, expression, **kwargs):
     """Run JavaScript expression.
 
     Args:
       expression: JavaScript expression to run.
 
     Returns:
       The return value from the JavaScript expression.
     """
+    expression = js_template.Render(expression, **kwargs)

[pasko, 2017/02/02 20:15:49]

the discussion in the bug is a leaning towards lengthy, can you clarify why this
needs to be done? does the doc for |expression| need to change? Seems like it is
not a JS expression any more, but rather a template, no?

[perezju, 2017/02/03 09:31:45]

You are right. Strictly speaking the changes in tools/android/loading are not
needed, since they don't actually use the Telemetry API but re-implement it
again. (Ideally this file should be de-duped form
telemetry.internal.backends.chrome_inspector.inspector_backend, but that's
beyond the scope of this refactor.)

I only made these changes for consistency, to mirror the interface and
functionality that the Telemetry APIs provide. I've updated the docs
accordingly.

If you prefer, I can also leave these two files alone, and just blacklist them
on my local script that finds API clients that need to be migrated.

[pasko, 2017/02/06 21:50:51]

Ah, I see. Actually we do not want to increase our dependency on Telemetry. I
would prefer just putting an assert here that would fire if any special
characters are in the stream (or catching the syntax error with re-raising - if
that's better from your standpoint), and making the code complain that those
chars are not supported.

This code is called in a very narrow use-case that you saw, and there are no
plans to make it accept more complicated inputs, so an assert would eliminate
the dependency nicely.

[perezju, 2017/02/07 10:02:21]

To keep things simple I'll just discard the changes on these files. If there is
a syntax error in the javascript string, it will be raised anyway as it did on
the first example in https://github.com/catapult-project/catapult/issues/3028

[pasko, 2017/02/07 18:57:29]

Sounds good. Please add a comment with this link to one of these places where
the exception can be raised, this way the hypothetical fix would be probably
more aligned with the new API.

[perezju, 2017/02/08 09:58:34]

Done.

     response = self.SyncRequest('Runtime.evaluate', {
         'expression': expression,
         'returnByValue': True})
     if 'error' in response:
       raise Exception(response['error']['message'])
     if 'wasThrown' in response['result'] and response['result']['wasThrown']:
       raise Exception(response['error']['result']['description'])
     if response['result']['result']['type'] == 'undefined':
       return None
     return response['result']['result']['value']
@@ skipping 179 matching lines @@
       json_data: (dict) Parsed from a JSON file using the json module.
 
     Returns:
       a Track instance.
     """
     # There is no sensible way to deserialize this abstract class, but
     # subclasses are not required to define a deserialization method. For
     # example, for testing we have a FakeRequestTrack which is never
     # deserialized; instead fake instances are deserialized as RequestTracks.
     assert False