Closed Bug 1130204 Opened 5 years ago Closed 5 years ago

Should hide features when not supported by the server in perf++

Categories

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

37 Branch
x86
macOS
defect
Not set

Tracking

(firefox38 fixed)

RESOLVED FIXED
Firefox 38
Tracking Status
firefox38 --- fixed

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(1 file, 1 obsolete file)

When connecting to Fx2.1, for example, we don't have a real TimelineActor, so we should hide the overview.

Fx 2.0, 2.1:
Hide all overview waterfall, JS Flamegraph, Memory Call Tree, Memory Flamegraph, possibly 'enable framerate/memory' options

Fx 2.2:
Figure this out in bug 1130202
No longer blocks: 1130200, 1130202
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Some bad compatibility bugs (if timeline is disabled, memory will never be enabled, as the actor cannot detect the methods) -- fixes are in bug 1130202
Depends on: 1130202
Should be the last component for supporting different targets. This'll hide everything but js calltree (including overview) on 2.0/2.1 b2g, and on b2g 2.2, disable the memory tree/flame view

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7246dab0d7d7
Attachment #8564329 - Flags: review?(vporof)
Comment on attachment 8564329 [details] [diff] [review]
1130204-hide-perf-views-support.patch

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

::: browser/devtools/performance/test/head.js
@@ +265,5 @@
>  function mousedown (win, button) {
>    EventUtils.sendMouseEvent({ type: "mousedown" }, button, win);
>  }
>  
> +function* startRecording(panel, waitForOverview=true) {

Maybe have waitForOverview as a property in an option, to make calls to this function more obvious.

startRecording(panel, { waitForOverview: true });

@@ +303,5 @@
>    ok(!button.hasAttribute("locked"),
>      "The record button should not be locked.");
>  }
>  
> +function* stopRecording(panel, waitForOverview=true) {

Ditto

::: browser/devtools/performance/views/details.js
@@ +11,5 @@
>    /**
>     * Name to node+object mapping of subviews.
>     */
>    components: {
> +    "waterfall": { id: "waterfall-view", view: WaterfallView, support: "timeline" },

s/support/requires maybe?

Also, I think it'd be best to have this an array, instead of a string. Let's not make the assumption that only one actor is needed, or that another actor is available implicitly based on another one.

@@ +16,4 @@
>      "js-calltree": { id: "js-calltree-view", view: JsCallTreeView },
> +    "js-flamegraph": { id: "js-flamegraph-view", view: JsFlameGraphView, support: "timeline" },
> +    "memory-calltree": { id: "memory-calltree-view", view: MemoryCallTreeView, support: "memory", pref: "enable-memory" },
> +    "memory-flamegraph": { id: "memory-flamegraph-view", view: MemoryFlameGraphView, support: "memory", pref: "enable-memory" }

e.g. here you should have [timeline, memory]

@@ +70,5 @@
>        let recording = PerformanceController.getCurrentRecording();
>  
>        let isRecorded = recording && !recording.isRecording();
> +      let isEnabled = (!pref || PerformanceController.getPref(pref)) &&
> +                      (!support || !mocks[support]);

Would help readability to have this split out as `isSupported` and `isEnabled`, and use both everywhere.
Attachment #8564329 - Flags: review?(vporof) → review+
comments all addressed
Attachment #8564329 - Attachment is obsolete: true
Attachment #8564584 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/4d43899972e7
Status: ASSIGNED → RESOLVED
Closed: 5 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.