1308697 - Implement UI for performance statistics

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

This is a break down task for bug 1308425.

- Implement UI component for performance statistics
- Use d3.js instead of chart.jsm

Comment 1

[Steve Chung [:steveck]]

I might have some time for this, since it looks more independent than other issues. But it seems not that straightforward: Chart.jsm is not only for rendering the pie chart, but also (1)table beside the pie chart, (2) calculation for translate animation, and (3)the event handler for listening/emitting mouseover/mouseleave events in pie chart and table(The hover and focus effect need to apply for both).

I probably won't introduce redux at the beginning, so not sure which action would be best as first step. Maybe we can make others into UI component and leave chart.jsm(and the related elements) as it is?

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Here is what I think we could do:

1) Start with bug 1308425. The PerformanceStatisticsView object should be moved into its own module (together with Chart.jsm import etc.)

2) Use React for table rendered next to the pie chart (i.e. for everything but the pie chart) 

3) Wrap Char.jsm into React component and integrate it with the rest of the system (i.e. with the React components hierarchy). We might take some inspiration from [1].

Later, we might want to replace Chart.jsm with D3.js [2] and integrate it with React [3], but we should have real reasons for this (this might be p3/reserve work).

Also, bug 1308696 might be implemented along the lines. Especially if it can simplify the integration process with the rest of the system/React.

---

In any case you should start with bug 1308425, it could be quite straightforward.

Honza



[1] https://github.com/reactjs/react-chartjs.
[2] https://square.github.io/intro-to-d3/examples/#a-pie-chart
[3] https://reactjsnews.com/playing-with-react-and-d3

Comment 3

[Steve Chung [:steveck]]

Attachment: Bug 1308697 - Implement UI for performance statistics.

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

Comment 4

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

Since it's just a experimental patch and it would be better to land after bug 1309194, change request to f? just for checking if the high level back button React migration and view reducer/action design is ok.

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review91178

::: devtools/client/netmonitor/constants.js:12
(Diff revision 1)
>    TOGGLE_FILTER: "TOGGLE_FILTER",
>    ENABLE_FILTER_ONLY: "ENABLE_FILTER_ONLY",
>    TOGGLE_SIDEBAR: "TOGGLE_SIDEBAR",
>    SHOW_SIDEBAR: "SHOW_SIDEBAR",
>    DISABLE_TOGGLE_BUTTON: "DISABLE_TOGGLE_BUTTON",
> +  SHOW_PERFORMANCE_STATISTICS_VIEW: "SHOW_PERFORMANCE_STATISTICS_VIEW",

Let's keep our action simple and remain one action type to update PERFORMANCE_STATISTICS_VIEW.

Like UPDATE_PERFORMANCE_STATISTICS_VIEW to set an enabled state.

::: devtools/client/netmonitor/requests-menu-view.js:167
(Diff revision 1)
>      this._onContextCopyImageAsDataUriCommand =
>        this.copyImageAsDataUri.bind(this);
>      this._onContextCopyResponseCommand = this.copyResponse.bind(this);
>      this._onContextResendCommand = this.cloneSelectedRequest.bind(this);
>      this._onContextToggleRawHeadersCommand = this.toggleRawHeaders.bind(this);
> -    this._onContextPerfCommand = () => NetMonitorView.toggleFrontendMode();
> +    this._onContextPerfCommand = () => this.store.dispatch(Actions.togglePerformanceStatisticsView());;

I recommend that remove all legacy calls like _onContextPerfCommand() and invoke dispatch(action...) directly where invoke _onContextPerfCommand().

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review91518

I couldn't apply the patch so, feedback based only on the review-board web UI.

See my inline comment.

In general, this looks good and it's going the right direction.

Honza

