The first implementation will always reset the values when opening the panel again. It would be nice to persist these settings between sessions. However, we should take care that this experience works well. I could see confusion if someone tweaked the settings from the defaults, and those persisted for ever. For instance someone could profile Android, and change the settings, then get confused profiling locally. One mitigation could be having separate preferences for remote targets. This could be done with the `target.isRemote` setting. Perhaps we should notify the user in the UI if that their values are different from the defaults. This would help avoid a case for instance where someone accidentally turned off stack walking.

Comment on attachment 8969049 [details] Bug 1453014 - Persist the recording settings to preferences; https://reviewboard.mozilla.org/r/237732/#review243662 I left a few small comments. Especially I think we should reuse the existing prefs where they exist already. I'd also like a simple test to at least check that the prefs are written and read. In addition I see you're not observing the prefs in this patch, is this something we should do? I haven't tried the patch for real, I'll assume you did. r=me with these changes. ::: devtools/client/performance-new/browser.js:60 (Diff revision 2) > + let isFullOfStrings = true; > + for (const feature of array) { > + isFullOfStrings = isFullOfStrings && typeof feature === "string"; > + } optional nit: I would have used `array.every`: ```javascript const isFullOfStrings = array.every( feature => typeof feature === "string" ); ``` ::: devtools/client/performance-new/browser.js:99 (Diff revision 2) > + await _getIntPref( > + preferenceFront, > + `devtools.performance.recording.entries`, > + defaultSettings.entries > + ), > + await _getIntPref( > + preferenceFront, > + `devtools.performance.recording.interval`, > + defaultSettings.interval > + ), > + await _getArrayOfStringsPref( > + preferenceFront, > + `devtools.performance.recording.features`, > + defaultSettings.features > + ), > + await _getArrayOfStringsPref( > + preferenceFront, > + `devtools.performance.recording.threads`, > + defaultSettings.threads > + ), You don't want to use `await` here. The `_getXXXPref` functions return promises, so you need to use them directly, instead of calling `await` on them. This defeats your use of `Promise.all` as your current code will sequentially wait for the result of `_getXXXPref` functions. (of course we still need the `await` in front of `Promise.all`, this isn't about this) ::: devtools/client/performance-new/browser.js:132 (Diff revision 2) > +async function setRecordingPreferences(preferenceFront, settings) { > + return Promise.all([ nit: Like it is now, this function shouldn't be async as it returns a promise directly and doesn't use `await` at all. But maybe better is that you could replace `return` by `await` below, because we don't really need the result of `Promise.all`, we only want to wait for it. ::: devtools/client/performance-new/browser.js:134 (Diff revision 2) > + preferenceFront.setIntPref( > + `devtools.performance.recording.entries`, > + settings.entries > + ), > + preferenceFront.setIntPref( > + `devtools.performance.recording.interval`, > + settings.interval > + ), note there are existing prefs for these: `devtools.performance.profiler.buffer-size` and `devtools.performance.profiler.sample-frequency-khz` I personally think we should reuse them, because 1. their naming is much better and 2. they're used in the old panel too. I also like the prefix `devtools.performance.profiler` better than the one you used `devtools.performance.recording`. `recording` is much less precise: after all we record the memory allocations too... While `profiler` is really the part we're dealing with. ::: devtools/client/performance-new/browser.js:142 (Diff revision 2) > + preferenceFront.setCharPref( > + `devtools.performance.recording.features`, > + JSON.stringify(settings.features) > + ), > + preferenceFront.setCharPref( > + `devtools.performance.recording.threads`, > + JSON.stringify(settings.threads) > + ) At first I thought that using a JSON serialization would make it less easy to manipulate directly in about:config. But I see that others are doing it and about:config seems to do a special handling of these values... so I'm fine with it. ::: devtools/client/performance-new/initializer.js:34 (Diff revision 2) > * Initialize the panel by creating a redux store, and render the root component. > * > * @param toolbox - The toolbox > * @param perfFront - The Perf actor's front. Used to start and stop recordings. > */ > -function gInit(toolbox, perfFront) { > +async function gInit(toolbox, perfFront, preferenceFront) { Why do you transform this to an `async` function here? I feel like it's a left-over from a previous implementation... ::: devtools/client/performance-new/initializer.js:49 (Diff revision 2) > - * tab, and then inject it into perf.html. In order to provide a clear separation > - * in the codebase between privileged and non-privileged code, this function is > - * defined in initializer.js, and injected into the the normal component. All of > - * the React components and Redux store behave as normal unprivileged web components. > - */ > - receiveProfile: profile => { > + // to what's in the target's preferences. This way the preferences are stored > + // on the target. This could be useful for something like Android where you might > + // want to tweak the settings. > + recordingSettingsFromPreferences: await getRecordingPreferences( > + preferenceFront, > + selectors.getRecordingSettings(store.getState()) If you want the default value, maybe you should just pass nothing or `undefined` instead of the current state? ::: devtools/client/performance-new/store/actions.js:44 (Diff revision 2) > + * Dispatch the given action, and then update the recording settings. > + * @param {object} action > + */ > +function _dispatchAndUpdatePreferences(action) { > + return (dispatch, getState) => { > + dispatch(action); Note that you assume the action is a vanilla action (not a thunk action) because you assume the state will be synchronously changed. This is fine for now because this is what you do, but please add a comment about it nevertheless. More robust solutions could be using a redux middleware (put it last in the middleware chain so that this avoids asynchronous actions) or subscribing to store changes. ::: devtools/client/performance-new/test/chrome/head.js:163 (Diff revision 2) > store.dispatch(actions.initializeStore({ > toolbox: toolboxMock, > perfFront, > receiveProfile: receiveProfileMock, > + recordingSettingsFromPreferences: selectors.getRecordingSettings(store.getState()), > + setRecordingPreferences: () => {} do you plan to add more test about this? Like checking the prefs are written, or that when you change the prefs they're actually taken into account when recording?

