Move Performance Statistics into its own module

VERIFIED FIXED in Firefox 52

Status

defect
P1
normal
VERIFIED FIXED
3 years ago
a year ago

People

(Reporter: rickychien, Assigned: steveck)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(Whiteboard: [netmonitor])

Attachments

(1 attachment)

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
Comment hidden (mozreview-request)
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
Assignee

Comment 6

3 years ago
(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
Assignee

Comment 7

3 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
Assignee

Comment 13

3 years ago
mozreview-review-reply
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+
Comment hidden (mozreview-request)
Assignee

Comment 16

3 years ago
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
Assignee

Comment 18

3 years ago
(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)
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 19

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/61627f78c095
Move statistics view into standalone module. r=Honza
Keywords: checkin-needed

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61627f78c095
Status: ASSIGNED → RESOLVED
Last Resolved: 3 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+

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.