1450709 - Surface profile recording settings in the new recording panel UI

Status: RESOLVED FIXED

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

Description

[Greg Tatum [:gregtatum]]

Attachment: recording-panel.jpg

We should surface profile recording settings in the new recording panel with some UI. I attached an example mock-up, but I will solicit team feedback outside of Bugzilla.

Comment 2

[Greg Tatum [:gregtatum]]

Attachment: Bug 1450709 - Add profile recording settings to the recording panel;

Review commit: https://reviewboard.mozilla.org/r/235380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235380/

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

https://reviewboard.mozilla.org/r/235380/#review241076


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/performance-new/components/Perf.js:254
(Diff revision 1)
>  
>    startRecording() {
> +    const settings = this.settings;
> +    if (!settings) {
> +      console.error("Expected the PerfSettings panel to be rendered and available.");
> +      return

Error: Missing semicolon. [eslint: semi]

Comment 4

[Greg Tatum [:gregtatum]]

julien: I have not written tests for the new component yet. I plan on doing that before I merge as a separate commit, although I wanted to get review feedback before further inflating my lines of code changes.

Comment 5

[Greg Tatum [:gregtatum]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235380/diff/1-2/

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

https://reviewboard.mozilla.org/r/235380/#review241366

Thanks, this looks good!

I left a lot of comments but most are small or suggestions.
The most important is how we get the settings, which is not very React-y. We can do better even without Redux :)

::: devtools/client/performance-new/components/Perf.js:262
(Diff revision 2)
> +      // Pull out the recording settings from the child component. This approach avoids
> +      // using Redux as a state manager.
> +      settings.getRecordingSettings()

The other solution is to "lift the state up" like they say in [React documentation](https://reactjs.org/docs/lifting-state-up.html).

1. Keep the recordingSettings here
2. pass it to `PerfSettings` as a prop, along with a `onSettingsChange` callback.
3. `PerfSettings` uses it directly to display its state (instead of using its internal state), and call the `onSettingsChange` for any change with the result of `getRecordingSettings` as parameter.

I think this is a better approach because this is less coupled. This is also what's recommended in the React documentation.

::: devtools/client/performance-new/components/Perf.js:358
(Diff revision 2)
> +
> +  handleLinkClick(event) {
> +    const { tab } = this.props.target;
> +    if (tab) {
> +      const browserWin = tab.ownerDocument.defaultView;
> +      browserWin.openUILinkIn(event.target.value, "tab");

There's a link utility in devtools, see https://bugzilla.mozilla.org/show_bug.cgi?id=1452678 for more information.

We need the "toolbox" as parameter, I'm not sure what it is. But then maybe we don't need to pass `target` around.

::: devtools/client/performance-new/components/Perf.js:366
(Diff revision 2)
> +
> +  render() {
> +    const { isSupportedPlatform } = this.state;
> +
> +    // Handle the cases of platform support.
> +    if (isSupportedPlatform === null) {

I miss the previous comment that was here. As a reminder it's:
```
// We don't know yet if this is a supported platform, wait for a response.
```

::: devtools/client/performance-new/components/Perf.js:380
(Diff revision 2)
> +            // Implement links as buttons to avoid any risk of loading the link in the
> +            // the panel.

Is this how we do it in other places? This is a bit sad...

Wouldn't a `<a target="_blank">` work ?

Otherwise [the code in devtools reps](https://github.com/devtools-html/devtools-core/blob/72504532bd95735dfa124214a3e694148449d5f0/packages/devtools-reps/src/reps/string.js#L195-L205) uses a link. I think using a link is better for accessibility.

::: devtools/client/performance-new/components/PerfSettings.js:158
(Diff revision 2)
> +    for (const [name, isSet] of Object.entries(this.state.features)) {
> +      if (isSet) {
> +        features.push(name);
> +      }
> +    }

nit: Object.entries returns an array, so this may be more idiomatic:
```javascript
const features = Object.entries(this.state.features)
  .filter(([, isSet]) => isSet)
  .map(([name]) => name);
```

But as said below I think `features` should hold the array already, and in that case you just need to return it.

::: devtools/client/performance-new/components/PerfSettings.js:189
(Diff revision 2)
> +          className:
> +          `perf-settings-notch perf-settings-notch-${level} ` +
> +            `perf-settings-notch-${active}`

nit: just wondering if you could use a multiline string here. I know we can't use our neat `common-tags` library here, but maybe React and/or Gecko accept class names with new lines.

::: devtools/client/performance-new/components/PerfSettings.js:210
(Diff revision 2)
> +          threadsList.push(value);
> +        }
> +      } else {
> +        threadsList = threadsList.filter(thread => thread !== value);
> +      }
> +      return { threads: threadsList.join(",") };

I'm not completely convinced that we should hold the threads as a string, rather than an array. But maybe this makes things easier for the "free form" possibility. So I'll leave it to your judgment.

::: devtools/client/performance-new/components/PerfSettings.js:218
(Diff revision 2)
> +
> +  _handleFeaturesCheckboxChange(event) {
> +    const { checked, value }  = event.target;
> +
> +    this.setState(state => ({
> +      features: Object.assign({}, state.features, { [value]: checked })

Firefox supports natively object spread since v55 so you can use it here :)

```javascript
this.setState(state => ({
  features: {...state.features, [value]: checked };
});
```

(this is the future, and I love it)

That said I think I'd rather use an array than an object. With an array you wouldn't need to filter and transform the data in `getRecordingSettings`. Is there a good reason to use an object for this in the state?

::: devtools/client/performance-new/components/PerfSettings.js:229
(Diff revision 2)
> +  }
> +
> +  _handleThreadTextCleanup() {
> +    this.setState(state => {
> +      const threadsList = _threadStringToList(state.threads);
> +      return threadsList.join(",");

This doesn't work, because... you need to
```javascript
return {
  threads: threadsList.join(","),
};
```

::: devtools/client/performance-new/components/PerfSettings.js:257
(Diff revision 2)
> +  }
> +
> +  _renderThreads() {
> +    return details(
> +      { className: "perf-settings-details" },
> +      summary({ className: "perf-settings-summary" }, "Threads:"),

nit: I'd remove the colon and possibly add a more descriptive label, like `Choose the threads to capture`.

_OR_ I'd display the full string here when it's collapsed, so that the user doesn't need to expand to see what's selected.
I think we'll eventually do the latter anyway but maybe not now.

::: devtools/client/performance-new/components/PerfSettings.js:258
(Diff revision 2)
> +      div(
> +        { className: "perf-settings-details-contents" },
> +        div(
> +          { className: "perf-settings-details-contents-slider" },

nit: Is there a reason we have 2 divs here? `perf-settings-details-contents` and `perf-settings-details-contents-slider` ? I feel like one of them is useless. If they're both useful I think a comment would help.

::: devtools/client/performance-new/components/PerfSettings.js:262
(Diff revision 2)
> +          div(
> +            { className: "perf-settings-thread-columns" },
> +            threadColumns.map(this._renderThreadsColumns),
> +          ),

Wondering if using either flex (with wrap) or [columns](https://developer.mozilla.org/en-US/docs/Web/CSS/columns) could be easier than having hard-coded columns.

Also it would make it possible to get the data dynamically from Gecko in the future (although I don't know if that's something we want to do).

::: devtools/client/performance-new/components/PerfSettings.js:294
(Diff revision 2)
> +  }
> +
> +  _renderFeatures() {
> +    return details(
> +      { className: "perf-settings-details" },
> +      summary({ className: "perf-settings-summary" }, "Features:"),

nit: same thing than for the threads

::: devtools/client/performance-new/components/PerfSettings.js:329
(Diff revision 2)
> +      )
> +    );
> +  }
> +
> +  render() {
> +    return div(

nit: use a section?

::: devtools/client/performance-new/components/PerfSettings.js:331
(Diff revision 2)
> +  }
> +
> +  render() {
> +    return div(
> +      { className: "perf-settings" },
> +      div({ className: "perf-settings-title" }, "Recording Settings"),

nit: use a real title element ? h2 for example ?

::: devtools/client/performance-new/components/PerfSettings.js:340
(Diff revision 2)
> +      Range({
> +        label: "Sampling interval:",
> +        value: this.state.interval,
> +        type: "interval",
> +        scale: makeExponentialScale(0.01, 100),
> +        display: val => `${val} ms`,
> +        update: interval => {
> +          this.setState({ interval });
> +        }
> +      }),
> +      Range({
> +        label: "Buffer size:",
> +        value: this.state.entries,
> +        type: "entries",
> +        scale: makeExponentialScale(100000, 100000000),
> +        display: val => formatFileSize(val * PROFILE_ENTRY_SIZE),
> +        update: entries => {
> +          this.setState({ entries });
> +        }
> +      }),

In both these ranges, you define new functions as props (`display` and `update`), as well as a new `scale`, which means that they'll always be rendered despite Range being a PureComponent.

So there are 2 solutions:
1. Make Range a Component -- maybe that's OK for such a simple widget
2. Make these functions methods or static functions, and generate the `scale`s only once, for example in the constructor and store them as class properties.

::: devtools/client/performance-new/components/PerfSettings.js:373
(Diff revision 2)
> +    // Split on commas and whitespace
> +    .split(",")

nit: the comment seems obsolete, now you split on commas only

::: devtools/client/performance-new/components/PerfSettings.js:375
(Diff revision 2)
> +    // Filter out any blank strings
> +    .filter(string => string)
> +    // Clean up any extraneous whitespace
> +    .map(string => string.trim());

You should `map` before `filter`, otherwise you'll miss filtering out strings that are made of only spaces.

::: devtools/client/performance-new/components/Range.js:12
(Diff revision 2)
> +const { div, input, label } = require("devtools/client/shared/vendor/react-dom-factories");
> +
> +class Range extends PureComponent {
> +  static get propTypes() {
> +    return {
> +      value: PropTypes.number.isRequired,

I'm not a big fan of the behavior, with "steps" in the range. This happens because we use a `value` that's already been clamped to specific values.

I believe that to make it work like I'd like we'd need to do the scale conversions directly in `getRecordingSettings` instead of doing it in the range (and also in the `display` func obviously).

Tell me what you think. I'd be OK to do this change in a separate bug if you want to move forward.

::: devtools/client/performance-new/components/Range.js:16
(Diff revision 2)
> +    return {
> +      value: PropTypes.number.isRequired,
> +      label: PropTypes.string.isRequired,
> +      type: PropTypes.string.isRequired,
> +      scale: PropTypes.object.isRequired,
> +      update: PropTypes.func.isRequired,

nit: I'd use `onChange` instead of `update`, to better indicate its purpose and have a name consistent with the DOM elements.

::: devtools/client/performance-new/components/Range.js:38
(Diff revision 2)
> +  render() {
> +    const { label: labelText, scale, type, value, display } = this.props;
> +    return (
> +      div(
> +        { className: "perf-settings-row" },
> +        label({ className: "perf-settings-label" }, labelText),

For accessibility, you need to associate the `label` with the `input`. The easiest is to add an `id` to the input and a `for` attribute to the `label`.

::: devtools/client/performance-new/utils.js:8
(Diff revision 2)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const UNITS = ["B", "kB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"];
> +
> +function lerp(frac, rangeStart, rangeEnd) {

nit: please add a comment explaining that "lerp" stands for "linear interpolation". Maybe this is obvious for native english speakers, but it wasn't for me :-)

::: devtools/client/performance-new/utils.js:115
(Diff revision 2)
> +module.exports = {
> +  formatFileSize,
> +  makeExponentialScale,
> +  scaleRangeWithClamping,
> +  calculateOverhead
> +};

nit: I'd appreciate some comments on each of these exported functions, explaining their parameters and their return values.

(I miss Flow :) )

::: devtools/client/themes/perf.css:10
(Diff revision 2)
>  .perf {
>    width: 100%;
>    height: 100%;
>    position: absolute;
>    display: flex;
>    flex-direction: column;

The design as it is isn't very nice when the devtools are docked at the bottom, which is the case in WebIDE. I think we should use `flex-direction: row` with wrap, and better delimitate the parts (for example the introductory text should be at the top), use margins.

This is for another bug, I think, so please file a bug.

::: devtools/client/themes/perf.css:54
(Diff revision 2)
> +.perf-settings-title {
> +  padding: 5px 10px;
> +  margin-bottom: 15px;
> +  background-color: var(--grey-10);
> +  border: var(--grey-30) 1px solid;
> +}

This styling for the title is misleading: I want to click it or interact with it.

See https://design.firefox.com/photon/visuals/typography.html for the styleguide about titles.

::: devtools/client/themes/perf.css:142
(Diff revision 2)
> +.perf-settings-details-contents-slider {
> +  margin: 10px 0;
> +  padding: 10px;
> +  margin: 0 0 18px;

Oops, `margin` is specified twice :)

::: devtools/client/themes/perf.css:211
(Diff revision 2)
> +  line-height: 1.6;
> +}
> +
> +.perf-settings-subtext {
> +  font-weight: bold;
> +}

nit: can you please add comments to mark out the CSS for the various "widgets" ?

Comment 7

[Greg Tatum [:gregtatum]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

https://reviewboard.mozilla.org/r/235380/#review241366

> The other solution is to "lift the state up" like they say in [React documentation](https://reactjs.org/docs/lifting-state-up.html).
> 
> 1. Keep the recordingSettings here
> 2. pass it to `PerfSettings` as a prop, along with a `onSettingsChange` callback.
> 3. `PerfSettings` uses it directly to display its state (instead of using its internal state), and call the `onSettingsChange` for any change with the result of `getRecordingSettings` as parameter.
> 
> I think this is a better approach because this is less coupled. This is also what's recommended in the React documentation.

My concern here is that it makes the Perf.js much more fat, while the PerfSettings encapsulates that bit of logic.

> There's a link utility in devtools, see https://bugzilla.mozilla.org/show_bug.cgi?id=1452678 for more information.
> 
> We need the "toolbox" as parameter, I'm not sure what it is. But then maybe we don't need to pass `target` around.

Ah, I'll have to rebase to use it.

> Is this how we do it in other places? This is a bit sad...
> 
> Wouldn't a `<a target="_blank">` work ?
> 
> Otherwise [the code in devtools reps](https://github.com/devtools-html/devtools-core/blob/72504532bd95735dfa124214a3e694148449d5f0/packages/devtools-reps/src/reps/string.js#L195-L205) uses a link. I think using a link is better for accessibility.

This is how I found the inspector doing it for MDN links. I'd prefer not to use a link as a security bug mitigation. One of the devtools panels loaded external content into devtools which sounds terrifying. If you strongly prefer a link anyway, I can change it.

> nit: Object.entries returns an array, so this may be more idiomatic:
> ```javascript
> const features = Object.entries(this.state.features)
>   .filter(([, isSet]) => isSet)
>   .map(([name]) => name);
> ```
> 
> But as said below I think `features` should hold the array already, and in that case you just need to return it.

I'm not seeing how it's more idiomatic with for of loops, which take iterators as a first class citizens. It's an imperative vs functional style, which both are widely used in the codebase as far as I can tell.

> nit: just wondering if you could use a multiline string here. I know we can't use our neat `common-tags` library here, but maybe React and/or Gecko accept class names with new lines.

I'd rather concat strings, this practice doesn't bother me.

> Firefox supports natively object spread since v55 so you can use it here :)
> 
> ```javascript
> this.setState(state => ({
>   features: {...state.features, [value]: checked };
> });
> ```
> 
> (this is the future, and I love it)
> 
> That said I think I'd rather use an array than an object. With an array you wouldn't need to filter and transform the data in `getRecordingSettings`. Is there a good reason to use an object for this in the state?

That's how the addon did it.

> nit: I'd remove the colon and possibly add a more descriptive label, like `Choose the threads to capture`.
> 
> _OR_ I'd display the full string here when it's collapsed, so that the user doesn't need to expand to see what's selected.
> I think we'll eventually do the latter anyway but maybe not now.

I'm keeping the colon since all the other rows have them.

> nit: Is there a reason we have 2 divs here? `perf-settings-details-contents` and `perf-settings-details-contents-slider` ? I feel like one of them is useless. If they're both useful I think a comment would help.

I added comments as to why it exists.

> Wondering if using either flex (with wrap) or [columns](https://developer.mozilla.org/en-US/docs/Web/CSS/columns) could be easier than having hard-coded columns.
> 
> Also it would make it possible to get the data dynamically from Gecko in the future (although I don't know if that's something we want to do).

It just seemed like an easy approach to ensure the columns displayed as intended without worrying about it over much. I'm keeping it as is unless you feel strongly that I should change it.

> I'm not a big fan of the behavior, with "steps" in the range. This happens because we use a `value` that's already been clamped to specific values.
> 
> I believe that to make it work like I'd like we'd need to do the scale conversions directly in `getRecordingSettings` instead of doing it in the range (and also in the `display` func obviously).
> 
> Tell me what you think. I'd be OK to do this change in a separate bug if you want to move forward.

The Range file is a verbatim copy of Bryan Clark's work, which was an adaptation of the original addon code. My preference would be to keep the logic as it's been implemented. Let me know your preference.

> nit: please add a comment explaining that "lerp" stands for "linear interpolation". Maybe this is obvious for native english speakers, but it wasn't for me :-)

This is all from the addon, I'll work on documenting it.

> nit: I'd appreciate some comments on each of these exported functions, explaining their parameters and their return values.
> 
> (I miss Flow :) )

All these functions are from the addon, I'll take a stab at it.

> The design as it is isn't very nice when the devtools are docked at the bottom, which is the case in WebIDE. I think we should use `flex-direction: row` with wrap, and better delimitate the parts (for example the introductory text should be at the top), use margins.
> 
> This is for another bug, I think, so please file a bug.

I meant to mention this as a follow-up. Here is a bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1453434

> This styling for the title is misleading: I want to click it or interact with it.
> 
> See https://design.firefox.com/photon/visuals/typography.html for the styleguide about titles.

I'd prefer to figure out design problems with mockups, rather than iterating in code review. I'll file a new issue for this. Here is a new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1453447

Comment 8

[Greg Tatum [:gregtatum]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235380/diff/2-3/

Comment 9

[Greg Tatum [:gregtatum]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

https://reviewboard.mozilla.org/r/235380/#review241366

> Ah, I'll have to rebase to use it.

It looks like `openLink` is currently broken. See bug 1453495

> I'm keeping the colon since all the other rows have them.

Hmmm... the extra words make the UI feel much more cluttered. I'm going to strip it off for now. Perhaps we should gather some user feedback on what works...

Comment 10

[Greg Tatum [:gregtatum]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

https://reviewboard.mozilla.org/r/235380/#review241366

> My concern here is that it makes the Perf.js much more fat, while the PerfSettings encapsulates that bit of logic.

I'm going to take another look at making a root controller element, that way I don't make the Perf.js componenent everly fat, and then I can delegate the rendering out to (more) child components.

Comment 11

[Greg Tatum [:gregtatum]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235380/diff/3-4/

Comment 12

[Code Review Bot [:reviewbot]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

https://reviewboard.mozilla.org/r/235380/#review241624


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/performance-new/components/PerfSettings.js:6
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +const { PureComponent, createFactory } = require("devtools/client/shared/vendor/react");
> +const { div, details, summary, label, input, span, h2, h3, section } = require("devtools/client/shared/vendor/react-dom-factories");

Error: 'h3' is assigned a value but never used. [eslint: no-unused-vars]

Comment 13

[Julien Wajsberg [:julienw]]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #7)
> Comment on attachment 8966681 [details]
> Bug 1450709 - Add profile recording settings to the recording panel;
> 
> https://reviewboard.mozilla.org/r/235380/#review241366
> 
> > The other solution is to "lift the state up" like they say in [React documentation](https://reactjs.org/docs/lifting-state-up.html).
> > 
> > 1. Keep the recordingSettings here
> > 2. pass it to `PerfSettings` as a prop, along with a `onSettingsChange` callback.
> > 3. `PerfSettings` uses it directly to display its state (instead of using its internal state), and call the `onSettingsChange` for any change with the result of `getRecordingSettings` as parameter.
> > 
> > I think this is a better approach because this is less coupled. This is also what's recommended in the React documentation.
> 
> My concern here is that it makes the Perf.js much more fat, while the
> PerfSettings encapsulates that bit of logic.

Yeah I understand your initial rationale. Also I know that "lifting the state up" is really boring to do for a developer...

My concern is that everytime I took shortcuts like this in the past I regretted it later and had to change it. It often makes the components less useful.

Also it's important that components handle one thing only, so that the complexity stays low.

For example later you'll implement loading/saving the data. I think it's better if PerfSettings merely displays the settings coming from its props, and gives back the modified settings everytime they're modified. If you add loading/saving the data inside PerfSettings (which I understand is your plan) I think you'll be pushing too many responsibilities into one component.

Rather the loading/saving mechanism will be handle elsewhere, in a SettingsPersister component for example, and then I can see this tree:

<PerfPanel>
  <SettingsPersister>
    <PerfSettings>

SettingsPersister would be working as a proxy between PerfSettings and PerfPanel. PerfPanel would work the same, PerfSettings would work the same, SettingsPersister would handle all the necessary logic.


I also agree that Perf.js is already quite fat.  Maybe all the recording logic could go to a separate component PerfRecorder that could take the settings and the recordingState as parameters. But that's another story...


I'll defer to your judgment here.

> 
> > Is this how we do it in other places? This is a bit sad...
> > 
> > Wouldn't a `<a target="_blank">` work ?
> > 
> 
> This is how I found the inspector doing it for MDN links. I'd prefer not to
> use a link as a security bug mitigation. One of the devtools panels loaded
> external content into devtools which sounds terrifying. If you strongly
> prefer a link anyway, I can change it.

For now I'm fine, especially the URLs themselves are in the button's value, so I think that for accessibility, even if it's likely "weird" for assistance technology (having a button instead of a link), it's not that bad.

I think that if we call "e.preventDefault" as the first line when handling a link click, we shouldn't have any problem. Usually problems happen when the developer calls e.preventDefault late in the event handler, some error is thrown before, and so the link is followed. I agree this is easy to get it wrong though.

> 
> > nit: Object.entries returns an array, so this may be more idiomatic:
> > ```javascript
> > const features = Object.entries(this.state.features)
> >   .filter(([, isSet]) => isSet)
> >   .map(([name]) => name);
> > ```
> > 
> > But as said below I think `features` should hold the array already, and in that case you just need to return it.
> 
> I'm not seeing how it's more idiomatic with for of loops, which take
> iterators as a first class citizens. It's an imperative vs functional style,
> which both are widely used in the codebase as far as I can tell.

I read something very true once: with filter/map, you know right away what the function is doing, but with a for loop you have to actually read the body of the loop to know what's happening.

When we use generators, `for .. of` is nice because it's lazily getting all values and if we break off the loop this can be a nice win. Here it's not a generator but an array, that's why I suggested this.

But I won't block on this.


> 
> > nit: just wondering if you could use a multiline string here. I know we can't use our neat `common-tags` library here, but maybe React and/or Gecko accept class names with new lines.
> 
> I'd rather concat strings, this practice doesn't bother me.

I don't mind, I was mostly surprised.

> 
> > 
> > That said I think I'd rather use an array than an object. With an array you wouldn't need to filter and transform the data in `getRecordingSettings`. Is there a good reason to use an object for this in the state?
> 
> That's how the addon did it.

Is that a good enough reason to do it here as well?

I won't block on it, but really I consider we're introducing technical debt here. Because of this choice we have to filter this object in `getRecordingSettings` and... convert it to an array.

> 
> > nit: I'd remove the colon and possibly add a more descriptive label, like `Choose the threads to capture`.
> > 
> > _OR_ I'd display the full string here when it's collapsed, so that the user doesn't need to expand to see what's selected.
> > I think we'll eventually do the latter anyway but maybe not now.
> 
> I'm keeping the colon since all the other rows have them.

But this is not the same thing. For other rows that's really labels, but here it's more a title...
I won't block on this, and this is something we can change later after an UI review.

> 
> > Wondering if using either flex (with wrap) or [columns](https://developer.mozilla.org/en-US/docs/Web/CSS/columns) could be easier than having hard-coded columns.
> > 
> > Also it would make it possible to get the data dynamically from Gecko in the future (although I don't know if that's something we want to do).
> 
> It just seemed like an easy approach to ensure the columns displayed as
> intended without worrying about it over much. I'm keeping it as is unless
> you feel strongly that I should change it.

I don't feel strongly about it, and I won't block on it.


> 
> > I'm not a big fan of the behavior, with "steps" in the range. This happens because we use a `value` that's already been clamped to specific values.
> > 
> > I believe that to make it work like I'd like we'd need to do the scale conversions directly in `getRecordingSettings` instead of doing it in the range (and also in the `display` func obviously).
> > 
> > Tell me what you think. I'd be OK to do this change in a separate bug if you want to move forward.
> 
> The Range file is a verbatim copy of Bryan Clark's work, which was an
> adaptation of the original addon code. My preference would be to keep the
> logic as it's been implemented. Let me know your preference.

We can keep it now, I don't want to question everything that's been done before. This is something that can be easily changed later too and it's not a trivial change to do. If this bothers me too much I'll do something about it later.

> 
> > nit: I'd appreciate some comments on each of these exported functions, explaining their parameters and their return values.
> 
> All these functions are from the addon, I'll take a stab at it.

Thanks this is much better.


> 
> > This styling for the title is misleading: I want to click it or interact with it.
> > 
> > See https://design.firefox.com/photon/visuals/typography.html for the styleguide about titles.
> 
> I'd prefer to figure out design problems with mockups, rather than iterating
> in code review. I'll file a new issue for this. Here is a new bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1453447

good for me

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

https://reviewboard.mozilla.org/r/235380/#review241844

I still feel bad about this reference and direct method call as I think this will just makes things more complex over time. I left some more thoughts in my other comment.

I don't want to hold back this bug and nothing is set in stone. So I'll r+ this patch nevertheless.

Thanks for this work, I really like how the panel works despite my small nits here and there :)

::: devtools/client/performance-new/utils.js:79
(Diff revisions 2 - 4)
>    return {
>      fromFractionToValue,
>      fromValueToFraction,
>      fromFractionToSingleDigitValue,
>    };

I think I'd like 1 example for each of these functions, to make them more concrete and better understand what they do.

Comment 15

[Greg Tatum [:gregtatum]]

Comment on attachment 8966681 [details]
Bug 1450709 - Add profile recording settings to the recording panel;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235380/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75b4974abeef
Add profile recording settings to the recording panel; r=julienw

Comment 17

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/75b4974abeef