Closed Bug 1308425 Opened 8 years ago Closed 8 years ago

Move Performance Statistics into its own module

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox52 verified)

VERIFIED FIXED
Firefox 52
Iteration:
52.3 - Nov 14
Tracking Status
firefox52 --- verified

People

(Reporter: rickychien, Assigned: steveck)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

We’re going to divide NetmonitorView into smaller pieces of code. Performance Statistics is a good entry point to get started since the module reference is not so complex.
Performance Statistics should be divided into a separate module file and loaded by NetmonitorView.
- Move PerformanceStatistics module from NetmonitorView to a separate file.
- Load PerformanceStatistics module in netmonitor-view.js
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Whiteboard: [devtools-html]
UI of this feature takes over the entire panel content and displays new thing. We might want to have other features that are using the same concept. E.g. when merging the Net panel with Web Sockets panel (and perhaps with WebRTC panel).

So, this might be split into smaller tasks:
Follow-ups: Implement a component for shared (virtualized) net panel content space
Follow-ups: Implement UI for performance analysis. Use d3.js instead of chart.jsm
Whiteboard: [devtools-html]
Depends on: 1308696
Depends on: 1308697
Blocks: 1308696
No longer depends on: 1308696
Blocks: 1308697
No longer depends on: 1308697
Whiteboard: [netmonitor]
Flags: qe-verify+
QA Contact: ciprian.georgiu
Priority: -- → P2
Hi Steve, want to confirm you are working on this bug?
Flags: needinfo?(schung)
(In reply to Ricky Chien [:rickychien] from comment #2)
> So, this might be split into smaller tasks:
> Follow-ups: Implement a component for shared (virtualized) net panel content
> space
Reported as bug 1308696

Honza
(In reply to Marco Mucci [:MarcoM] from comment #4)
> Hi Steve, want to confirm you are working on this bug?

Yes, I'll work on this one.
Assignee: nobody → schung
Flags: needinfo?(schung)
Status: NEW → ASSIGNED
Iteration: --- → 52.3 - Nov 7
Priority: P2 → P1
Comment on attachment 8802455 [details]
Bug 1308425 - Move statistics view into standalone module.

Hi Honza, it's the first step that simply moved the statistics view into another js(with some workaround that makes the request working. Ricky's RequestMenu patch would refactor that part). There's 2 options in my mind:
- Land this patch in this bug(after rebase) and apply React/Redux(moving the view status into action and adding to store) in bug 1308697, or
- Land this patch with moving the view status into action, then apply React in bug 1308697.

What you think?
Attachment #8802455 - Flags: feedback?(odvarko)
Comment on attachment 8802455 [details]
Bug 1308425 - Move statistics view into standalone module.

(In reply to Steve Chung [:steveck] from comment #7)
> Comment on attachment 8802455 [details]
> Bug 1308425 - Part 1: Move statistics view into standalone module.
> 
> Hi Honza, it's the first step that simply moved the statistics view into
> another js(with some workaround that makes the request working. Ricky's
> RequestMenu patch would refactor that part). There's 2 options in my mind:
> - Land this patch in this bug(after rebase) and apply React/Redux(moving the
> view status into action and adding to store) in bug 1308697, or
> - Land this patch with moving the view status into action, then apply React
> in bug 1308697.

I am unsure what you mean by "the view status". But, I would definitely recommend to use this bug to only move PerformanceStatisticsView into its own module. Any other changes that are *not* required for the standalone module to exist should be done in bug 1308697 (together with Reaact/Redux changes).

Also, the name of the new file is statistics-view.js, but the object implemented in is `PerformanceStatisticsView`. I think the file name should correspond to the object name and should be changed to performance-statistics-view.js

Otherwise I am sure you are on the right track, nice work!

Honza
Attachment #8802455 - Flags: feedback?(odvarko) → feedback+
Comment on attachment 8802455 [details]
Bug 1308425 - Move statistics view into standalone module.

https://reviewboard.mozilla.org/r/86860/#review88114

Thanks for the patch!

See my inline comments.

Honza

::: devtools/client/netmonitor/netmonitor-view.js:8
(Diff revision 3)
>  /* 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/. */
>  /* import-globals-from ./netmonitor-controller.js */
>  /* globals Prefs, gNetwork, setInterval, setTimeout, clearInterval, clearTimeout, btoa */
> -/* exported $, $all */
> +/* exported $, $all, responseIsFresh */

When responseIsFresh() is moved into performance-statistics-view.js the 'responseIsFresh' eslint global can be removed.

::: devtools/client/netmonitor/netmonitor-view.js:31
(Diff revision 3)
>  const {L10N} = require("./l10n");
>  const {RequestsMenuView} = require("./requests-menu-view");
>  const {CustomRequestView} = require("./custom-request-view");
>  const {ToolbarView} = require("./toolbar-view");
>  const {configureStore} = require("./store");
> -const Actions = require("./actions/index");
> +const {PerformanceStatisticsView} = require("./performance-statistics-view");

Removing the 'Actions' variable from here causes tests to faile (see also the try run).

It's used in netmonitor-controlls.js (since both these files are included in netmonitor.xul)

I think we should require it in netmonitor-controlls.js

I can't wait till all is properly 'required'!

::: devtools/client/netmonitor/netmonitor-view.js
(Diff revision 3)
> -   *        An object containing all or some the following properties:
> -   *          - id: either "#primed-cache-chart" or "#empty-cache-chart"
> -   *          - title/data/strings/totals/sorted: @see Chart.jsm for details
> -   */
> -  _createChart: function ({ id, title, data, strings, totals, sorted }) {
> -    let container = $(id);

This line is now in performance-statistics-view.js, but '$' function isn't defined (required) in this scope.

::: devtools/client/netmonitor/netmonitor-view.js:1199
(Diff revision 3)
> -
> -/**
>   * DOM query helper.
>   */
>  var $ = (selector, target = document) => target.querySelector(selector);
>  var $all = (selector, target = document) => target.querySelectorAll(selector);

What if we moved `$` and `$all` into `dom-utils.js` module and 'require' it whenever needed?
(e.g. in 'performance-statistics-view.js', etc.)

It's currently shared throug the 'window' object (monitor.panelWin), which is something we want to avoid in the future.

::: devtools/client/netmonitor/performance-statistics-view.js
(Diff revision 3)
> - * @param object
> - *        An object containing the { responseHeaders, status } properties.
> - * @return boolean
> - *         True if the response is fresh and loaded from cache.
> - */
> -function responseIsFresh({ responseHeaders, status }) {

responseIsFresh function can be moved into performance-statistics-view.js as well since it's only used there.

If done, it'll also fix eslint error about defining, but never using this function.
Attachment #8802455 - Flags: review?(odvarko)
Comment on attachment 8802455 [details]
Bug 1308425 - Move statistics view into standalone module.

https://reviewboard.mozilla.org/r/86860/#review88114

> This line is now in performance-statistics-view.js, but '$' function isn't defined (required) in this scope.

I think it's just lint error that I didn't declare it as global at the top, but it should just work since it's defined in netmonitor-view instead of other module.

> What if we moved `$` and `$all` into `dom-utils.js` module and 'require' it whenever needed?
> (e.g. in 'performance-statistics-view.js', etc.)
> 
> It's currently shared throug the 'window' object (monitor.panelWin), which is something we want to avoid in the future.

I totally agree here, and I think it might be better to have a specific bug to address this and move all the ui related functions into this utils. So I simply add a TODO comment here.
Comment on attachment 8802455 [details]
Bug 1308425 - Move statistics view into standalone module.

https://reviewboard.mozilla.org/r/86860/#review88334

Looks great to me!

R+ assuming Try is green.

There is also one nit, please resolve.

Also, please file a follow-up for the dom-utils.js module so, it isn't forgotten.

Thanks!
Honza

::: devtools/client/netmonitor/netmonitor-view.js:30
(Diff revisions 3 - 4)
> + * @param object
> + *        An object containing the { responseHeaders, status } properties.
> + * @return boolean
> + *         True if the response is fresh and loaded from cache.
> -   */
> + */
> -  _initializePanes: function () {
> +function responseIsFresh({ responseHeaders, status }) {

nit: I think that all helper functions should be located at the bottom of the module - after PerformanceStatisticsView object.
Attachment #8802455 - Flags: review?(odvarko) → review+
Thanks for the review!
Keywords: checkin-needed
can you check mozreview to fix the issues there so we can land this via autoland ? Thanks!
Flags: needinfo?(schung)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #17)
> can you check mozreview to fix the issues there so we can land this via
> autoland ? Thanks!

All the issues were closed as fixed, thanks.
Flags: needinfo?(schung)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/61627f78c095
Move statistics view into standalone module. r=Honza
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/61627f78c095
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
This issue is verified fixed on latest Nightly 52.0a1 (Build ID 20161108030212) across platforms:
- MacOS X 10.11
- Windows 10 x64
- Ubuntu 14.04 x86 LTS

I didn't encountered any issues during testing and based on that, I will mark this bug as verified fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.