1324334 - Migrate Chart.jsm to js and remove xul components in the chart

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Steve Chung [:steveck]]

We'll need to replace all the xul components with html element or React component for the chart display. Using other libraries like d3 is not a priority right, but we could file a follow up for it if it does bring any benefit.

Comment 1

[Steve Chung [:steveck]]

Attachment: Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

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

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8821395 [details]
Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

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

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8821395 [details]
Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

https://reviewboard.mozilla.org/r/100692/#review101248

::: devtools/client/netmonitor/netmonitor.xul:291
(Diff revision 2)
>        <html:div id="network-statistics-toolbar"
> -               class="devtools-toolbar">
> +                class="devtools-toolbar">
>          <html:div xmlns="http://www.w3.org/1999/xhtml"
>                    id="react-statistics-back-hook"/>
>        </html:div>
> -      <box id="network-statistics-charts"
> +      <html:div id="network-statistics-charts"

I'm still thinking that can we reduce these static html tags in netmonitor.xul?
Netmonitor is going to de-xul and adopt react architecture soon. Static html tags is supposed to be removed as well. So we might think about moving these code to javascript.

How do you think if we create a react chart component which wraps the current chart.js and create these static element with react itself?

My current WIP patch of ParamsPanel is wrapping the editor like this way. Please see also https://bugzilla.mozilla.org/show_bug.cgi?id=1317650

::: devtools/client/themes/netmonitor.css:898
(Diff revision 2)
>  
>  #network-statistics-charts {
>    background-color: var(--theme-sidebar-background);
>  }
>  
> +#network-statistics-charts,

nit: I think this part can be moved to the end of css file. There has an identical css rule for -moz-box

#react-params-tabpanel-hook,
#react-preview-tabpanel-hook,
#react-security-tabpanel-hook,
#react-timings-tabpanel-hook {
  display: -moz-box;
  -moz-box-orient: vertical;
  -moz-box-flex: 1;
}

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8821395 [details]
Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

https://reviewboard.mozilla.org/r/100692/#review101254

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8821395 [details]
Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

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

Comment 6

[Steve Chung [:steveck]]

After some discussion with Ricky, I think moving the Chart.js into React component would need to rewrite the Chart module, and it might be better if we create a follow up to implement a new Chart React component and still leave the original Chart.js for csscoverage.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8821395 [details]
Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

https://reviewboard.mozilla.org/r/100692/#review102444

This patch looks good to me. Let's wait for Honza's review and maybe he has other opinions.

Comment 8

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

Comment on attachment 8821395 [details]
Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

https://reviewboard.mozilla.org/r/100692/#review102640

The patch looks good to me, R+ assuming Try is green.

Please don't forget to file a follow up for implementing Chart react component that reduces amount of static HTML markup in netmonitor.xul.

Thanks!
Honza

Comment 9

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8821395 [details]
Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

https://reviewboard.mozilla.org/r/100692/#review102662

::: devtools/client/netmonitor/statistics-view.js:18
(Diff revision 3)
>  const { EVENTS } = require("./events");
>  const { DOM } = require("devtools/client/shared/vendor/react");
>  const { button } = DOM;
>  const ReactDOM = require("devtools/client/shared/vendor/react-dom");
>  const Actions = require("./actions/index");
> +const { Chart } = require("devtools/client/shared/widgets/Chart");

just a little nit: the module filename should be lowercase - chart.js - to be consistent with other JS modules.

::: devtools/client/netmonitor/statistics-view.js:38
(Diff revision 3)
>    /**
>     * Initialization function, called when the statistics view is started.
>     */
>    initialize: function (store) {
>      this.store = store;
> +    this.chart = Chart;

another little nit: this property (used only by tests, right?) could have a camel-case name:

this.Chart = Chart;

The tests can then access the property with destructuring:

const { Chart } = NetMonitorView.Statistics;

Not a big deal, just slightly more elegant. And maybe eslint will complain about the camel-case name, in which case the suggestion is not worth doing.

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8821395 [details]
Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

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

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8821395 [details]
Bug 1324334 - Migrate Chart.jsm to js and remove xul components in the chart,

https://reviewboard.mozilla.org/r/100692/#review102662

> just a little nit: the module filename should be lowercase - chart.js - to be consistent with other JS modules.

Since the widgets are all using uppercase, I'll just copy another chart React component just for netmonitor in the next follow up and still keet the original Chart in widgets.

> another little nit: this property (used only by tests, right?) could have a camel-case name:
> 
> this.Chart = Chart;
> 
> The tests can then access the property with destructuring:
> 
> const { Chart } = NetMonitorView.Statistics;
> 
> Not a big deal, just slightly more elegant. And maybe eslint will complain about the camel-case name, in which case the suggestion is not worth doing.

Yeah you're right that it's simply for the test... I think we might refactor the test part in the follow up as well.

Comment 12

[Steve Chung [:steveck]]

Thanks for all the feedback and suggestions! I created bug 1328812 for the React Chart component follow up.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ee4d028ba89
Migrate Chart.jsm to js and remove xul components in the chart, r=Honza,rickychien

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/8ee4d028ba89

Comment 15

[Ciprian Georgiu, Desktop QA]

I performed tests around Chart display on latest DevEdition 53.0a2 (2017-01-24), and I can confirm that is working accordingly under the following OSes:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 LTS x64