::: devtools/client/netmonitor/actions/performance-statistics-view.js:4
(Diff revision 1)
> +/* 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";

nit: missing empty line between "use strict" and Mozilla license header

::: devtools/client/netmonitor/constants.js:13
(Diff revision 1)
>    ENABLE_FILTER_ONLY: "ENABLE_FILTER_ONLY",
>    TOGGLE_SIDEBAR: "TOGGLE_SIDEBAR",
>    SHOW_SIDEBAR: "SHOW_SIDEBAR",
>    DISABLE_TOGGLE_BUTTON: "DISABLE_TOGGLE_BUTTON",
> +  SHOW_PERFORMANCE_STATISTICS_VIEW: "SHOW_PERFORMANCE_STATISTICS_VIEW",
> +  TOGGLE_PERFORMANCE_STATISTICS_VIEW: "TOGGLE_PERFORMANCE_STATISTICS_VIEW",

Yes, agree with Ricky. Just a bit more background for this. 

I am seeing this pattern a lot. Quite often we have a boolean state in a reducer and then designing proper actions to change it.

There are three options:

# Option 1
A single action named UPDATE_BOOLEAN_THING, and you pass in the new state to the action creator like `updateBooleanThing(true)` to enable it.

# Option 2
Separate actions for each state transition, so ENABLE_BOOLEAN_THING and DISABLE_BOOLEAN_THING.

# Option 3
A single TOGGLE_BOOLEAN_THING action so the component is not
describing the state to move towards explicitly, but leaves it to the reducer to handle instead. 

The recommended and agreed approach is #1 and we should also follow that to be consistent.

---

With #1, you can also define helpers to make it similar to #2 from the caller's side without doubling the number of reducers:

const enableThing = () => ({
  type: UPDATE_BOOLEAN_THING,
  value: true,
});

const disableThing = () => ({
  type: UPDATE_BOOLEAN_THING,
  value: false,
}); 


Here is the full discussion (DevTools team) about it:
https://groups.google.com/forum/#!searchin/mozilla.dev.developer-tools/ryan%7Csort:relevance/mozilla.dev.developer-tools/XPx2pgz6EQs/LAEd4P5JIAAJ

Honza

::: devtools/client/netmonitor/netmonitor-view.js:1242
(Diff revision 1)
> +    if (newValue !== currentValue) {
> +      onChange(newValue, currentValue);
> +      currentValue = newValue;
> +    }
> +  };
> +}

This is dup of the storeWatcher() method in request-menu-view? I guess this will go away eventually, correct?

::: devtools/client/netmonitor/reducers/index.js:4
(Diff revision 1)
>  /* 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";

nit: missing empty line

::: devtools/client/netmonitor/reducers/performance-statistics-view.js:4
(Diff revision 1)
> +/* 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";

nit: missing empty line

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review91536

::: devtools/client/netmonitor/netmonitor.xul:729
(Diff revision 1)
>  
> +    <!-- <html:div xmlns="http://www.w3.org/1999/xhtml" id="react-network-statistics-view"> -->
>      <box id="network-statistics-view">
>        <toolbar id="network-statistics-toolbar"
>                 class="devtools-toolbar">
> -        <button id="network-statistics-back-button"
> +        <html:div xmlns="http://www.w3.org/1999/xhtml" id="react-network-statistics-back-button"/>

nit: Ricky is using a postfix '-hook' for mount nodes (e.g. in bug 1309194). We can be consistent and use the following ID: "network-statistics-back-button-hook"

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review91518

> Yes, agree with Ricky. Just a bit more background for this. 
> 
> I am seeing this pattern a lot. Quite often we have a boolean state in a reducer and then designing proper actions to change it.
> 
> There are three options:
> 
> # Option 1
> A single action named UPDATE_BOOLEAN_THING, and you pass in the new state to the action creator like `updateBooleanThing(true)` to enable it.
> 
> # Option 2
> Separate actions for each state transition, so ENABLE_BOOLEAN_THING and DISABLE_BOOLEAN_THING.
> 
> # Option 3
> A single TOGGLE_BOOLEAN_THING action so the component is not
> describing the state to move towards explicitly, but leaves it to the reducer to handle instead. 
> 
> The recommended and agreed approach is #1 and we should also follow that to be consistent.
> 
> ---
> 
> With #1, you can also define helpers to make it similar to #2 from the caller's side without doubling the number of reducers:
> 
> const enableThing = () => ({
>   type: UPDATE_BOOLEAN_THING,
>   value: true,
> });
> 
> const disableThing = () => ({
>   type: UPDATE_BOOLEAN_THING,
>   value: false,
> }); 
> 
> 
> Here is the full discussion (DevTools team) about it:
> https://groups.google.com/forum/#!searchin/mozilla.dev.developer-tools/ryan%7Csort:relevance/mozilla.dev.developer-tools/XPx2pgz6EQs/LAEd4P5JIAAJ
> 
> Honza

I could agree with #1, but I'm think a possible solution like #1 + #3. We only expose 1 action for view state, and toggle the state if the state is not provided. So we can still leave view state to the reducer to handle. Not sure if it's too complicated for an acion.

> This is dup of the storeWatcher() method in request-menu-view? I guess this will go away eventually, correct?

Yes, I was thinking to moving this to an utility function, but I gave up since it will be removed once we apply React completely just like you said.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review94466

Please rebase to latest version since there are so many update last week. UI relevant actions or reducers for performance-statistics-view should put in actions/ui.js or reducer/ui.js.

Let's shorten the name of performance-statistics-view to statistics. I think it's ok to rename netmonitor/statistics-view.js and rename to statistics for redux relevant parts (ex: openStatistics() and toggleStatistics() in actions/ui.js and OPEN_STATISTICS and TOGGLE_STATISTICS for constants. But perhaps openStatistics() is unnecessary in this case).

Remember to keep code style consistent with other existing code but please take a look at bug 1309866 as well. For instance, we're going to use sidebarOpen state in reducer/ui.js (bug 1309866), so follow that pattern would be statisticsOpen.

Feel free to set me as second reviewer. thanks!

::: devtools/client/netmonitor/actions/moz.build:1
(Diff revision 1)
>  # vim: set filetype=python:

nit: remove vim config

::: devtools/client/netmonitor/performance-statistics-view.js:1
(Diff revision 1)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */

