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

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

Clients are switched to use EvaluateJavaScript2, ExecuteJavaScript2, and
WaitForJavaScriptCondition2.

Some instances of unsafe value interpolation are fixed in the process.

For details see:
https://github.com/catapult-project/catapult/issues/3028

BUG=catapult:#3028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2672803002
Cr-Commit-Position: refs/heads/master@{#449579}
Committed: https://chromium.googlesource.com/chromium/src/+/f1860b973a77bc737dd64664cdc0e382cfbaccf0

[perezju, 2017-02-02 15:28:41]

Description was changed from

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

Clients are switched to use EvaluateJavaScript2, ExecuteJavaScript2, and
WaitForJavaScriptCondition2.

Some instances of unsafe value interpolation are fixed in the process.

For details see:
https://github.com/catapult-project/catapult/issues/3028

BUG=catapult:#3028
==========

to

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

Clients are switched to use EvaluateJavaScript2, ExecuteJavaScript2, and
WaitForJavaScriptCondition2.

Some instances of unsafe value interpolation are fixed in the process.

For details see:
https://github.com/catapult-project/catapult/issues/3028

BUG=catapult:#3028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
==========

[perezju, 2017-02-02 15:28:53]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 15:29:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[perezju, 2017-02-02 16:58:24]

perezju@chromium.org changed reviewers:
+ hansberry@chromium.org, imcheng@chromium.org, kbr@chromium.org,
pasko@chromium.org, ryansturm@chromium.org

[perezju, 2017-02-02 16:58:24]

Please review files in:

imcheng: chrome/test/media_router/telemetry/benchmarks/
hansberry: components/proximity_auth/
kbr: content/test/gpu/gpu_tests/
pasko: ools/android/loading/
ryansturm: tools/chrome_proxy/

+ned FYI

Thanks!

[pasko, 2017-02-02 17:05:44]

On 2017/02/02 16:58:24, perezju wrote:
> pasko: ools/android/loading/

naming is a little .. surprising, is there a plan to remove the '2' from the
names?

tools/android/loading/ is not covered by tests on the bots afair, what did you
run to confirm that it still works?

[perezju, 2017-02-02 17:20:24]

On 2017/02/02 17:05:44, pasko wrote:
> On 2017/02/02 16:58:24, perezju wrote:
> > pasko: ools/android/loading/
> 
> naming is a little .. surprising, is there a plan to remove the '2' from the
> names?

Yep. After the migration is completed there will be a second rename to drop the
'2'. I really tried to avoid the 2-step rename, but the API currently exposed by
Telemetry is a bit of a mess (similarly named methods on different classes have
different interfaces), and it was safer to do it this way to make sure that we
don't break any clients.

Full details in the catapult bug.

> tools/android/loading/ is not covered by tests on the bots afair, what did you
> run to confirm that it still works?

Was not aware of this. Can you suggest a command to try?

[commit-bot: I haz the power, 2017-02-02 17:48:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 17:48:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Kyle Horimoto, 2017-02-02 18:04:15]

Description was changed from

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

Clients are switched to use EvaluateJavaScript2, ExecuteJavaScript2, and
WaitForJavaScriptCondition2.

Some instances of unsafe value interpolation are fixed in the process.

For details see:
https://github.com/catapult-project/catapult/issues/3028

BUG=catapult:#3028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
==========

to

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

Clients are switched to use EvaluateJavaScript2, ExecuteJavaScript2, and
WaitForJavaScriptCondition2.

Some instances of unsafe value interpolation are fixed in the process.

For details see:
https://github.com/catapult-project/catapult/issues/3028

BUG=catapult:#3028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
==========

[Kyle Horimoto, 2017-02-02 18:04:15]

khorimoto@chromium.org changed reviewers:
+ tengs@chromium.org
- hansberry@chromium.org

[pasko, 2017-02-02 20:12:36]

On 2017/02/02 17:20:24, perezju wrote:
> > tools/android/loading/ is not covered by tests on the bots afair, what did
you
> > run to confirm that it still works?
> 
> Was not aware of this. Can you suggest a command to try?

Apologies, this is only used from sandwich.py for speed index calculation, which
is not the default mode, and may need another patch to run.

So let's not do it, unless I get more suspicious :)

[pasko, 2017-02-02 20:15:50]

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
tools/android/loading/devtools_monitor.py:292: expression =
js_template.Render(expression, **kwargs)
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?

[RyanSturm, 2017-02-02 20:21:47]

chrome_proxy: lgtm

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

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
tools/android/loading/devtools_monitor.py:292: expression =
js_template.Render(expression, **kwargs)
On 2017/02/02 20:15:49, pasko wrote:
> 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?

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.

[perezju, 2017-02-03 09:33:51]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 09:34:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 10:26:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 10:26:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ken Russell (switch to Gerrit), 2017-02-04 21:42:26]

