Open Bug 1196945 Opened 5 years ago Updated 1 year ago

When changing recordings, reset selection

Categories

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

defect

Tracking

(Not tracked)

People

(Reporter: jsantell, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Attached image resetselection.gif
When going from one recording to another, the selection is removed from the graph, but the second recording is still scoped by the previous selection, which can cause all sorts of problems (time range could be out of bounds, or not rendering the markers that you'd expect to be the full recording)

Ran into this via test failures for bug 1153477. See attachment.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Depends on: 1197031
No longer blocks: 1153477
Attached patch 1196945.patch (obsolete) — Splinter Review
Attachment #8654552 - Flags: review?(jsantell)
Looks like this gets close -- the visual selection is dropped but the range selected of the details views are still scoped by the selection: http://i.imgur.com/WmCPPsU.gif

You can see when switching to the second recording, the call tree is still scoped by a small window, then when clicking again to reset it, the call tree is populated
oh! This is working fine in my machine though. The selection is reset as soon as the recordings changed ,not on mouse click event :\.
Yes not working for call tree
Attached patch calltree-reset.patch (obsolete) — Splinter Review
This works for call tree view and uses a simple flag logic.
Attachment #8654945 - Flags: review?(jsantell)
Comment on attachment 8654945 [details] [diff] [review]
calltree-reset.patch

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

Does this pass tests? I have a feeling this if it works, will only work on the call tree; not the other details views. Maybe if something was similar in the master details view (performance/views/details.js), where it reset the interval before firing the on recording change event
Attachment #8654945 - Flags: review?(jsantell)
Attachment #8654552 - Flags: review?(jsantell)
Attached patch reset_interval_draft.patch (obsolete) — Splinter Review
Made few changes in details abstract subview. Working for all the views.
Attachment #8654552 - Attachment is obsolete: true
Attachment #8654945 - Attachment is obsolete: true
Attachment #8655688 - Flags: review?(jsantell)
Comment on attachment 8655688 [details] [diff] [review]
reset_interval_draft.patch

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

Pushed patch to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f09e7d6c0d3e
The browser_perf-range-changed-render.js test fails saying waterfall rendered 3 times ,got 4 .
Locally? The try test looks good, but I'm suspicious of it -- I can imagine this being fine in the past, lots of architectural changes have occurred. What are you asking?
Comment on attachment 8655688 [details] [diff] [review]
reset_interval_draft.patch

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

::: browser/devtools/performance/views/details-abstract-subview.js
@@ +14,5 @@
>      this._onRecordingStoppedOrSelected = this._onRecordingStoppedOrSelected.bind(this);
>      this._onOverviewRangeChange = this._onOverviewRangeChange.bind(this);
>      this._onDetailsViewSelected = this._onDetailsViewSelected.bind(this);
>      this._onPrefChanged = this._onPrefChanged.bind(this);
> +    this._interval=null;

spaces on both sides of assignment operator

@@ +34,5 @@
>      PerformanceController.off(EVENTS.PREF_CHANGED, this._onPrefChanged);
>      OverviewView.off(EVENTS.OVERVIEW_RANGE_SELECTED, this._onOverviewRangeChange);
>      DetailsView.off(EVENTS.DETAILS_VIEW_SELECTED, this._onDetailsViewSelected);
>    },
> +  

remove whitespace

@@ +85,5 @@
>    /**
>     * Called when recording stops or is selected.
>     */
>    _onRecordingStoppedOrSelected: function(_, state, recording) {
> +    this._interval=OverviewView.getTimeInterval();

This should probably just be a local variable -- no reason to store it as a property if it's only calculated and used in this function.

Actually there's no reason to have this at all -- see below

@@ +97,5 @@
>      if (!recording || !recording.isCompleted()) {
>        return;
>      }
> +    if (recording.isCompleted()) {
> +      this._interval.startTime=0;

spaces on both sides of assignment operator here and everywhere else

@@ +99,5 @@
>      }
> +    if (recording.isCompleted()) {
> +      this._interval.startTime=0;
> +      this._interval.endTime=recording.getDuration();
> +      this.render(this._interval);

This would force rerender on every details view regardless of if it was the one selected if the recording is finished

@@ +105,2 @@
>      if (DetailsView.isViewSelected(this) || this.canUpdateWhileHidden) {
> +      this.render(this._interval);

I think just changing this to be this.render({ startTime: 0, endTime: recording.getDuration() }); should do it
Attachment #8655688 - Flags: review?(jsantell)
Attached image selection.gif
Yes I figured, that there is no need to put recording.isCompleted() check , however rendering in here 'DetailsView.isViewSelected', there is a lag between unnselecting the previous set region while changing recordings.
Attachment #8655688 - Attachment is obsolete: true
No longer blocks: perf-tools-fx43
No longer blocks: perf-tools-fx44
Assignee: jsantell → nobody
Status: ASSIGNED → NEW
Triaging. Filter on ADRENOCORTICOTROPIC (yes).
Priority: -- → P3
Version: 41 Branch → unspecified
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.