Closed Bug 1120800 Opened 7 years ago Closed 7 years ago

New Performance tool should clear out its profiles when 'clear' clicked

Categories

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

37 Branch
x86
macOS
defect
Not set
normal

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 1 obsolete file)

Like previous Profiler.
Assignee: nobody → jsantell
Depends on: 1120699
Status: NEW → ASSIGNED
Attached patch 1120800-clear-perf.patch (obsolete) — Splinter Review
Other non-obvious changes are the PerformanceView now no longer directly sets the lock/unlock status of the record buttons, as the controller can stop recording directly (like clearing out all recordings, including stopping the current recording), using the WILL_START|STOP events.
Attachment #8559382 - Flags: review?(vporof)
Comment on attachment 8559382 [details] [diff] [review]
1120800-clear-perf.patch

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

::: browser/devtools/performance/performance-controller.js
@@ +82,5 @@
>    RECORDING_WILL_START: "Performance:RecordingWillStart",
>    RECORDING_WILL_STOP: "Performance:RecordingWillStop",
>  
> +  // When recordings have been cleared out
> +  CLEAR_RECORDINGS: "Performance:ClearRecordings",

s/CLEAR_RECORDINGS/RECORDINGS_CLEARED/, or see the comment below.

@@ +273,5 @@
> +    }
> +
> +    this._recordings.length = 0;
> +    this.setCurrentRecording(null);
> +    this.emit(EVENTS.CLEAR_RECORDINGS);

Is EVENTS.CLEAR_RECORDINGS really necessary? Can't we just yield on UI_CLEAR_RECORDINGS and RECORDING_SELECTED?

::: browser/devtools/performance/performance-view.js
@@ +131,5 @@
>    /**
> +   * Fired when a recording is starting, but not yet completed.
> +   */
> +  _onRecordingWillStart: function () {
> +    this._lockRecordButton();

Isn't it redundant to lockRecordButton() both on both EVENTS.RECORDING_WILL_START and EVENTS.RECORDING_STARTED?

::: browser/devtools/performance/test/browser_perf-clear-02.js
@@ +13,5 @@
> +  yield startRecording(panel);
> +  yield stopRecording(panel);
> +
> +  yield startRecording(panel);
> +

Maybe yield on RECORDING_STOPPED here?

::: browser/devtools/performance/views/recordings.js
@@ +167,5 @@
>  
>    /**
> +   * Clears out all recordings.
> +   */
> +  _onClearRecordings: function () {

s/.../_onRecordingsCleared/
Attachment #8559382 - Flags: review?(vporof) → review+
Comment on attachment 8559382 [details] [diff] [review]
1120800-clear-perf.patch

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

::: browser/devtools/performance/performance-controller.js
@@ +267,5 @@
> +   */
> +  clearRecordings: Task.async(function* () {
> +    let latest = this._getLatestRecording();
> +
> +    if (latest && latest.isRecording()) {

Should be careful in future patches, because we can still be recording something even if the latest recording is finished. Also, everything will need to be stopped, not just the last recording.
Currently, only the last recording can be in progress. Even when we add console hooks, other recordings can be recorded, but only one manually. So I guess we should change _getLatestRecording to something that indicates this is the recording model in progress (the comments on _getLatestRecording indicate that, but do not check if the model is spawned via console).

I'll make a note of this in bug 1077464 to track that use case

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d4d775dfb01
Comment on attachment 8559382 [details] [diff] [review]
1120800-clear-perf.patch

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

::: browser/devtools/performance/performance-controller.js
@@ +273,5 @@
> +    }
> +
> +    this._recordings.length = 0;
> +    this.setCurrentRecording(null);
> +    this.emit(EVENTS.CLEAR_RECORDINGS);

We could, but right now, we have a clear separation of all UI_* events (only emitted by the PerformanceView, and handled by the controller, which all other views subscribe to). This lets us hook into a final event, and currently it's immediately after RECORDING_SELECTED, but that's not necessarily true in the future

::: browser/devtools/performance/performance-view.js
@@ +131,5 @@
>    /**
> +   * Fired when a recording is starting, but not yet completed.
> +   */
> +  _onRecordingWillStart: function () {
> +    this._lockRecordButton();

I don't understand -- the WILL_START|STOP events lock the button, and STARTED|STOPPED unlock the button currently
Attachment #8559382 - Attachment is obsolete: true
Attachment #8559513 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/525132c2c642
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.