1213137 - Memory snapshots list view

Status: RESOLVED FIXED

(DevTools :: Memory, defect)

Description

[Jordan Santell [:jsantell] [@jsantell]]

A very unexciting patch, but necessary, and contains foundation of the react app in memory tool, and will just be a snapshot button, and updates a list of current snapshots, and allows selecting a snapshot. Necessary for heap view.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1213137-snapshot-list.patch

First round at this. Getting the hang of it. How's this structure? If it generally looks good, I'll start on the tests. Dependent on react-redux, which I'll try to land in bug 1213138 unless you have a strong opinion against it (I'd rather use something else than a NIH module, unless it really is that simple)

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8671706 [details] [diff] [review]
1213137-snapshot-list.patch

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

::: devtools/client/memory/actions/snapshot.js
@@ +15,5 @@
> +
> +const selectSnapshot = exports.selectSnapshot = function takeSnapshot (snapshot) {
> +  return {
> +    type: actions.SELECT_SNAPSHOT,
> +    snapshot

What is snapshot here? A snapshot file path? A snapshot id? Not clear to me.

::: devtools/client/memory/app.js
@@ +20,5 @@
> +
> +  render() {
> +    let { dispatch, snapshots } = this.props;
> +    return (
> +      DOM.div({ className: "memory-tool" }, ...[

Why `...[ one, two]` rather than `one, two`?

@@ +25,5 @@
> +
> +        Toolbar({
> +          buttons: [{
> +            className: "take-snapshot",
> +            onClick: () => dispatch(takeSnapshot(this.props.front))

Nit: might as well destructure front from this.props when destructuring dispatch and snapshots above.

@@ +31,5 @@
> +        }),
> +
> +        List({
> +          items: snapshots,
> +          onClick: (snapshot) => dispatch(selectSnapshot(snapshot))

Nit: () around snapshot not needed.

::: devtools/client/memory/components/list.js
@@ +1,5 @@
> +const { DOM, createClass, PropTypes } = require("devtools/client/shared/vendor/react");
> +
> +/**
> + * Factory that takes a component `ListItem` and returns
> + * a List component that populates itself with ListItems.

Nit: Ah, if this is really a factory, maybe it shouldn't be called list.js but list-factor.js

@@ +3,5 @@
> +/**
> + * Factory that takes a component `ListItem` and returns
> + * a List component that populates itself with ListItems.
> + */
> +module.exports = (ListItem) => createClass({

Nit: () unnecessary

@@ +12,5 @@
> +    items: PropTypes.array.isRequired,
> +  },
> +
> +  render() {
> +    console.log("lsit", this.props);

s/lsit/list/

or just remove the whole thing

@@ +17,5 @@
> +    let items = this.props.items;
> +    let onClick = this.props.onClick;
> +
> +    return (
> +      DOM.ul({ className: "list" }, ...items.map((item, index) => {

React lets you pass an array of children in, doesn't it? I thought it did. If so, the ... is unnecessary.

::: devtools/client/memory/components/snapshot-list-item.js
@@ +6,5 @@
> +  propTypes: {
> +    onClick: PropTypes.func,
> +    item: PropTypes.any.isRequired,
> +    index: PropTypes.number.isRequired,
> +  },

Why do you have propTypes inline here, but out of line for App? Seems we should be consistent one way or the other.

@@ +12,5 @@
> +  render() {
> +    let index = this.props.index;
> +    let item = this.props.item;
> +    let onClick = this.props.onClick;
> +    let className = `snapshot-list-item ${item.selected ? " selected" : ""}`; 

Nit: trailing white space.

::: devtools/client/memory/components/toolbar.js
@@ +5,5 @@
> +
> +  render() {
> +    let buttons = this.props.buttons;
> +    return (
> +      DOM.div({ className: "devtools-toolbar" }, ...buttons.map(spec => {

Again, I think the ... is unnecessary.

::: devtools/client/memory/initializer.js
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +const { Task } = Cu.import("resource://gre/modules/Task.jsm", {});

Using Cu.import's return value is a recently discovered anti-pattern. It doesn't actually do what you'd think (return an object containing the exported items from the module, ie the second param to Cu.import) but instead returns the whole freaking global, allowing access to the module's private state and breaking some kind of thing related to top level let/const. Ask shu for more details about that last part.

Instead, use plain old `Cu.import"...")` to attach the exported items to the global scope or write a wrapper like:

  function scopedImport(path) {
    const scope = {};
    Cu.import(path, scope);
    return scope;
  }

and then use that with destructuring:

  const { Task } = scopedImport("...");

@@ +6,5 @@
>  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +const { Task } = Cu.import("resource://gre/modules/Task.jsm", {});
> +const { BrowserLoader } = Cu.import("resource:///modules/devtools/client/shared/browser-loader.js", {});
> +const { require } = BrowserLoader("resource:///modules/devtools/client/memory/", this);
> +//const { require } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

?

@@ +26,1 @@
>  function initialize () {

Wait, there are two initialize functions? One in controller.js and one in intializer.js? How do their responsibilities differ and what does each one do? Which one is called first? I assume they do different things, but then why are they called the same thing?

We need some comments and clean up surrounding this...

@@ +26,2 @@
>  function initialize () {
> +  return Task.spawn(function*() {

Why is this a task? I see no yields or anything like that.

::: devtools/client/memory/reducers/snapshot.js
@@ +32,5 @@
> +function handleSelectSnapshot (state, action) {
> +  let selected = state.find(s => s.id === action.snapshot.id);
> +
> +  if (!selected) {
> +    DevToolsUtils.reportException(`Cannot select non-existant snapshot ${snapshot.id}`);

Should we early return after this and not try and select the non-existant snapshot? Usually things early exit after a reportException. If we do mean to fall through here, then a small comment would help the reader.

::: devtools/client/memory/store.js
@@ +3,5 @@
>  const reducers = require("./reducers");
>  const DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  
>  module.exports = function () {
> +  let shouldLog = DevToolsUtils.testing || true;

lol :)

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8671706 [details] [diff] [review]
1213137-snapshot-list.patch

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

::: devtools/client/memory/actions/snapshot.js
@@ +15,5 @@
> +
> +const selectSnapshot = exports.selectSnapshot = function takeSnapshot (snapshot) {
> +  return {
> +    type: actions.SELECT_SNAPSHOT,
> +    snapshot

Snapshot model, described by React's App PropTypes for the main state (should clarify here too)

::: devtools/client/memory/app.js
@@ +20,5 @@
> +
> +  render() {
> +    let { dispatch, snapshots } = this.props;
> +    return (
> +      DOM.div({ className: "memory-tool" }, ...[

I think I was using this pattern elsewhere and just stuck with it, no specific reason here as we do not need to create a new parent object like we do in reducers

@@ +25,5 @@
> +
> +        Toolbar({
> +          buttons: [{
> +            className: "take-snapshot",
> +            onClick: () => dispatch(takeSnapshot(this.props.front))

+1

::: devtools/client/memory/components/list.js
@@ +1,5 @@
> +const { DOM, createClass, PropTypes } = require("devtools/client/shared/vendor/react");
> +
> +/**
> + * Factory that takes a component `ListItem` and returns
> + * a List component that populates itself with ListItems.

Not sure what the idiom is here -- other Components generally are wrapped in `createFactory` when using non-JSX to instantiate, but that seems like a transparent by product of non-JSX, whereas this is explicitly a factory. James?

@@ +17,5 @@
> +    let items = this.props.items;
> +    let onClick = this.props.onClick;
> +
> +    return (
> +      DOM.ul({ className: "list" }, ...items.map((item, index) => {

Not that I've seen, although I cannot find any good documentation on React that does not use JSX.

::: devtools/client/memory/initializer.js
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +const { Task } = Cu.import("resource://gre/modules/Task.jsm", {});

This sounds disgusting, and will avoid anything that implicitly adds globals to a context -- it's completely prohibitive to anyone new looking at the codebase

@@ +6,5 @@
>  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
> +const { Task } = Cu.import("resource://gre/modules/Task.jsm", {});
> +const { BrowserLoader } = Cu.import("resource:///modules/devtools/client/shared/browser-loader.js", {});
> +const { require } = BrowserLoader("resource:///modules/devtools/client/memory/", this);
> +//const { require } = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});

React requires BrowserLoader due to accessing document/window directly, there are a lot of things commented out here, no tests, etc., WIP

@@ +26,1 @@
>  function initialize () {

controller.js should be removed now actually, initializer is loaded into browser context and initializes the react app

@@ +26,2 @@
>  function initialize () {
> +  return Task.spawn(function*() {

Easier to go async->sync than vice versa

::: devtools/client/memory/reducers/snapshot.js
@@ +32,5 @@
> +function handleSelectSnapshot (state, action) {
> +  let selected = state.find(s => s.id === action.snapshot.id);
> +
> +  if (!selected) {
> +    DevToolsUtils.reportException(`Cannot select non-existant snapshot ${snapshot.id}`);

Not sure what to do in these failure cases -- in this case it'd unselect everything

::: devtools/client/memory/store.js
@@ +3,5 @@
>  const reducers = require("./reducers");
>  const DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  
>  module.exports = function () {
> +  let shouldLog = DevToolsUtils.testing || true;

i had a fun time debugging

Comment 4

[James Long (:jlongster)]

Comment on attachment 8671706 [details] [diff] [review]
1213137-snapshot-list.patch

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

::: devtools/client/memory/components/list.js
@@ +1,5 @@
> +const { DOM, createClass, PropTypes } = require("devtools/client/shared/vendor/react");
> +
> +/**
> + * Factory that takes a component `ListItem` and returns
> + * a List component that populates itself with ListItems.

I probably wouldn't call this a "factory" because it's not a factory in the sense of `React.createFactory` which just turns `React.createElement('div')` into `div()`. This is just a "higher-order component" in a sense. You could even just a "A function that takes a component...".

But as you just said on IRC, you might want to try just passing the component type as a prop to the list component, and it just renders it with `React.createElement(this.props.itemComponent)` or something.

I don't have time to do a full review, just glancing through. Will look more thoroughly by Monday.

@@ +17,5 @@
> +    let items = this.props.items;
> +    let onClick = this.props.onClick;
> +
> +    return (
> +      DOM.ul({ className: "list" }, ...items.map((item, index) => {

Why capitalize DOM? It's probably more technically correct, but it's so much easier to type "dom"...

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8671706 [details] [diff] [review]
1213137-snapshot-list.patch

Talked to jlong about this, some high level critiques:

* List component should take its child component as a property (rather than the module exporting a "factory" [not to be confused with react's factory])
* Test actions and expected state as xpcshell
* Individually test React components via Shallow Rendering[0]
* Overall integration testing via browser tests (action creator testing is hard, and this will test all the glue inbetween)
* General architecture is good, any changes wouldn't be that big

[0] https://facebook.github.io/react/docs/test-utils.html

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1213137-snapshot-list.patch

Addressed Nick's comments, added tests for selecting snapshot. Holding off on the browser tests until we have more actions to take (like rendering the heap)

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8672065 [details] [diff] [review]
1213137-snapshot-list.patch

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

::: devtools/client/memory/app.js
@@ +46,5 @@
> +
> +  render() {
> +    let { dispatch, snapshots, front } = this.props;
> +    return (
> +      dom.div({ className: "memory-tool" }, ...[

I would rather use an id here since there's only one memory tool (unless you plan to reuse the memory tool App component somewhere else)

@@ +48,5 @@
> +    let { dispatch, snapshots, front } = this.props;
> +    return (
> +      dom.div({ className: "memory-tool" }, ...[
> +
> +        Toolbar({

Was there a discussion about whether JSX should be used ? This code would be much shorter with JSX.

::: devtools/client/themes/memory.css
@@ +24,5 @@
> + * TODO bug 1213100
> + * should generalize toolbar buttons with images in them
> + * for non-XUL environments
> + */
> +.devtools-toolbarbutton.take-snapshot::before {

nit: Use the .devtools-button class, it's made for HTML and you only need to set the background-image on ::before for the button image.

@@ +37,5 @@
> +
> +/**
> + * TODO bug 1213100
> + * Once we figure out how to store invertable buttons (pseudo element like in this case?)
> + * we should add a .invertable class to handle this generally, rather than the definitions

That would rather be bug 1173397

Comment 8

[James Long (:jlongster)]

> Was there a discussion about whether JSX should be used ? This code would be much shorter with JSX.

IMHO it's basically a non-option right now. We'd need to integrate a build step which is a decent amount of work, and also debugging would be a lot harder because we have to go through sourcemaps, and we don't support them very well in the devtools (console still doesn't sourcemap line numbers, which we're going to fix soon). The worst part to me is that Error stacks in the terminal aren't automatically sourcemapped either.

Honestly code is not that much shorter with JSX. All the closing tags and other stuff cancel out other verbosity savings. I think right now it's way easier to just avoid it.

Comment 9

[James Long (:jlongster)]

Comment on attachment 8672065 [details] [diff] [review]
1213137-snapshot-list.patch

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

Comment 10

[James Long (:jlongster)]

Comment on attachment 8672065 [details] [diff] [review]
1213137-snapshot-list.patch

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

I guess bugzilla reviews don't support emoji :( at least as the first character. Let's try again:

Comment 11

[James Long (:jlongster)]

Ok, so bugzilla cuts messages off when you use an emoji. Here's an ascii version. :thumbsup: Looks great! No big problems, just some nits that you can ignore if you don't have time for now

Comment 12

[James Long (:jlongster)]

Agh, looks like bugzilla actually ate all of my reviews because I used emoji :( I'll go through it quickly again...

Comment 13

[James Long (:jlongster)]

Comment on attachment 8672065 [details] [diff] [review]
1213137-snapshot-list.patch

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

Had to re-write this, I don't think I missed anything.

::: devtools/client/memory/components/list.js
@@ +10,5 @@
> +
> +  propTypes: {
> +    itemComponent: PropTypes.any.isRequired,
> +    onClick: PropTypes.func,
> +    items: PropTypes.array.isRequired,

You get many +1s for writing propTypes. Many people don't and they are awesome. Typed components are awesome.

@@ +17,5 @@
> +  render() {
> +    let { items, onClick, itemComponent: Item } = this.props;
> +
> +    return (
> +      dom.ul({ className: "list" }, ...items.map((item, index) => {

I think Nick already said this but you don't need the `...`. React will automatically "flatMap" arrays for you.

::: devtools/client/memory/components/snapshot-list-item.js
@@ +10,5 @@
> +  },
> +
> +  render() {
> +    let { index, item, onClick } = this.props;
> +    let className = `snapshot-list-item ${item.selected ? " selected" : ""}`;

There's a "classnames" package on npm that everybody uses to selectively apply classes. This is fine for now, but we may want to look into that in the future. It allows stuff like this:

dom.li({ className: classNames('snapshot-list-item', { selected: item.selected }) })

You can pass strings or objects, and if you pass an object it will only apply the classes with truthy values. Very nice when things get more complex. This is fine for now though.

(It's too bad it's a pain to use npm packages. We could also write our own at some point, it's very small code. I don't think there's anything wrong with writing some of our own custom React utils)

@@ +12,5 @@
> +  render() {
> +    let { index, item, onClick } = this.props;
> +    let className = `snapshot-list-item ${item.selected ? " selected" : ""}`;
> +    return (
> +      DOM.li({ className, onClick },

Need to lowercase DOM (like you did in list.js). For larger components we can also destructure the common tags, like `div` etc.

::: devtools/client/memory/reducers/snapshot.js
@@ +3,3 @@
>  const DevToolsUtils = require("devtools/shared/DevToolsUtils");
>  
>  function handleTakeSnapshot (state, action) {

nit: no space before parens. This should be enforced by eslint not (looks like you do that consistently, not going to comment on all of them)

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8672065 [details] [diff] [review]
1213137-snapshot-list.patch

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

::: devtools/client/memory/app.js
@@ +46,5 @@
> +
> +  render() {
> +    let { dispatch, snapshots, front } = this.props;
> +    return (
> +      dom.div({ className: "memory-tool" }, ...[

Changed to an id

::: devtools/client/memory/components/list.js
@@ +17,5 @@
> +  render() {
> +    let { items, onClick, itemComponent: Item } = this.props;
> +
> +    return (
> +      dom.ul({ className: "list" }, ...items.map((item, index) => {

Changed instances of splatting an array into a dom function

::: devtools/client/memory/components/snapshot-list-item.js
@@ +10,5 @@
> +  },
> +
> +  render() {
> +    let { index, item, onClick } = this.props;
> +    let className = `snapshot-list-item ${item.selected ? " selected" : ""}`;

Oh that'll indeed be nice

::: devtools/client/themes/memory.css
@@ +37,5 @@
> +
> +/**
> + * TODO bug 1213100
> + * Once we figure out how to store invertable buttons (pseudo element like in this case?)
> + * we should add a .invertable class to handle this generally, rather than the definitions

I'll add that as a note, but this bug # is more for cleaning up memory styles

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1213137-snapshot-list.patch

Addressed comments!

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/25d4835ddc13

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25d4835ddc13

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8673822 [details] [diff] [review]
1213137-snapshot-list.patch

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

::: devtools/client/themes/memory.css
@@ +28,5 @@
> + */
> +.devtools-button.take-snapshot {
> +  margin: 2px 1px;
> +  padding: 1px;
> +  border-width: 0px;

I don't think the padding and margin rules are necessary.

@@ +29,5 @@
> +.devtools-button.take-snapshot {
> +  margin: 2px 1px;
> +  padding: 1px;
> +  border-width: 0px;
> +  /* [standalone] buttons override min-height from 18px to 24px -- why? */

Why are you using a standalone button in the first place ? Standalone buttons are for the "Reload to ..." buttons at the middle of the page.

@@ +35,5 @@
> +  /* not sure why this is needed for positioning */
> +  display: -moz-box;
> +}
> +
> +.devtools-button.take-snapshot::before {

Most of the rules below can be omitted (width, height, -moz-appearance and background-repeat).

They are already defined here (hence the advantage of using .devtools-button): https://dxr.mozilla.org/mozilla-central/source/devtools/client/themes/toolbars.inc.css#303