The state management has probably gotten tricky enough to warrant Redux at this point.

Comment on attachment 8967859 [details] Bug 1454061 - Introduce redux to performance recording panel; https://reviewboard.mozilla.org/r/236558/#review242334 ::: devtools/client/performance-new/components/Perf.js (Diff revision 1) > - recordingState: AVAILABLE_TO_RECORD, > - recordingUnexpectedlyStopped: false > - }); > - } > - > - startRecording() { These are all moved into thunk action creators. ::: devtools/client/performance-new/components/Perf.js (Diff revision 1) > - stopProfilerAndDiscardProfile() { > - this.setState({ recordingState: REQUEST_TO_STOP_PROFILER }); > - this.props.perfFront.stopProfilerAndDiscardProfile(); > - } > - > - renderButton() { Moved into RecordingButton component. ::: devtools/client/performance-new/components/Perf.js:226 (Diff revision 1) > if (isSupportedPlatform === null) { > // We don't know yet if this is a supported platform, wait for a response. > return null; > } > > return div( This component no longer has much HTML any more, so it's a lot less fat than it used to be.

Comment on attachment 8967859 [details] Bug 1454061 - Introduce redux to performance recording panel; https://reviewboard.mozilla.org/r/236558/#review242338 ::: devtools/client/performance-new/initializer.js:16 (Diff revision 3) > baseURI: "resource://devtools/client/memory/", > window > }); > const Perf = require("devtools/client/performance-new/components/Perf"); > const Services = require("Services"); > -const { render, unmountComponentAtNode } = require("devtools/client/shared/vendor/react-dom"); > +const ReactDOM = require("devtools/client/shared/vendor/react-dom"); I stopped destructuring for more clarity on the mounting, since we're doing more complicated things now. ::: devtools/client/themes/perf.css:25 (Diff revision 3) > .perf-button-image { > vertical-align: text-top; > padding-inline-end: 4px; > } > > +.perf-button-container { I snuck this in, as the recording button was not centered anymore when it had an `additionalMessage`.

