Fix periodic dumping for memory-infra on Android.

Fix periodic dumping for memory-infra on Android.

This CL is inspired by ssid's CL: issue 2933193002 at patchset 20001

This CL converts InspectorTracingControllerClient to use the non-deprecated
tracing.start devtools API parameter tracingConfig rather than the deprecated
category + options parameters. This allows InspectorTracingControllerClient to
specify the memoryDumpConfig.

This CL also converts recordingOptions to use the new TraceConfig format, which
can be passed directly to the json/begin_recording endpoint.

BUG=chromium:735124
Review-Url: https://codereview.chromium.org/2948023002
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/a1de83d172bdb9bb8db4d3796f9f6c117a7adbaa

[erikchen, 2017-06-21 01:00:07]

erikchen@chromium.org changed reviewers:
+ fmeawad@chromium.org, ssid@chromium.org

[erikchen, 2017-06-21 01:00:08]

fmeawad: Please review.

[erikchen, 2017-06-21 01:00:13]

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

[commit-bot: I haz the power, 2017-06-21 01:00:23]

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-06-21 01:25:31]

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

[commit-bot: I haz the power, 2017-06-21 01:25:32]

Dry run: This issue passed the CQ dry run.

[fmeawad, 2017-06-21 17:22:10]

On 2017/06/21 01:00:08, erikchen wrote:
> fmeawad: Please review.

This generally looks good, I am wondering if Sid abandoned his CL or he is still
planning to finish it?
Sid, if you abandoned yours, I would appreciate if you take a first look.

[fmeawad, 2017-06-21 17:22:18]

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client_test.html
(right):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client_test.html:75:
result.params.traceConfig.memoryDumpConfig.triggers.length === 1);
nit: Should we also add a check for the default dumping interval?

[ssid, 2017-06-21 18:59:19]

ssid@chromium.org changed reviewers:
+ caseq@chromium.org

[ssid, 2017-06-21 18:59:20]

Thanks!

+Andrey for looking at the part about "enable-sampling" parameter.

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client.html
(left):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client.html:100:
(recordingOptions.useSampling ? 'enable-sampling' : '')
+Andrey can you verify this is not required? I can't see this being used
anywhere, but I am worried some V8 tracing might break because of this?

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File tracing/tracing/ui/extras/about_tracing/record_controller.html (left):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/record_controller.html:89:
categoryFilter: selectionDlg.categoryFilter(),
This line should not be removed. This is used by other tracing clients. Only the
inspector_client does not use this anymore. See the last patchset in my CL.
This would break tracing on desktop.

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/record_controller.html:92: useSampling:
selectionDlg.useSampling
I think we should keep this. I am not sure if any other clients use this field
in options.

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File tracing/tracing/ui/extras/about_tracing/record_selection_dialog.html
(left):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/record_selection_dialog.html:458:
categoryFilter() {
As I mentioned earlier, the desktop tracing controller clients still needs this
function. Else we need to switch all the clients to use trace config instead of
filter+options.

[erikchen, 2017-06-21 22:56:42]

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

[commit-bot: I haz the power, 2017-06-21 22:56:48]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-06-21 22:56:50]

ssid, fmeadwad: PTAL

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client.html
(left):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client.html:100:
(recordingOptions.useSampling ? 'enable-sampling' : '')
On 2017/06/21 18:59:20, ssid wrote:
> +Andrey can you verify this is not required? I can't see this being used
> anywhere, but I am worried some V8 tracing might break because of this?

This is dead code, which is why I removed it. The parser for "options" does not
check for "enable-sampling" anywhere. Also, try code-search for
"enable-sampling":
https://cs.chromium.org/search/?q=%22enable-sampling%22+package:%5Echromium$&...

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client_test.html
(right):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client_test.html:75:
result.params.traceConfig.memoryDumpConfig.triggers.length === 1);
On 2017/06/21 17:22:18, fmeawad wrote:
> nit: Should we also add a check for the default dumping interval?

Done.

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File tracing/tracing/ui/extras/about_tracing/record_controller.html (left):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/record_controller.html:89:
categoryFilter: selectionDlg.categoryFilter(),
On 2017/06/21 18:59:20, ssid wrote:
> This line should not be removed. This is used by other tracing clients. Only
the
> inspector_client does not use this anymore. See the last patchset in my CL.
> This would break tracing on desktop.

There's only 1 other client . iswitched it to also not use categoryFilter.

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/record_controller.html:92: useSampling:
selectionDlg.useSampling
On 2017/06/21 18:59:20, ssid wrote:
> I think we should keep this. I am not sure if any other clients use this field
> in options.

