1077447 - Add manual recording to PerformanceFront

Status: RESOLVED FIXED

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

Description

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

Crude connections to UI, tests that recording data comes in

Comment 1

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

Attachment: 1077447-recording-ui-perf.patch

Comment 2

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

Comment on attachment 8513105 [details] [diff] [review]
1077447-recording-ui-perf.patch

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

Excellent! r+ with nits below.

::: browser/devtools/performance/performance.js
@@ +88,5 @@
>    destroy: function() {
> +    this._recordButton.removeEventListener("mouseup", this._onRecordButtonClick);
> +  },
> +
> +  startRecording: Task.async(function *() {

Document things please.

@@ +98,5 @@
> +    let results = yield gFront.stopRecording();
> +    this.emit(EVENTS.RECORDING_STOPPED, results);
> +  }),
> +
> +  _onRecordButtonClick: function (e) {

I'm assuming all of this will be moved to a PerformanceView singleton at some point?

@@ +100,5 @@
> +  }),
> +
> +  _onRecordButtonClick: function (e) {
> +    if (this._recordButton.hasAttribute("checked")) {
> +      this._recordButton.removeAttribute("checked");

Should also set the 'locked' state, so the button can't be pressed *while* a recording is being started or stopped (which might take some time). So:

start:
  1. setAttribute("checked")
  2. setAttribute("locked")
  3. yield gFront.startRecording()
  4. removeAttribute("locked")

stop:
  1. removeAttribute("checked")
  2. setAttribute("locked")
  3. yield gFront.stopRecording()
  4. removeAttribute("locked")

Test for that too.

::: browser/devtools/performance/test/browser_perf-ui-recording.js
@@ +18,5 @@
> +  yield startRecording(panel);
> +  busyWait(WAIT_TIME); // allow the profiler module to sample some cpu activity
> +
> +  ok(nsIProfilerModule.IsActive(),
> +    "The built-in profiler module should still be active.");

This should read "The built-in profiler mdoule should now be active".

@@ +20,5 @@
> +
> +  ok(nsIProfilerModule.IsActive(),
> +    "The built-in profiler module should still be active.");
> +
> +  yield stopRecording(panel);

How about checking if the profiler module is still active after stopping the recording too.
And now "[...] should still be active".

Comment 3

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

Attachment: 1077447-recording-ui-perf.patch

Resubmitting for review, since a bit more changes:

* fixed nits
* added documentation, button "lock"
* separate view functions from controller functions, as per our discussion (other things can trigger stop/start recording)

Comment 4

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

Comment on attachment 8514380 [details] [diff] [review]
1077447-recording-ui-perf.patch

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

Sweet!

::: browser/devtools/performance/controller.js
@@ +80,5 @@
> +   * Listen for events emitted by the current tab target and
> +   * main UI events.
> +   */
> +  initialize: function() {
> +

Nit: unnecessary newline.

@@ +85,5 @@
> +    this.startRecording = this.startRecording.bind(this);
> +    this.stopRecording = this.stopRecording.bind(this);
> +
> +    PerformanceView.on(EVENTS.UI_START_RECORDING, this.startRecording);
> +    PerformanceView.on(EVENTS.UI_STOP_RECORDING, this.stopRecording);

Yeah, this is much better!

@@ +98,5 @@
> +  },
> +
> +  /**
> +   * Starts recording via the PerformanceFront. Emits `EVENTS.RECORDING_STARTED`
> +   * when the front is starting to record. Called via PerformanceView's record button.

I think you should remove 'called via ...', since this won't always be the case.

@@ +118,5 @@
> +
> +/**
> + * Convenient way of emitting events from the controller.
> + */
> +EventEmitter.decorate(PerformanceController);

So we're not decorating the window anymore? Ok, that's cool.

::: browser/devtools/performance/test/head.js
@@ +213,5 @@
> +  let started = panel.panelWin.PerformanceController.once(win.EVENTS.RECORDING_STARTED);
> +  let button = win.$("#record-button");
> +
> +  ok(!button.hasAttribute("checked"),
> +    "The record button should not be checked yet.");

How about also checking that the button isn't locked as well?

@@ +227,5 @@
> +
> +  yield started;
> +
> +  ok(!button.hasAttribute("locked"),
> +    "The record button should not be locked.");

Check the 'checked' state too, to be symmetrical with `stopRecording`

@@ +237,5 @@
> +  let ended = panel.panelWin.PerformanceController.once(win.EVENTS.RECORDING_STOPPED);
> +  let button = win.$("#record-button");
> +
> +  ok(button.hasAttribute("checked"),
> +    "The record button should already be checked.");

Ditto. Button shouldn't be locked when pressing it.

::: browser/devtools/performance/views/main.js
@@ +6,5 @@
> +/**
> + * Master view handler for the performance tool.
> + */
> +let PerformanceView = {
> +  initialize: function () {

Document initialize and destroy.

Comment 5

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

Attachment: 1077447-recording-ui-perf.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=158f75c2b622

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b7b32eafddfd