Comment on attachment 8967859 [details] Bug 1454061 - Introduce redux to performance recording panel; https://reviewboard.mozilla.org/r/236558/#review242568 Thanks, this looks really good! I left a few small nits, some are optional. Also I have a question for you about how to handle `receiveProfile` but solving it isn't in the scope of this bug. ::: devtools/client/performance-new/components/Perf.js:173 (Diff revision 3) > case LOCKED_BY_PRIVATE_BROWSING: > // The profiler is already locked, so we know about this already. > break; > > case RECORDING: > - this.setState({ > + changeRecordingState(AVAILABLE_TO_RECORD, true); optional nit: this boolean as parameter isn't very nice to the reader. I'd rather have a full object as parameter (possibly only for the second parameter). ::: devtools/client/performance-new/components/Settings.js:213 (Diff revision 3) > - this.setState(state => ({ > - features: {...state.features, [value]: checked} > - })); > + if (checked) { > + if (!features.includes(value)) { > + changeFeatures([value, ...features]); > + } > + } else { > + changeFeatures(features.filter(thread => thread !== value)); nit: you used `thread` as parameter name, it should be `feature`. ::: devtools/client/performance-new/components/Settings.js:276 (Diff revision 3) > - value: this.state.threads, > - onChange: this._handleThreadTextChange, > + value: this.state.temporaryThreadText === null > + ? this.props.threads nice, you use the implicit `Array.prototype.toString` to convert the array to a string, I like this! ::: devtools/client/performance-new/components/RecordingButton.js:43 (Diff revision 3) > + renderButton(buttonSettings) { > + const { disabled, label, onClick, additionalMessage } = buttonSettings; optional nit: Firefox can natively destructure parameters. ::: devtools/client/performance-new/initializer.js:34 (Diff revision 3) > function gInit(toolbox, perfFront) { > - const props = { > + const store = createStore(reducers); > + store.dispatch(actions.initializeStore({ > toolbox, > perfFront, > receiveProfile: profile => { I gave some thoughts about this `receiveProfile` function that's stored in the redux store. I wonder if this couldn't just be an action... I mean an action with a side effect (obviously), a thunk action indeed, that wouldn't actually dispatch any "raw" redux action. I think that would be cleaner, but... I don't know if it would be easily mockable. Note we have `sinon` in `testing/modules/sinon-2.3.2.js`, I think we could use it. I think we can file a follow-up about it to think about this specific issue separately, but I'm curious to have your opinion. ::: devtools/client/performance-new/store/actions.js:109 (Diff revision 3) > + dispatch(changeRecordingState(AVAILABLE_TO_RECORD)); > + }; > +}; > + > +/** > + * Stops the profiler, but does not try to retreive the profile. nit: retrieve ::: devtools/client/performance-new/store/reducers.js:85 (Diff revision 3) > + } > +} > + > +/** > + * The features that are enabled for the profiler. > + * @param {object} state nit: array, not object ::: devtools/client/performance-new/store/selectors.js:15 (Diff revision 3) > +const getRecordingSettings = state => { > + return { > + entries: getEntries(state), > + interval: getInterval(state), > + features: getFeatures(state), > + threads: getThreads(state), > + }; > +}; note: this is the only place where we should memoize the value, but because we don't use it to render anything (we use it only when `startRecording`) it's OK. That said, this function is more an utility function than a real selector, so if you don't plan on using it to render components, maybe this could belong to store/actions.js as a local utility function instead of exporting as a selector. Your call! ::: devtools/client/performance-new/test/chrome/head.js:136 (Diff revision 3) > "actor's spec. It should be added to the mock."); > } > }); > + > +/** > + * This is a helper function to correctlymount the Perf component, and provide nit: you miss a space in "correctlymount" ::: devtools/client/performance-new/utils.js:138 (Diff revision 3) > > /** > * Use some heuristics to guess at the overhead of the recording settings. > * @param {number} interval > * @param {number} bufferSize > - * @param {object} features - Map of the feature name to a boolean. > + * @param {array} features - Map of the feature name to a boolean. nit: It's not a `Map of the feature name to a boolean` anymore, rather it's an `array of the selected features`.

Comment on attachment 8967859 [details] Bug 1454061 - Introduce redux to performance recording panel; https://reviewboard.mozilla.org/r/236558/#review242568 > optional nit: this boolean as parameter isn't very nice to the reader. I'd rather have a full object as parameter (possibly only for the second parameter). Changing too: ``` changeRecordingState( AVAILABLE_TO_RECORD, { didRecordingUnexpectedlyStopped: true } ); ``` > nit: you used `thread` as parameter name, it should be `feature`. Thanks, copy pasta programming. > I gave some thoughts about this `receiveProfile` function that's stored in the redux store. I wonder if this couldn't just be an action... > > I mean an action with a side effect (obviously), a thunk action indeed, that wouldn't actually dispatch any "raw" redux action. > > I think that would be cleaner, but... I don't know if it would be easily mockable. Note we have `sinon` in `testing/modules/sinon-2.3.2.js`, I think we could use it. > > I think we can file a follow-up about it to think about this specific issue separately, but I'm curious to have your opinion. I'll add a comment in the code to explain my rationale, but I was trying to keep privileged code out of the main code paths, and only have it in the initializer.js file. That way our main client code acts like a normal web app. If there were some non-privileged side-effect I would agree with putting it into a thunk action. ``` /** * This function uses privileged APIs in order to take the profile, open up a new * 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. */ ``` > note: this is the only place where we should memoize the value, but because we don't use it to render anything (we use it only when `startRecording`) it's OK. > > That said, this function is more an utility function than a real selector, so if you don't plan on using it to render components, maybe this could belong to store/actions.js as a local utility function instead of exporting as a selector. Your call! Hmm.. I'd prefer it as a selector, but I'd also not like to load reselect as a dependency. I think I'll leave it for now. > nit: It's not a `Map of the feature name to a boolean` anymore, rather it's an `array of the selected features`. JSDoc strikes again!

Pushed by gtatum@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/734ac146303f Introduce redux to performance recording panel; r=julienw