1111004 - Handle multiple recordings in the new performance tool

Status: RESOLVED FIXED

(DevTools :: Performance Tools (Profiler/Timeline), defect)

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1111004-multi-recordings.patch

Check this out. Still gotta clean up some tests and add more tests, but this is the idea.

Comment 2

[Victor Porof [:vporof][:vp]]

Comment on attachment 8547870 [details] [diff] [review]
1111004-multi-recordings.patch

Review of attachment 8547870 [details] [diff] [review]:
-----------------------------------------------------------------

Swag, r+ with tests and everything. Ask me again for review.

::: browser/devtools/jar.mn
@@ +89,5 @@
>  #ifdef MOZ_DEVTOOLS_PERFTOOLS
>      content/browser/devtools/performance.xul                           (performance/performance.xul)
>      content/browser/devtools/performance/performance-controller.js     (performance/performance-controller.js)
>      content/browser/devtools/performance/performance-view.js           (performance/performance-view.js)
> +    content/browser/devtools/performance/recording-model.js            (performance/recording-model.js)

I feel like this should be a module.

::: browser/devtools/performance/performance-controller.js
@@ +17,5 @@
>  devtools.lazyRequireGetter(this, "DevToolsUtils",
>    "devtools/toolkit/DevToolsUtils");
> +// Overloads ViewHelper's `Heritage`.
> +devtools.lazyRequireGetter(this, "Heritage",
> +  "sdk/core/heritage");

Why?

@@ +50,5 @@
>  const EVENTS = {
> +  // Emitted by the PerformanceController when a recording model
> +  // is selected. Also emitted by the RecordingsView, which is
> +  // re-emitted by the PerformanceController. Usually, should only listen
> +  // to the PerformanceController's event here.

This comment makes sounds too canadian. Just write "Emitted by the PerformanceController and PerformanceView when a recording model is selected"

@@ +343,2 @@
>     */
> +  _getLastManualProfile: function () {

Nit: _getLatest sounds more accurate.

@@ +343,4 @@
>     */
> +  _getLastManualProfile: function () {
> +    for (let i = this._recordings.length - 1; i >= 0; i--) {
> +      if (!this._recordings[i].fromConsole()) {

No need for .fromConsole for now. Should be added in a later patch.

@@ +349,1 @@
>      }

This method doesn't always return something. Either throw or return null.

::: browser/devtools/performance/recording-model.js
@@ +6,5 @@
> +/**
> + * Model for a wholistic profile, containing start/stop times, profiling data, frames data,
> + * timeline (marker, tick, memory) data, and methods to start/stop recording.
> + */
> +const RecordingModel = Heritage.Class({

Why use Class?

::: browser/devtools/performance/views/recordings.js
@@ +97,5 @@
> +
> +    // If a label is specified (e.g due to a call to `console.profile`),
> +    // then try reusing a pre-existing recording item, if there is one.
> +    // This is symmetrical to how `this.handleRecordingEnded` works.
> +    if (recording.fromConsole()) {

It's probably more idiomatic to have a `hasLabel` and use it here, instead of `fromConsole`. We don't want to make the two synonymous (think GCLI, ulterior additions to the console API, GCLI, etc.).

@@ +132,5 @@
> +    // then try reusing a pre-existing recording item, if there is one.
> +    // This is symmetrical to how `this.handleRecordingStarted` works.
> +    if (profileLabel) {
> +      recordingItem = this.getItemForAttachment(e =>
> +        e.profilerData.profileLabel == profileLabel);

Why aren't we using getLabel() here?

@@ +210,5 @@
> +      return;
> +    }
> +
> +    // TODO 1120699
> +    // show appropriate empty/recording panels

I feel like just one TODO comment is enough for this method.

@@ +217,5 @@
> +
> +    // Only clear the checked state if there's nothing recording.
> +    if (!this.getItemForPredicate(e => e.isRecording)) {
> +      $("#record-button").removeAttribute("checked");
> +    }

Is all of this still necessary? Looks kind of ugly to control the recording button from here.

@@ +220,5 @@
> +      $("#record-button").removeAttribute("checked");
> +    }
> +
> +    // But don't leave it locked in any case.
> +    $("#record-button").removeAttribute("locked");

Ditto.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8547870 [details] [diff] [review]
1111004-multi-recordings.patch

Review of attachment 8547870 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/performance-controller.js
@@ +17,5 @@
>  devtools.lazyRequireGetter(this, "DevToolsUtils",
>    "devtools/toolkit/DevToolsUtils");
> +// Overloads ViewHelper's `Heritage`.
> +devtools.lazyRequireGetter(this, "Heritage",
> +  "sdk/core/heritage");

ViewHelper's Heritage is just a piecemeal copy of the sdk/core/heritage, with comments that should be removed at some point, and this one has more functionality that's used.

@@ +50,5 @@
>  const EVENTS = {
> +  // Emitted by the PerformanceController when a recording model
> +  // is selected. Also emitted by the RecordingsView, which is
> +  // re-emitted by the PerformanceController. Usually, should only listen
> +  // to the PerformanceController's event here.

Understood.

::: browser/devtools/performance/recording-model.js
@@ +6,5 @@
> +/**
> + * Model for a wholistic profile, containing start/stop times, profiling data, frames data,
> + * timeline (marker, tick, memory) data, and methods to start/stop recording.
> + */
> +const RecordingModel = Heritage.Class({

Originally used some properties of `Class`. No longer the case, so this can be just a basic Function.

::: browser/devtools/performance/views/recordings.js
@@ +97,5 @@
> +
> +    // If a label is specified (e.g due to a call to `console.profile`),
> +    // then try reusing a pre-existing recording item, if there is one.
> +    // This is symmetrical to how `this.handleRecordingEnded` works.
> +    if (recording.fromConsole()) {

Agreed

@@ +132,5 @@
> +    // then try reusing a pre-existing recording item, if there is one.
> +    // This is symmetrical to how `this.handleRecordingStarted` works.
> +    if (profileLabel) {
> +      recordingItem = this.getItemForAttachment(e =>
> +        e.profilerData.profileLabel == profileLabel);

We can -- just need to return recording model instead of recordingData everywhere, so small refactor, but should be fine (because you can easily get the recording data from the model)

@@ +217,5 @@
> +
> +    // Only clear the checked state if there's nothing recording.
> +    if (!this.getItemForPredicate(e => e.isRecording)) {
> +      $("#record-button").removeAttribute("checked");
> +    }

Yeah I'm not 100% sure what this is, was legacy from the `ui-recordings.js`

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1111004-multi-recordings.patch

Added tests -- pushed to try https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=063620213af7

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8547949 [details] [diff] [review]
1111004-multi-recordings.patch

Review of attachment 8547949 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/recording-model.js
@@ +225,5 @@
> +      let [markers] = data;
> +      Array.prototype.push.apply(this._markers, markers);
> +    }
> +    // Accumulate stack frames into an array.
> +    else if (eventName == "frames") {

Why not a switch statement ?

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8547949 [details] [diff] [review]
1111004-multi-recordings.patch

Review of attachment 8547949 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/performance/modules/recording-model.js
@@ +225,5 @@
> +      let [markers] = data;
> +      Array.prototype.push.apply(this._markers, markers);
> +    }
> +    // Accumulate stack frames into an array.
> +    else if (eventName == "frames") {

This just was here before in the main controller, no preference though, uploading new patch with a switch in it

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1111004-multi-recordings.patch

Added switch statement, try looks good

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/96b5f182f5aa

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/96b5f182f5aa