Comment on attachment 8969049 [details] Bug 1453014 - Persist the recording settings to preferences; https://reviewboard.mozilla.org/r/237732/#review243662 I'm not observing the changes, as they are persisting preferences between sessions only, which is less code to maintain than observing them. > note there are existing prefs for these: > `devtools.performance.profiler.buffer-size` and `devtools.performance.profiler.sample-frequency-khz` > > I personally think we should reuse them, because 1. their naming is much better and 2. they're used in the old panel too. > > I also like the prefix `devtools.performance.profiler` better than the one you used `devtools.performance.recording`. `recording` is much less precise: after all we record the memory allocations too... While `profiler` is really the part we're dealing with. I'd prefer to keep it as is for the following reasons: * The wording matches the terminology used by the actors and nsIProfiler interface. * It doesn't conflate settings from the old panel with the new one for surprising behaviors. * The values are for persisting state in the UI, and the fact that they are exposed in about:config is a side-effect, so I'd prefer to optimize for easier to read code. > Why do you transform this to an `async` function here? I feel like it's a left-over from a previous implementation... For the line `recordingSettingsFromPreferences: await getRecordingPreferences`, > If you want the default value, maybe you should just pass nothing or `undefined` instead of the current state? I don't believe that would work, as selectors are monomorphic of the type `State => selectedOrDerivedValue`, and don't accept a null value. > Note that you assume the action is a vanilla action (not a thunk action) because you assume the state will be synchronously changed. > This is fine for now because this is what you do, but please add a comment about it nevertheless. > > More robust solutions could be using a redux middleware (put it last in the middleware chain so that this avoids asynchronous actions) or subscribing to store changes. It now throws an error if a thunk action is passed to it.

Comment on attachment 8969049 [details] Bug 1453014 - Persist the recording settings to preferences; https://reviewboard.mozilla.org/r/237732/#review243662 Follow-up for tests: https://bugzilla.mozilla.org/show_bug.cgi?id=1456972

Pushed by gtatum@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/252a244dc3d4 Persist the recording settings to preferences; r=julienw

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #4) > Comment on attachment 8969049 [details] > > Why do you transform this to an `async` function here? I feel like it's a left-over from a previous implementation... > > For the line `recordingSettingsFromPreferences: await getRecordingPreferences`, Right, I missed it, thanks. > > If you want the default value, maybe you should just pass nothing or `undefined` instead of the current state? > > I don't believe that would work, as selectors are monomorphic of the type > `State => selectedOrDerivedValue`, and don't accept a null value. Right I was confused with the reducers here... Sorry about that.