Closed Bug 1454061 Opened 6 years ago Closed 6 years ago

Introduce redux to performance recording panel

Categories

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

60 Branch
defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: gregtatum, Assigned: gregtatum)

References

Details

Attachments

(1 file)

The state management has probably gotten tricky enough to warrant Redux at this point.
Assignee: nobody → gtatum
Priority: -- → P1
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`.
Attachment #8967859 - Flags: review?(felash) → review+
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!
Blocks: 1447338
Pushed by gtatum@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/734ac146303f
Introduce redux to performance recording panel; r=julienw
https://hg.mozilla.org/mozilla-central/rev/734ac146303f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.