content/test/gpu lgtm

I hope that you'll consider renaming these back once everyone's cut over to the
new APIs.

[perezju, 2017-02-06 09:51:57]

The CQ bit was checked by perezju@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 09:52:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[perezju, 2017-02-06 09:55:01]

perezju@chromium.org changed reviewers:
+ hansberry@chromium.org

[perezju, 2017-02-06 09:55:02]

On 2017/02/04 21:42:26, Ken Russell wrote:
> content/test/gpu lgtm
> 
> I hope that you'll consider renaming these back once everyone's cut over to
the
> new APIs.

Yes. That's definitely in the plans.

[commit-bot: I haz the power, 2017-02-06 10:58:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-06 10:58:35]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[imcheng, 2017-02-06 19:54:44]

imcheng@chromium.org changed reviewers:
+ leilei@chromium.org

[imcheng, 2017-02-06 19:54:45]

media_router lgtm.

+leilei as FYI

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

sorry for slow response, I was on various convergences :)

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
tools/android/loading/devtools_monitor.py:23: from telemetry.util import
js_template
we try to avoid depending on libraries exported from Telemetry (except the
absolutely essential low-level ones).

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
tools/android/loading/devtools_monitor.py:292: expression =
js_template.Render(expression, **kwargs)
On 2017/02/03 09:31:45, perezju wrote:
> On 2017/02/02 20:15:49, pasko wrote:
> > 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?
> 
> 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.

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]

tengs, hansberry: hope you can help me with a stamp for
components/proximity_auth - Thanks!

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
tools/android/loading/devtools_monitor.py:292: expression =
js_template.Render(expression, **kwargs)
On 2017/02/06 21:50:51, pasko wrote:
> 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.

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]

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
tools/android/loading/devtools_monitor.py:292: expression =
js_template.Render(expression, **kwargs)
On 2017/02/07 10:02:21, perezju wrote:
> On 2017/02/06 21:50:51, pasko wrote:
> > 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.
> 
> 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

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]

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
File tools/android/loading/devtools_monitor.py (right):

https://codereview.chromium.org/2672803002/diff/1/tools/android/loading/devto...
tools/android/loading/devtools_monitor.py:292: expression =
js_template.Render(expression, **kwargs)
On 2017/02/07 18:57:29, pasko wrote:
> On 2017/02/07 10:02:21, perezju wrote:
> > On 2017/02/06 21:50:51, pasko wrote:
> > > 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.
> > 
> > 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
> 
> 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.

Done.

[pasko, 2017-02-08 18:29:49]

tools/android/loading lgtm, thank you

[Tim Song, 2017-02-09 17:29:55]

components/proximity_auth/e2e_test/ LGTM

[perezju, 2017-02-10 09:08:30]

The CQ bit was checked by perezju@chromium.org

[perezju, 2017-02-10 09:08:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, kbr@chromium.org, imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2672803002/#ps80001
(title: "add comment on tools/android")

[commit-bot: I haz the power, 2017-02-10 09:08:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 10:38:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1486717710280380,
"parent_rev": "7f190b5091427c5d08ad224415ff1b5e5f57b90f", "commit_rev":
"f1860b973a77bc737dd64664cdc0e382cfbaccf0"}

[commit-bot: I haz the power, 2017-02-10 10:39:25]

Description was changed from

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

Clients are switched to use EvaluateJavaScript2, ExecuteJavaScript2, and
WaitForJavaScriptCondition2.

Some instances of unsafe value interpolation are fixed in the process.

For details see:
https://github.com/catapult-project/catapult/issues/3028

BUG=catapult:#3028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
==========

to

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

Clients are switched to use EvaluateJavaScript2, ExecuteJavaScript2, and
WaitForJavaScriptCondition2.

Some instances of unsafe value interpolation are fixed in the process.

For details see:
https://github.com/catapult-project/catapult/issues/3028

BUG=catapult:#3028
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2672803002
Cr-Commit-Position: refs/heads/master@{#449579}
Committed:
https://chromium.googlesource.com/chromium/src/+/f1860b973a77bc737dd64664cdc0...
==========

[commit-bot: I haz the power, 2017-02-10 10:39:27]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/f1860b973a77bc737dd64664cdc0...