Closed Bug 1102350 Opened 7 years ago Closed 7 years ago

Implement inverted call tree in new performance tool

Categories

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

36 Branch
x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Blocks: perf-tool-v2
No longer blocks: 1102347
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Depends on: 1121700
Attached patch 1102350-invert-call-tree.patch (obsolete) — Splinter Review
Also fixes bug 1087473 (invert call tree by default) and bug 1087477 (back invert by a pref)

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7afcf26c944e
Attachment #8550455 - Flags: review?(vporof)
Comment on attachment 8550455 [details] [diff] [review]
1102350-invert-call-tree.patch

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

Would love to see a ToolbarView for the options view.

::: browser/app/profile/firefox.js
@@ +1434,5 @@
>  pref("devtools.profiler.ui.show-platform-data", false);
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
> +// The default Performance UI settings
> +pref("devtools.performance.invert-call-tree", true);

Group this with devtools.performance.ui.show-timeline-memory, and rename it to devtools.performance.ui.invert-call-tree

::: browser/devtools/performance/performance-controller.js
@@ +173,5 @@
>     * Listen for events emitted by the current tab target and
>     * main UI events.
>     */
> +  initialize: Task.async(function* () {
> +    this.optionsView = new OptionsView({

Kinda weird to have an options view inside a controller. How about creating a ToolbarView, alongside the OverviewView etc.?

@@ +175,5 @@
>     */
> +  initialize: Task.async(function* () {
> +    this.optionsView = new OptionsView({
> +      branchName: BRANCH_NAME,
> +      window: window,

Instead of passing in the window, you can get it via menupopup.ownerDocument.defaultView. Would make the OptionsView smarter.

::: browser/devtools/performance/performance.xul
@@ +65,5 @@
>                           class="devtools-toolbarbutton"
>                           data-view="flamegraph" />
>          </hbox>
>          <spacer flex="1"></spacer>
> +        <hbox id="performance-toolbar-control-optionss" class="devtools-toolbarbutton-group">

optionssssss
Attachment #8550455 - Flags: review?(vporof) → review-
Comment on attachment 8550455 [details] [diff] [review]
1102350-invert-call-tree.patch

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

::: browser/devtools/performance/performance-controller.js
@@ +175,5 @@
>     */
> +  initialize: Task.async(function* () {
> +    this.optionsView = new OptionsView({
> +      branchName: BRANCH_NAME,
> +      window: window,

Debated the same thing with a "PerformanceOptionsView", but it was pretty tiny, which I guess is fine. Done.

A+ on the ownerDocument.defaultView
Comment on attachment 8550568 [details] [diff] [review]
1102350-invert-call-tree.patch

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

::: browser/app/profile/firefox.js
@@ +1434,5 @@
>  pref("devtools.profiler.ui.show-platform-data", false);
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
> +// The default Performance UI settings
> +pref("devtools.performance.invert-call-tree", true);

Previous comment: devtools.performance.ui.invert-call-tree :)

::: browser/devtools/performance/performance-controller.js
@@ +179,5 @@
>      this.importRecording = this.importRecording.bind(this);
>      this.exportRecording = this.exportRecording.bind(this);
>      this._onTimelineData = this._onTimelineData.bind(this);
>      this._onRecordingSelectFromView = this._onRecordingSelectFromView.bind(this);
> +    this._onPrefChanged = this._onPrefChanged.bind(this);

How does this clash with PrefObserver in this file btw? Is that deprecated now? Should remove it if so.
Attachment #8550568 - Flags: review?(vporof) → review+
Comment on attachment 8550568 [details] [diff] [review]
1102350-invert-call-tree.patch

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

::: browser/app/profile/firefox.js
@@ +1434,5 @@
>  pref("devtools.profiler.ui.show-platform-data", false);
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
> +// The default Performance UI settings
> +pref("devtools.performance.invert-call-tree", true);

Ah yes adding that now

::: browser/devtools/performance/performance-controller.js
@@ +179,5 @@
>      this.importRecording = this.importRecording.bind(this);
>      this.exportRecording = this.exportRecording.bind(this);
>      this._onTimelineData = this._onTimelineData.bind(this);
>      this._onRecordingSelectFromView = this._onRecordingSelectFromView.bind(this);
> +    this._onPrefChanged = this._onPrefChanged.bind(this);

It doesn't clash as this pref isn't tracked by that. Leaving it in for now, and made bug 1122639 to migrate these properties over to the new pref/expose on the options button
Attached patch 1102350-invert-call-tree.patch (obsolete) — Splinter Review
All good to go
Attachment #8550568 - Attachment is obsolete: true
Attachment #8550644 - Flags: review+
Hi,

this patch failed to apply:

applying 1102350-invert-call-tree.patch
patching file browser/devtools/performance/views/details-call-tree.js
Hunk #1 FAILED at 8
Hunk #2 FAILED at 44
2 out of 4 hunks FAILED -- saving rejects to file browser/devtools/performance/views/details-call-tree.js.rej
patching file browser/devtools/performance/views/details-flamegraph.js
Hunk #1 FAILED at 49
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/performance/views/details-flamegraph.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 1102350-invert-call-tree.patch

could you take a look, thanks!
Flags: needinfo?(jsantell)
Keywords: checkin-needed
Here's a rebased patch
Attachment #8550644 - Attachment is obsolete: true
Flags: needinfo?(jsantell)
Attachment #8551416 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a74557453c80
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
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.