Let's not keep dead code.

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File tracing/tracing/ui/extras/about_tracing/record_selection_dialog.html
(left):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/record_selection_dialog.html:458:
categoryFilter() {
On 2017/06/21 18:59:20, ssid wrote:
> As I mentioned earlier, the desktop tracing controller clients still needs
this
> function. Else we need to switch all the clients to use trace config instead
of
> filter+options.

There's only 1 other client - the xhr client used by desktop.
https://codereview.chromium.org/2948033004/

[commit-bot: I haz the power, 2017-06-21 23:16:19]

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

[commit-bot: I haz the power, 2017-06-21 23:16:20]

Dry run: Try jobs failed on following builders:
  Catapult Mac Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Ma...)

[ssid, 2017-06-22 18:16:22]

lgtm.
At this point I am unaware of any more side effects of this change :P
Thanks for fixes.

[erikchen, 2017-06-22 19:40:10]

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

[commit-bot: I haz the power, 2017-06-22 19:40:19]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-06-22 19:41:30]

Description was changed from

==========
Fix periodic dumping for memory-infra on Android.

This CL is inspired by ssid's CL: issue 2933193002 at patchset 20001

This CL converts InspectorTracingControllerClient to use the non-deprecated
tracing.start devtools API parameter tracingConfig rather than the deprecated
category + options parameters. This allows InspectorTracingControllerClient to
specify the memoryDumpConfig.

BUG=chromium:735124
==========

to

==========
Fix periodic dumping for memory-infra on Android.

This CL is inspired by ssid's CL: issue 2933193002 at patchset 20001

This CL converts InspectorTracingControllerClient to use the non-deprecated
tracing.start devtools API parameter tracingConfig rather than the deprecated
category + options parameters. This allows InspectorTracingControllerClient to
specify the memoryDumpConfig.

This CL also converts recordingOptions to use the new TraceConfig format, which
can be passed directly to the json/begin_recording endpoint.

BUG=chromium:735124
==========

[commit-bot: I haz the power, 2017-06-22 20:05:28]

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

[commit-bot: I haz the power, 2017-06-22 20:05:29]

Dry run: This issue passed the CQ dry run.

[fmeawad, 2017-06-22 23:16:39]

LGTM and thank you for doing the cleanup bits as well.

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client.html
(left):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/inspector_tracing_controller_client.html:100:
(recordingOptions.useSampling ? 'enable-sampling' : '')
On 2017/06/21 22:56:50, erikchen wrote:
> On 2017/06/21 18:59:20, ssid wrote:
> > +Andrey can you verify this is not required? I can't see this being used
> > anywhere, but I am worried some V8 tracing might break because of this?
> 
> This is dead code, which is why I removed it. The parser for "options" does
not
> check for "enable-sampling" anywhere. Also, try code-search for
> "enable-sampling":
>
https://cs.chromium.org/search/?q=%22enable-sampling%22+package:%5Echromium$&...

I can confirm that this is dead code, this is not related to the V8 sampler,
this is a different sampler that was removed.

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
File tracing/tracing/ui/extras/about_tracing/record_controller.html (left):

https://codereview.chromium.org/2948023002/diff/1/tracing/tracing/ui/extras/a...
tracing/tracing/ui/extras/about_tracing/record_controller.html:92: useSampling:
selectionDlg.useSampling
On 2017/06/21 22:56:50, erikchen wrote:
> On 2017/06/21 18:59:20, ssid wrote:
> > I think we should keep this. I am not sure if any other clients use this
field
> > in options.
> 
> Let's not keep dead code.

sampling is to be removed everywhere. Thanks for taking care of that Erik

[erikchen, 2017-06-22 23:18:04]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-06-22 23:18:05]

The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/2948023002/#ps40001
(title: "fix tests.")

[commit-bot: I haz the power, 2017-06-22 23:18:16]

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-06-22 23:19:18]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498173484756000,
"parent_rev": "7935210209d6446cd9f85625857ed87ffb1761a1", "commit_rev":
"a1de83d172bdb9bb8db4d3796f9f6c117a7adbaa"}

[commit-bot: I haz the power, 2017-06-22 23:19:24]

Description was changed from

==========
Fix periodic dumping for memory-infra on Android.

This CL is inspired by ssid's CL: issue 2933193002 at patchset 20001

This CL converts InspectorTracingControllerClient to use the non-deprecated
tracing.start devtools API parameter tracingConfig rather than the deprecated
category + options parameters. This allows InspectorTracingControllerClient to
specify the memoryDumpConfig.

This CL also converts recordingOptions to use the new TraceConfig format, which
can be passed directly to the json/begin_recording endpoint.

BUG=chromium:735124
==========

to

==========
Fix periodic dumping for memory-infra on Android.

This CL is inspired by ssid's CL: issue 2933193002 at patchset 20001

This CL converts InspectorTracingControllerClient to use the non-deprecated
tracing.start devtools API parameter tracingConfig rather than the deprecated
category + options parameters. This allows InspectorTracingControllerClient to
specify the memoryDumpConfig.

This CL also converts recordingOptions to use the new TraceConfig format, which
can be passed directly to the json/begin_recording endpoint.

BUG=chromium:735124

Review-Url: https://codereview.chromium.org/2948023002
Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2017-06-22 23:19:25]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...