Recording configuration should be set by target support, not just prefs

RESOLVED FIXED in Firefox 42

Status

RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

41 Branch
Firefox 42
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 affected, firefox42 fixed)

Details

Attachments

(1 attachment)

Right now we set a recording's configuration based on prefs, like record memory, record framerate, record allocations etc.

Our views check if an actor is supported as well if the configuration was set, based on the client's pref. When importing a profile like this, it'll say it has "withMemory" support, even though there's no memory data on the original target. I'm not sure if we even cover that import case, since there's no target to feature detect.

What we should do is set the configuration via prefs like we do now, and then when setting on the RecordingModel via PerformanceFront, we should override if target does not support. Like:

new RecordingModel({ withMemory: this._memorySupported ? options.withMemory : false })

That way we don't have to have feature detection stuff littered throughout the client, and a recording's configuration is a single source of truth.

We need to do this before creating a real PerformanceActor.
Created attachment 8633234 [details] [diff] [review]
1175760-target-prefs.patch

Big piece required before moving things to PerformanceActor

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=588ba3504fdb
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8633234 - Flags: review?(vporof)
Comment on attachment 8633234 [details] [diff] [review]
1175760-target-prefs.patch

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

::: browser/devtools/performance/modules/logic/front.js
@@ +433,5 @@
> +   *
> +   * @param {object} options
> +   * @param {boolean}
> +   */
> +  _normalizeOptions: function (options) {

Maybe move this to recording-utils.js?

::: browser/devtools/performance/views/details.js
@@ +118,5 @@
>      }
>    }),
>  
>    /**
> +   * Takes a view name and determines if the current recording 

Nit: trailing ws
Attachment #8633234 - Flags: review?(vporof) → review+
https://hg.mozilla.org/mozilla-central/rev/277d6aaf2ba2
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.