nit: remove vim config

::: devtools/client/netmonitor/performance-statistics-view.js:2
(Diff revision 1)
>  /* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* vim: set ft=javascript ts=2 et sw=2 tw=80: */

nit: remove vim config

::: devtools/client/netmonitor/reducers/moz.build:1
(Diff revision 1)
>  # vim: set filetype=python:

nit: remove vim config

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

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

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review95644

::: devtools/client/netmonitor/components/summary-button.js:5
(Diff revision 2)
>  /* 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/. */
>  
>  /* globals NetMonitorView */

nit: NetMonitorView is no longer used. We can remove this line.

::: devtools/client/netmonitor/netmonitor.xul:723
(Diff revision 2)
>  
>      </vbox>
>  
>      <box id="network-statistics-view">
>        <toolbar id="network-statistics-toolbar"
>                 class="devtools-toolbar">

nit: add break line for id:"xxx" to keep code style consistent. I think it's too long for current id, how do you think "react-statistics-back-hook"?

::: devtools/client/netmonitor/performance-statistics-view.js:1
(Diff revision 2)
>  /* This Source Code Form is subject to the terms of the Mozilla Public

Let's shorten the module name to statistics-view.js

::: devtools/client/netmonitor/performance-statistics-view.js:28
(Diff revision 2)
>  const NETWORK_ANALYSIS_PIE_CHART_DIAMETER = 200;
>  
>  /**
>   * Functions handling the performance statistics view.
>   */
>  function PerformanceStatisticsView() {

Likwise above. shorten the name to StatisticsView.

::: devtools/client/netmonitor/performance-statistics-view.js:33
(Diff revision 2)
>  function PerformanceStatisticsView() {
>  }
>  
>  PerformanceStatisticsView.prototype = {
>    /**
>     * Initialization function, called when the debugger is started.

Update the comment. "debugger" should be "network monitor"

::: devtools/client/netmonitor/performance-statistics-view.js:43
(Diff revision 2)
> +
> +    let backStr = L10N.getStr("netmonitor.backButton");
> +    ReactDOM.render(button({
> +      id: "network-statistics-back-button",
> +      className: "devtools-toolbarbutton",
> +      label: backStr,

label is an useless attribute for button, please remove it.

::: devtools/client/netmonitor/performance-statistics-view.js:46
(Diff revision 2)
> +      id: "network-statistics-back-button",
> +      className: "devtools-toolbarbutton",
> +      label: backStr,
> +      title: backStr,
> +      onClick: () => {
> +        this.store.dispatch(Actions.openStatistics(false));

I think the openStatistics() action is unnecessary in the case. toggleStatistics() is enough to handle back behavior, so it makes sense to remove openStatistics() in action.

::: devtools/client/netmonitor/performance-statistics-view.js:47
(Diff revision 2)
> +      className: "devtools-toolbarbutton",
> +      label: backStr,
> +      title: backStr,
> +      onClick: () => {
> +        this.store.dispatch(Actions.openStatistics(false));
> +      }

nit: add trailing comma for last item as well.

https://github.com/airbnb/javascript#commas--dangling

::: devtools/client/netmonitor/performance-statistics-view.js:52
(Diff revision 2)
> +      }
> +    }, backStr), this._backButton);
> +  },
> +
> +  /**
> +    * Destruction function, called when the debugger is closed.

Update the comment. "debugger" should be "network monitor"

::: devtools/client/netmonitor/reducers/ui.js:20
(Diff revision 2)
>  const Sidebar = I.Record({
>    open: false,
>  });
>  
> +const Statistics = I.Record({
> +  visible: false,

We should keep aware of Jarda's patch.

https://reviewboard.mozilla.org/r/90444/diff/14#39

Latest structure has changed to sidebarOpen in UI instead of a new Sidebar record.

So please change to the new one because is more straightforward to understand:

const UI = I.Record({
  statisticsOpen: false,
});

::: devtools/client/netmonitor/reducers/ui.js:37
(Diff revision 2)
>  function toggleSidebar(state, action) {
>    return state.setIn(["sidebar", "open"], !state.sidebar.open);
>  }
>  
> +function openStatistics(state, action) {
> +  return state.setIn(["statistics", "visible"], action.visible);

likewise. update it to statisticsOpen

::: devtools/client/netmonitor/reducers/ui.js:41
(Diff revision 2)
> +function openStatistics(state, action) {
> +  return state.setIn(["statistics", "visible"], action.visible);
> +}
> +
> +function toggleStatistics(state, action) {
> +  return state.setIn(["statistics", "visible"], !state.statistics.visible);

likewise. update it to statisticsOpen

::: devtools/client/netmonitor/requests-menu-view.js:333
(Diff revision 2)
>      this._flushRequestsTask.arm();
>      return undefined;
>    },
>  
>    /**
> +   * Opens selected item in a new tab.

Is there any reason why we copy these utils APIs from  request-list-context-menu.js to here?

It also introduced duplicated code in two places.

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review95658

Noticed there are some test failures in browser_net_statistics-01.js and timeout in browser_net_statistics-02.js and browser_net_statistics-03.js. Please make sure address them as well.

Comment 13

[Ricky Chien [:rickychien] (inactive)]

BTW, make sure to close reviewboard issues once there are fixed.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am seeing good comments from Ricky. I'll do my review as soon as they are resolved.

Honza

Comment 15

[Steve Chung [:steveck]]

Attachment: Bug 1308697 - Part 2: Address Ricky's comments and fix failed tests,

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

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

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

Comment 17

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

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

Comment 18

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

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

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

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

Comment 20

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review95644

> I think the openStatistics() action is unnecessary in the case. toggleStatistics() is enough to handle back behavior, so it makes sense to remove openStatistics() in action.

I kept this because we need to open/close panel, but it seems could be replaced by toggle because there's no case that could open an opened panel(or close vise versa). I still keep this because it's more close to our intention, but please let me know if you still prefer to make the action simpler.

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review99060

Most of the code looks good! Please address open-issues and make sure try is green.

::: devtools/client/netmonitor/actions/ui.js:10
(Diff revision 6)
>  "use strict";
>  
>  const {
>    OPEN_SIDEBAR,
>    WATERFALL_RESIZE,
> +  OPEN_STATISTICS,

nit: move it after OPEN_SIDEBAR to comply with alphabetical order

::: devtools/client/netmonitor/actions/ui.js:42
(Diff revision 6)
>      type: WATERFALL_RESIZE,
>      width
>    };
>  }
>  
> +/**

nit: move it after openSidebar() to comply with alphabetical order

::: devtools/client/netmonitor/actions/ui.js:42
(Diff revision 6)
>      type: WATERFALL_RESIZE,
>      width
>    };
>  }
>  
> +/**

nit: method name should keep alphabetical order as well

::: devtools/client/netmonitor/actions/ui.js:65
(Diff revision 6)
> +
>  module.exports = {
>    openSidebar,
>    toggleSidebar,
>    resizeWaterfall,
> +  openStatistics,

nit: keep alphabetical order like above

::: devtools/client/netmonitor/constants.js:31
(Diff revision 6)
>    TOGGLE_FILTER_TYPE: "TOGGLE_FILTER_TYPE",
>    ENABLE_FILTER_TYPE_ONLY: "ENABLE_FILTER_TYPE_ONLY",
>    SET_FILTER_TEXT: "SET_FILTER_TEXT",
>    OPEN_SIDEBAR: "OPEN_SIDEBAR",
>    WATERFALL_RESIZE: "WATERFALL_RESIZE",
> +  OPEN_STATISTICS: "OPEN_STATISTICS",

nit: Let's re-arrange these aciton types to follow alphabetical order

::: devtools/client/netmonitor/statistics-view.js:10
(Diff revision 6)
>  const {XPCOMUtils} = require("resource://gre/modules/XPCOMUtils.jsm");
>  const {PluralForm} = require("devtools/shared/plural-form");
>  const {Filters} = require("./filter-predicates");
>  const {L10N} = require("./l10n");
>  const {EVENTS} = require("./events");
> +const { createFactory, DOM } = require("devtools/client/shared/vendor/react");
> +const ReactDOM = require("devtools/client/shared/vendor/react-dom");
>  const Actions = require("./actions/index");
> +const { button, div } = DOM;

nit: keep syntax in the same code style

Most of our new code follow the space-surround pattern, and it looks good. If you'd like space-surround, please modify all of them at once.

::: devtools/client/netmonitor/reducers/ui.js:11
(Diff revision 6)
>  
>  const I = require("devtools/client/shared/vendor/immutable");
>  const {
>    OPEN_SIDEBAR,
>    WATERFALL_RESIZE,
> +  OPEN_STATISTICS,

nit: keep alphabetical order

::: devtools/client/netmonitor/reducers/ui.js:17
(Diff revision 6)
>  } = require("../constants");
>  
>  const UI = I.Record({
>    sidebarOpen: false,
>    waterfallWidth: 300,
> +  statisticsOpen: false,

nit: keep alphabetical order

::: devtools/client/netmonitor/reducers/ui.js:31
(Diff revision 6)
>  
>  function resizeWaterfall(state, action) {
>    return state.set("waterfallWidth", action.width - REQUESTS_WATERFALL_SAFE_BOUNDS);
>  }
>  
> +function openStatistics(state, action) {

nit: keep alphabetical order

::: devtools/client/netmonitor/reducers/ui.js:41
(Diff revision 6)
>    switch (action.type) {
>      case OPEN_SIDEBAR:
>        return openSidebar(state, action);
>      case WATERFALL_RESIZE:
>        return resizeWaterfall(state, action);
> +    case OPEN_STATISTICS:

nit: keep alphabetical order

::: devtools/client/netmonitor/requests-menu-view.js:57
(Diff revision 6)
>  }
>  
>  /**
>   * Functions handling the requests menu (containing details about each request,
>   * like status, method, file, domain, as well as a waterfall representing
> - * timing information).
> + * timing imformation).

nit: information

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91350/diff/6-7/

Comment 23

[Steve Chung [:steveck]]

Patch updated. Thanks for the review!

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review99082

Looks good, thanks for working on this!

Please see my inline comments.

Honza

::: devtools/client/netmonitor/netmonitor-view.js:57
(Diff revision 7)
> +    this.unsubscribeStore = gStore.subscribe(storeWatcher(
> +      false,
> +      () => gStore.getState().ui.statisticsOpen,
> +      this.toggleFrontendMode.bind(this)
> +    ));

It would be great to have a comment that explains why this code is here and when is the callback (toggleFrontEndMode) executed.

::: devtools/client/netmonitor/netmonitor-view.js:162
(Diff revision 7)
>  
>    /**
>     * Toggles between the frontend view modes ("Inspector" vs. "Statistics").
>     */
>    toggleFrontendMode: function () {
> -    if (this.currentFrontendMode != "network-inspector-view") {
> +    if (!gStore.getState().ui.statisticsOpen) {

The ! operator should be removed.

::: devtools/client/netmonitor/netmonitor-view.js:281
(Diff revision 7)
> +// A smart store watcher to notify store changes as necessary
> +function storeWatcher(initialValue, reduceValue, onChange) {
> +  let currentValue = initialValue;
> +
> +  return () => {
> +    const newValue = reduceValue();
> +    if (newValue !== currentValue) {
> +      onChange();
> +      currentValue = newValue;
> +    }
> +  };
> +}

Shouldn't you use the storeWatcher() helper from requests-menu-view.js to avoid duplication?

::: devtools/client/netmonitor/netmonitor.xul:482
(Diff revision 7)
>      <box id="network-statistics-view">
>        <toolbar id="network-statistics-toolbar"
>                 class="devtools-toolbar">
> -        <button id="network-statistics-back-button"
> -                class="devtools-toolbarbutton"
> +        <html:div xmlns="http://www.w3.org/1999/xhtml"
> +                  id="react-statistics-back-hook"/>
> -                data-localization="label=netmonitor.backButton"/>
>        </toolbar>
>        <box id="network-statistics-charts"
>             class="devtools-responsive-container"
>             flex="1">
>          <vbox id="primed-cache-chart" pack="center" flex="1"/>
>          <splitter id="network-statistics-view-splitter"
>                    class="devtools-side-splitter"/>
>          <vbox id="empty-cache-chart" pack="center" flex="1"/>
>        </box>
>      </box>

I was hoping that this markup (the entire "network-statistics-view" xul:box) would shrink to something like as follows after the refactoring:

<html:div xmlns="http://www.w3.org/1999/xhtml" id="react-statistics-view-hook"/>

At this moment, I can see that the panel is still using:

- xul:toolbar
- xul:button 
- xul:splitter
- xul:box
- xul:vbox

Can you please also remove that?

It's ok if this is done in separate bug report, but please make sure it's reported (there is no other bug related to the stats view AFAIK).

As far as the Splitter is concerned, it might be replaced by a static <div> (workaround) since it doesn't work anyway. Consequently it might be done/fixed as part of bug 1309183. I'll let you to decide!


Honza

Comment 25

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91350/diff/7-8/

Comment 26

[Steve Chung [:steveck]]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

I modified outer box and toolbar element and changed them to div, and left some comments for the store watcher part. I dup the watcher since they should be removed in the end, but please let me know if you prefer to move the watcher into utils for reusing.

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8808507 [details]
Bug 1308697 - Implement UI for performance statistics.

https://reviewboard.mozilla.org/r/91350/#review99394

This is very close to landing, please see my inline comments.

Honza

::: devtools/client/netmonitor/netmonitor-view.js:164
(Diff revisions 7 - 8)
> -    if (!gStore.getState().ui.statisticsOpen) {
> +    if (gStore.getState().ui.statisticsOpen) {
> -      this.showNetworkInspectorView();
> -    } else {
>        this.showNetworkStatisticsView();
> +    } else {
> +      this.showNetworkInspectorView();

This still doesn't look good to me. I am still not able to open the Statistics view through request-list context menu.

STR:

1) Right click on a request
2) Pick "Start Performance Analysis..."
3) The Perf view doesn't open -> BUG

Honza

::: devtools/client/netmonitor/netmonitor.xul:496
(Diff revisions 7 - 8)
>          <vbox id="primed-cache-chart" pack="center" flex="1"/>
>          <splitter id="network-statistics-view-splitter"
>                    class="devtools-side-splitter"/>
>          <vbox id="empty-cache-chart" pack="center" flex="1"/>
>        </box>
> -    </box>
> +    </html:div>

Great, nicely using <div>s to remove xul elements!

Make sure there is bug reported for the rest of xul elements, thanks!

Comment 28

[Steve Chung [:steveck]]

Attachment: Bug 1308697 - Part 2: Fix statistics view item in request list context menu.

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

Comment 29

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8819735 [details]
Bug 1308697 - Part 2: Fix statistics view item in request list context menu.

https://reviewboard.mozilla.org/r/99382/#review99728

I'm find with importing redux store and action in request-list-context-menu.js, because the request-list-context-menu.js ends up should be removed and use a react component instead.

Comment 30

[Steve Chung [:steveck]]

Thanks for all the review! I create a follow up bug 1324334 for the chart part.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4012349b4188
Implement UI for performance statistics. r=rickychien
https://hg.mozilla.org/integration/autoland/rev/410e5d477c0d
Part 2: Fix statistics view item in request list context menu. r=rickychien

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4012349b4188
https://hg.mozilla.org/mozilla-central/rev/410e5d477c0d

Comment 33

[Ciprian Georgiu, Desktop QA]

I can confirm that, this issue is verified fixed on latest Nightly 53.0a1 (2016-12-29) under the following OSes:
- Windows 10 x64
- Ubuntu 16.04 x64 LTS
- Mac OS X 10.11.6