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

tools/android/loading/devtools_monitor.py

Index: tools/android/loading/devtools_monitor.py
diff --git a/tools/android/loading/devtools_monitor.py b/tools/android/loading/devtools_monitor.py
index b64e468d5c210c447c8cd7178214f27413e93382..1216b2c2c1fc3b3d006a43abc45033176ebbd6ca 100644
--- a/tools/android/loading/devtools_monitor.py
+++ b/tools/android/loading/devtools_monitor.py
@@ -20,6 +20,7 @@ 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
 
@@ -279,7 +280,7 @@ class DevToolsConnection(object):
           current_time + datetime.timedelta(
               seconds=stop_delay_duration_seconds))
 
-  def ExecuteJavaScript(self, expression):
+  def ExecuteJavaScript2(self, expression, **kwargs):
     """Run JavaScript expression.
 
     Args:
@@ -288,6 +289,7 @@ class DevToolsConnection(object):
     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})