Migrate RequestsMenuView to a React component with Redux store

VERIFIED FIXED in Firefox 53

Status

defect
P1
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: jsnajdr, Assigned: jsnajdr)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
Firefox 53
Dependency tree / graph

Firefox Tracking Flags

(firefox53 verified)

Details

(Whiteboard: [netmonitor])

Attachments

(5 attachments)

Replace the RequestsMenuView XUL component with a React/Redux version:
- move the model logic to a Redux store
- move the view logic to a React component
- make sure that all tests still pass
Assignee: nobody → jsnajdr
Status: NEW → ASSIGNED
Whiteboard: [netmonitor]
Flags: qe-verify?
QA Contact: ciprian.georgiu
Priority: -- → P1
Updated the WIP patch at my github fork repo: https://github.com/jsnajdr/gecko-dev

All mochitests are passing except:
- browser_net_autoscroll.js
- browser_net_sort-01.js
- browser_net_image-tooltip.js
- browser_net_security-icon-click.js
- browser_net_timeline_ticks.js

These tests are disabled. In all cases the tested feature works, but the test needs nontrivial refactoring, because it relies on too many implementation details.

Next steps:
- make it fast by using immutable.js and reselect
- fix l10n after bug 1308500 is done
- fix CSS styles in header (sort arrows, timeline ticks etc)
- convert parts of mochitests to unit tests
Iteration: --- → 52.3 - Nov 7
Progress over the last week:
- Immutable used for the state including the request list
- Reselect used to optimize getting computed data from the store
- merged in the l10n changes, l10n is 100% correct now
- fixed a lot of styling issues - header alignment, sort arrows, empty notice, Firebug theme glitches
- merged in the changes by Fred and Ricky - filter buttons, clear button, context menu, ...
- fixed several remaining failing tests

Next steps:
- fix keyboard focus when navigating
- fix glitches in displaying the waterfall
- fix the autoscroll test
- make the UI work correctly in RTL enviroments
- add more Immutable.js, e.g., the request list item is still a plain object
- more optimizations

Latest code is in my Github fork repo: https://github.com/jsnajdr/gecko-dev
Still a lot of work to do, but posting the patch for the first review anyway. I'll write a guide to the changes soon.
Depends on: 1315175
I am seeing one test failure
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba5c3f4b19e99ce2c70029c102adfc28ef7a93ad&selectedJob=30402121

devtools/client/inspector/test/browser_inspector_addSidebarTab.js | My Panel is selected by default - Got ruleview, expected myPanel

chrome://mochikit/content/browser-test.js:test_is:909
chrome://mochitests/content/browser/devtools/client/inspector/test/browser_inspector_addSidebarTab.js:null:38

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> I am seeing one test failure

That's a try run for bug 1297651, not related to this one.
(In reply to Jarda Snajdr [:jsnajdr] from comment #6)
> (In reply to Jan Honza Odvarko [:Honza] from comment #5)
> > I am seeing one test failure
> 
> That's a try run for bug 1297651, not related to this one.
Ah, sorry, I mixed up try runs.

Honza
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review90342

Great job here, first round of reviews...


Some general comments:

* WebSocket Monitor is broken. I am seeing 'TypeError: NetMonitorView.RequestsMenu.getItemByValue is not a function:'. We should probably file a bug and investigate.
* More comments wouldn't hurt. Especially introductionary comments for React components and some helper methods. Also some actions could be briefly explained
* Keep index.js files small (see inline comments)
* Some suggesions related to Action creators might be done as a follow up
* I skipped test changes, will do it in next round
* It would be great if :ntim could look at the CSS changes.
* Somebody else should definitelly be asked for review or feedback so, we have more eyes on this huge patch.


Thanks Jarda!


Honza

::: devtools/client/netmonitor/actions/index.js:12
(Diff revision 1)
>  const filters = require("./filters");
>  const sidebar = require("./sidebar");
>  
> -module.exports = Object.assign({}, filters, sidebar);
> +Object.assign(exports, filters, sidebar);
> +
> +exports.batchActions = (actions) => {

Can we have all request-related actions in separate file and keep the index.js just for joining all the action files?

I think that in general it would be great if index files are just small files including other files.

Honza

::: devtools/client/netmonitor/actions/index.js:96
(Diff revision 1)
> +  };
> +};
> +
> +const PAGE_SIZE_ITEM_COUNT_RATIO = 5;
> +
> +exports.selectDelta = (delta) => {

nit: Please add a comment explaning possible argument values.

::: devtools/client/netmonitor/components/request-list-empty.js:1
(Diff revision 1)
> +"use strict";

Missing Mozilla license header

::: devtools/client/netmonitor/components/request-list-empty.js:7
(Diff revision 1)
> +
> +const { createClass, PropTypes, DOM } = require("devtools/client/shared/vendor/react");
> +const { L10N } = require("../l10n");
> +const { div, span, button } = DOM;
> +
> +const RequestListEmptyNotice = createClass({

Please append a comment explaining why we have the component. We should do this for every component.

::: devtools/client/netmonitor/components/request-list-header.js:42
(Diff revision 1)
> +    firstDocumentDOMContentLoadedTimestamp: state.firstDocumentDOMContentLoadedTimestamp,
> +    firstDocumentLoadTimestamp: state.firstDocumentLoadTimestamp,
> +  };
> +}
> +
> +const RequestListHeader = createClass({

Missing displayName property (this is true also for the other components)

::: devtools/client/netmonitor/components/request-list-item.js:1
(Diff revision 1)
> +/* globals window */

Missing Mozilla license header

::: devtools/client/netmonitor/components/request-list-item.js:30
(Diff revision 1)
> + * Get a human-readable string from a number of bytes, with the B, KB, MB, or
> + * GB value. Note that the transition between abbreviations is by 1000 rather
> + * than 1024 in order to keep the displayed digits smaller as "1016 KB" is
> + * more awkward than 0.99 MB"
> + */
> +function getFormattedSize(bytes) {

This should be in utils, I am sure the StatisticsView can use it too.

::: devtools/client/netmonitor/components/request-list-item.js:327
(Diff revision 1)
> +  componentWillUnmount() {
> +    // If this node is being destroyed and has focus, transfer the focus manually
> +    // to the parent tree component. Otherwise, the focus will get lost and keyboard
> +    // navigation in the tree will stop working. This is a workaround for a XUL bug.
> +    // See bugs 1259228 and 1152441 for details.
> +    // DE-XUL: Remove this hack once all usages are only in HTML documents.

Should we have a bug report for this so, it isn't forgotten?

::: devtools/client/netmonitor/components/request-list.js:1
(Diff revision 1)
> +/* globals $, gNetwork, NetMonitorController */

Missing Mozilla license header

::: devtools/client/netmonitor/components/request-list.js:121
(Diff revision 1)
> +  let listRect = list.getBoundingClientRect();
> +
> +  return (childRect.height + childRect.top) <= listRect.bottom;
> +}
> +
> +const RequestListContent = createFactory(createClass({

Could we put all the helper method at the bottom of the file? The component is usually the most interesting thing in the file (inside components dir) ant it wouild be nice to have it at the top + introductory comment.

::: devtools/client/netmonitor/netmonitor-controller.js:534
(Diff revision 1)
>     * @param object marker
>     */
>    _onDocLoadingMarker: function (marker) {
>      window.emit(EVENTS.TIMELINE_EVENT, marker);
>      this._markers.push(marker);
> +    NetMonitorView.RequestsMenu.addTimingMarker(marker);

What about to fire the action directly? The 'addTimingMarker' isn't used anywhere else.

::: devtools/client/netmonitor/netmonitor-controller.js:534
(Diff revision 1)
>     * @param object marker
>     */
>    _onDocLoadingMarker: function (marker) {
>      window.emit(EVENTS.TIMELINE_EVENT, marker);
>      this._markers.push(marker);
> +    NetMonitorView.RequestsMenu.addTimingMarker(marker);

What about firing action here and removing the addTimingMarker() helper? It's used only here anyway.

::: devtools/client/netmonitor/netmonitor-controller.js:556
(Diff revision 1)
>        fromCache,
>        fromServiceWorker
>      } = networkInfo;
>  
>      NetMonitorView.RequestsMenu.addRequest(
> -      actor, startedDateTime, method, url, isXHR, cause, fromCache,
> +      actor, {startedDateTime, method, url, isXHR, cause, fromCache, fromServiceWorker}

Similarly here, what about firing an action and remove `NetMonitorView.RequestsMenu.addRequest` helper?

::: devtools/client/netmonitor/netmonitor-controller.js:558
(Diff revision 1)
>  
>      NetMonitorView.RequestsMenu.addRequest(
> -      actor, startedDateTime, method, url, isXHR, cause, fromCache,
> +      actor, {startedDateTime, method, url, isXHR, cause, fromCache, fromServiceWorker}
> -        fromServiceWorker
>      );
>      window.emit(EVENTS.NETWORK_EVENT, actor);

The same here what if we fired an action directly? No additional addRequest helper. (see another comment related to addRequest)

::: devtools/client/netmonitor/requests-menu-view.js:115
(Diff revision 1)
> +    onReloadClick: e => NetMonitorView.reloadPage(),
> +  });
> +}
>  
>  // A smart store watcher to notify store changes as necessary
>  function storeWatcher(initialValue, reduceValue, onChange) {

request-menu-view.js is huge and I think that we should use every opportunity to make it smaller. What if we moved the storeWatcher helper to utils?
(if there is more such things it can be follow up)

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

nit: typo, 'imformation'

::: devtools/client/netmonitor/requests-menu-view.js:158
(Diff revision 1)
> -    this.tooltip.startTogglingOnHover(widgetParentEl, this._onHover, {
> -      toggleDelay: REQUESTS_TOOLTIP_TOGGLE_DELAY,
> -      interactive: true
> -    });
> +      null,
> +      () => this.selectedItem,
> +      (newSelected, oldSelected) => {
> +        if (newSelected && oldSelected && newSelected.id == oldSelected.id) {
> +          // The same item is still selected, it only got updated
> +          this.onSelectChange(newSelected);

If this is about update, shouldn't onSelectChange be renamed t onUpdate or something?

::: devtools/client/netmonitor/requests-menu-view.js:291
(Diff revision 1)
> +
> +  empty() {
> +    this.store.dispatch(Actions.clearRequests());
> +  },
> +
> +  addRequest(id, data) {

Shouldn't the entire addRequest method be converted into action creator? (i.e. move the entire logic into addRequest() action)

::: devtools/client/netmonitor/requests-menu-view.js:291
(Diff revision 1)
> +
> +  empty() {
> +    this.store.dispatch(Actions.clearRequests());
> +  },
> +
> +  addRequest(id, data) {

Could we move all the logic in this method into the action creator (addRequest)?

::: devtools/client/netmonitor/selectors/index.js:14
(Diff revision 1)
> +
> +/**
> + * Check if the given requests is a clone, find and return the original request if it is.
> + * Cloned requests are sorted by comparing the original ones.
> + */
> +function getOrigRequest(requests, req) {

Similarly to actions & reducer index file it would be nice to have request-selectores in request.js selector file.
Attachment #8807207 - Flags: review?(odvarko)
New version of the patch with the following changes:

Timing markers (DOMContentLoaded, DocumentLoaded)
- separate modules for actions and reducers, imported and composed in the index.js files
- dispatch actions everywhere, removed helper methods (addTimingMarker, clearTimingMarkers)
- removed all timing markers logic from netmonitor-controller.js. Now it just dispatches the timing actions and all the work is done by Redux.

Context menu:
- rebased on top of bug 1315175 (you need that patch to apply this one)
- requests-menu-view.js file is getting really small and easy to understand!

Use constants from constants.js for all actions

Removed the onCountUpdate handler from RequestsMenuView - it was used to update the sidebar, now handled by React/Redux

Removed the focusPrevItem el al. methods from RequestsMenuView - fire "selectDelta" actions directly.
(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> * WebSocket Monitor is broken. I am seeing 'TypeError:
> NetMonitorView.RequestsMenu.getItemByValue is not a function:'. We should
> probably file a bug and investigate.

"getItemByValue" is a method of the original XUL widget (SideMenuView). Now the internal API of the view is completely different, of course - if you want to get a request object by ID, you use the getRequestById selector. And remember that the returned request is no longer a JS object, but an Immutable.Record.

Two ways how to solve this:
1. Fix the WebSocket inspector to use the selector and the React/Redux API
2. Keep a method "getItemByValue" for extension compatibility

> * Keep index.js files small (see inline comments)

Yes, I'll be factoring the actions and reducers into separate smaller files.

> Similarly here, what about firing an action and remove
> `NetMonitorView.RequestsMenu.addRequest` helper?

The addRequest and updateRequest actions are put into a queue, which is flushed as a batch every 50ms. We could create a Redux middleware for this. But I think that it's one of the great opportunities where using Observables from most.js would be very useful. You create an observable stream of addRequest/updateRequest actions, implement the batching as a simple stream operator, and then feed the result to Redux. I'm going to experiment with this in the following few days.

> request-menu-view.js is huge and I think that we should use every
> opportunity to make it smaller. What if we moved the storeWatcher helper to
> utils?
> (if there is more such things it can be follow up)

It helped a lot that the context menu was moved elsewhere - now the requests-menu-view.js is getting quite small. I'd keep storeWatcher here - it's going to disappear soon anyway, as the toolbar and sidebar are moved to React/Redux.

I don't have any comment on the other issues you reported - I'll fix them.
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review91186

The CSS looks fine, I would have to test the patch to find more issues, but I think Honza has done enough testing :)

::: devtools/client/themes/netmonitor.css:125
(Diff revision 2)
>  #notice-perf-message {
>    margin-top: 2px;
>  }
>  
>  #requests-menu-perf-notice-button {
> +  font-size: inherit;

If I'm not wrong, #requests-menu-perf-notice-button is the notice button that only has the stopwatch icon in it. So font-size: inherit shouldn't be needed.

::: devtools/client/themes/netmonitor.css:157
(Diff revision 2)
>  .theme-firebug #requests-menu-toolbar {
> -  height: 16px !important;
> +  height: 19px !important;
> +}
> +
> +.requests-menu-contents {
> +  display: -moz-box;

Let's hope we can get rid of display: -moz-box someday :)

::: devtools/client/themes/netmonitor.css:205
(Diff revision 2)
> -  padding-bottom: 2px;
> -  padding-inline-start: 13px;
>    padding-top: 2px;
> +  padding-bottom: 2px;
> +  padding-inline-start: 16px;
> +  padding-inline-end: 0px;

nit: remove unit from 0px

::: devtools/client/themes/netmonitor.css:231
(Diff revision 2)
> -  display: -moz-box;
> +  white-space: nowrap;
> +  overflow: hidden;
> +  text-overflow: ellipsis;
> +}
> +
> +.requests-menu-header-button > .button-icon {

I suspect we can use just one element for both the header icon and the text.
.requests-menu-header-button for the text
and .requests-menu-header-button::after for the icon.

The storage inspector uses 1 element, so you should be able to look at that.

::: devtools/client/themes/netmonitor.css:321
(Diff revision 2)
>  
>  .requests-menu-icon {
>    outline: 1px solid var(--table-splitter-color);
>  }
>  
> +.requests-menu-file, .requests-menu-domain {

nit: remove this empty rule

::: devtools/client/themes/netmonitor.css:598
(Diff revision 2)
>  
>  .side-menu-widget-item-contents {
>    padding: 0px;
>  }
>  
> -.side-menu-widget-item:not(.selected)[odd] {
> +.side-menu-widget-item:not(.selected).odd {

I'd be nice if we could get rid of all the .side-menu-widget-* classes completely. These are classes provided specifically by SideMenuWidget.jsm (which the network monitor previously used).

Let's use class names that are in sync with the react component names.
Added a new version of the patch with the following changes:

Fixed displaying the waterfall in RTL mode. Now, RTL work perfectly (and much better than the original version!)

Divided the code into more modules:
- request-list-tooltip.js: Code for setting content of the image/code tooltip
- request-list-content.js: Render the request list content. The file request-list.js now only has code to compose other components together.
- utils/format-utils.js: New directory for utils files, and the format-utils file now contains getFormattedSize function

Reorganized how the components are connected with Redux. Now, every component where it makes sense has its own connect() call. I divided the big global mapStateToProps and mapDispatchToProps into several smaller ones.
Blocks: 1316291
New version of the patch contains:
- fixes for CSS issues reported by Tim (see the next comment for details)
- a lot of polish and fixes in the JS code
(In reply to Tim Nguyen :ntim (use needinfo?) from comment #12)
> If I'm not wrong, #requests-menu-perf-notice-button is the notice button
> that only has the stopwatch icon in it. So font-size: inherit shouldn't be
> needed.

Fixed.

> Let's hope we can get rid of display: -moz-box someday :)

We can remove it as soon as all XUL is gone and when Netmonitor is a HTML document with flex layout. -moz-box (aka XUL flex) and the HTML flex don't work together very well, so -moz-box is still necessary for HTML elements embedded inside XUL.

> nit: remove unit from 0px

Fixed.

> > +.requests-menu-header-button > .button-icon {
> 
> I suspect we can use just one element for both the header icon and the text.
> .requests-menu-header-button for the text
> and .requests-menu-header-button::after for the icon.
> 
> The storage inspector uses 1 element, so you should be able to look at that.

Hmm, I prefer to have explicit markup with class names instead of ::before/::after CSS tricks.

In this case, .requests-menu-header-button::after would insert a pseudo-element outside the button, which means the sorting arrow wouldn't be clickable.

Storage Inspector uses a background-image with the right alignment. I'll try to play with rendering the table header this way, but right now I think it's not very important.

> nit: remove this empty rule

Fixed.

> I'd be nice if we could get rid of all the .side-menu-widget-* classes
> completely. These are classes provided specifically by SideMenuWidget.jsm
> (which the network monitor previously used).
> 
> Let's use class names that are in sync with the react component names.

I removed the side-menu-widget-* classes in favor of request-list-item-*. I'm still keeping the requests-menu-* legacy class names, as it would inflate this patch to even bigger size. Filed a followup bug 1316291 for this.
The React RequestList has some performance issues when dealing with sites that make a lot of requests. (I'm testing on cnn.com right now, the list grows to 800 requests quickly)

I identified two causes:

1. Image thumbnails. We're displaying a 15x15px thumbnail for image requests, implemented as <img> element where the "src" is the whole original image serialized as a "data" URL. This amounts to megabytes of data that are rendered as strings. React 15 makes this visibly faster, but not much.

Possible solution: generate 15x15 thumbnails (downsample the image in a canvas) to reduce the data size. Or use Blob URLs instead of Data URLs.

1. Sorting is inefficient, especially in the default case, when sorting by startedMillis. The request list is re-sorted on every change, although the relevant startedMillis property never changes for a given request item.

Possible solution: optimize the sorting selector, at least for the default startedMillis case.
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review91842

::: devtools/client/netmonitor/components/request-list-header.js:41
(Diff revision 4)
> +/**
> + * Render the request list header with sorting arrows for columns.
> + * Displays tick marks in the waterfall column header.
> + * Also draws the waterfall background canvas and updates it when needed.
> + */
> +const RequestListHeader = createClass({

We're using react v0.14.6 which has supported ES6 classes, so please use new class syntax instead.

::: devtools/client/netmonitor/components/request-list-item.js:22
(Diff revision 4)
> +        getUriHostPort } = require("../request-utils");
> +
> +/**
> + * Render one row in the request list.
> + */
> +const RequestListItem = createClass({

the same. we can use ES6 class instead.

::: devtools/client/netmonitor/reducers/index.js:22
(Diff revision 4)
> +        PRESELECT_ITEM,
> +        SORT_BY,
> +        SHOW_SIDEBAR,
> +        WATERFALL_RESIZE } = require("../constants");
>  
> -module.exports = combineReducers({
> +const UPDATE_PROPS = [

Is there any reason to put these reducers within index.js?

I prefer to separate every reducer to their place with different files. And also the advantage of original pattern combineReducers({r1, r2, ...}) which provides a straightforward structure to understand entire application state.
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review91852

Please, see my inline comments.


Some general notes:

* The 'reaload' button displayed when the request list is empty uses bigger font.
* Font in the request list header is bigger.
* I pushed to try and I am seeing some test failures
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccb9d86e3a4c2c2c3776fb74469d1da085080f72&selectedJob=30805631


Honza

::: devtools/client/netmonitor/actions/index.js:22
(Diff revision 4)
> +        SORT_BY,
> +        WATERFALL_RESIZE } = require("../constants");
>  
> -module.exports = Object.assign({}, filters, sidebar);
> +Object.assign(exports, filters, sidebar, timingMarkers);
> +
> +exports.batchActions = (actions) => {

Shouldn't all the remaining actions in this file be moved to separate module (e.g. to 'requests.js')? Just like the Comment 10 says.

::: devtools/client/netmonitor/actions/sidebar.js
(Diff revision 4)
>  "use strict";
>  
> -const {
> +const { SHOW_SIDEBAR } = require("../constants");
> -  DISABLE_TOGGLE_BUTTON,
> -  SHOW_SIDEBAR,
> -  TOGGLE_SIDEBAR,

So, the TOGGLE_SIDEBAR should be changed to UPDATE_SIDEBAR (or UPDATE_SIDEBAR_VISIBILITY) to follow our discusssion from toda's meeting (follow up)

::: devtools/client/netmonitor/components/request-list-content.js:202
(Diff revision 4)
> +    onItemMouseDown: (e, item) => dispatch(Actions.selectItem(item)),
> +    onItemContextMenu: (e, item) => {
> +      e.preventDefault();
> +      NetMonitorView.RequestsMenu.contextMenu.open(e);
> +    },
> +    onKeyDown: (e) => {

I would rather vote to to implement event handlers  as regular methods of the RequestListContent component. onScroll and onHover are already there too.

Or it could be separate function in this module if having it implemented as a method isn't nice from some reason. 

In any case, it's nice to keep the connect() method short and simple.

::: devtools/client/netmonitor/components/request-list-header.js:130
(Diff revision 4)
> +
> +  // Insert one label for each division on the current scale.
> +  for (let x = 0; x < waterfallWidth; x += scaledStep) {
> +    let millisecondTime = x / scale;
> +
> +    let normalizedTime = millisecondTime;

It would be nice to have getFormattedTime() method in format-utils.js that does all that time label calculation.

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

Missing empty line between "use strict" and the Mozilla license header (also in other files)

::: devtools/client/netmonitor/custom-request-view.js:118
(Diff revision 4)
>    /**
>     * Update the query string field based on the url.
>     *
>     * @param object url
>     *        The URL to extract query string from.
>     */

nit: It isn't caused by this patch, but parseQueryText() method has typo in its comment:
'represetation' -> 'representation'

::: devtools/client/netmonitor/netmonitor-controller.js:294
(Diff revision 4)
>      // Look for the request in the existing ones or wait for it to appear, if
>      // the network monitor is still loading.
>      let deferred = promise.defer();
>      let request = null;
>      let inspector = function () {
> -      let predicate = i => i.value === requestId;
> +      request = getDisplayedRequestById(gStore.getState(), requestId);

It would be great to get rid of the global gStore variable. It's now shared between two files (netmonitor-controller.js and netmonitor-view.js).

It's fine to do it later in a follow up, but the sooner we do it the better (we could perhaps turn NetMonitorController into an object that needs to be instantiated and pass the store into it - at least as a workaround for now).

::: devtools/client/netmonitor/netmonitor-controller.js:434
(Diff revision 4)
>  };
>  
>  /**
>   * Functions handling target network events.
>   */
>  function NetworkEventsHandler() {

NetworkEventsHandler could be easily moved into separate module (a follow up)

::: devtools/client/netmonitor/store.js:11
(Diff revision 4)
> -const createStore = require("devtools/client/shared/redux/create-store");
> +const { createStore, compose, applyMiddleware } = require("devtools/client/shared/vendor/redux");
> +const { thunk } = require("devtools/client/shared/redux/middleware/thunk");
>  const reducers = require("./reducers/index");
>  
> +/**
> + * A enhancer for the store to handle batched actions.

More detailed comment about how batching work would be nice.
Attachment #8807207 - Flags: review?(odvarko)
Posted image timeline.png
One more thing, the last time-label in the Waterfall graph header is bold on my machine. See the attached screenshot.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #20)
> * The 'reaload' button displayed when the request list is empty uses bigger font.
> * Font in the request list header is bigger.

Seems like it is the same issue as reported in the summary button. I'll have a look at it.

> Shouldn't all the remaining actions in this file be moved to separate module
> (e.g. to 'requests.js')? Just like the Comment 10 says.

Yes, I'm working on it today - move all action creators and reducers out of the index.js file into dedicated modules.

It's not easy in some cases. Separate reducers (composed together with composeReducers) cannot see each other. Therefore, if a reducer for "requests" needs to see "selection" or "filters", it's not possible. Sometimes code needs to be moved to action creator, sometimes one action needs to be implemented in two reducers, each of them updating only its own state. Not extremely hard, but requires moving a lot of code around.

> I would rather vote to to implement event handlers  as regular methods of
> the RequestListContent component. onScroll and onHover are already there too.
> 
> Or it could be separate function in this module if having it implemented as
> a method isn't nice from some reason.

Yes, I'll refactor this, too.

> It would be great to get rid of the global gStore variable. It's now shared
> between two files (netmonitor-controller.js and netmonitor-view.js).

It's also used in many tests. We'll need to do a lot of work before we can remove it - move many tests from the integration to the unit level, unify the NetmonitorView and NetmonitorController etc.
Latest changes:
Decomposed the actions and reducers into separate modules. Selectors are still in one index.js file (todo)

Fixed a bug where the "status" icon wasn't displayed correctly in the Headers tab in sidebar.

Fixed the browser_net_reload-* tests - they were trying to click on a "reload" button in the empty notice. This worked in the old version where the notice was just hidden, but now it's not rendered at all.

Fixed many other style issues from review.
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review91852

> NetworkEventsHandler could be easily moved into separate module (a follow up)

This module is closely related to netmonitor-controller and at this moment I don't know at all what will happen to the controller. Let's file bugs when we know more. This work is not related much to this bug.
Posted image load-event.png
It looks like the load-event (red vertical line rendered across all entries) isn't properly displayed

STR:
1) Load http://janodvarko.cz/devtools/bugzilla/1309866/images.html
2) Open Toolbox, select the Net panel and reload the page
3) Check the load event. I am not seeing it -> BUG

I can see the load event properly in the old UI

Screenshot of what exactly I am seeing is attached (new UI at the top, old UI at the bottom of the screenshot)

Honza
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review90342

> Should we have a bug report for this so, it isn't forgotten?

Added a comment to bug 1309826, listing things that should be done when moving netmonitor.xul to netmonitor.html.
Changes in the latest version of the patch (interdiff 6-7):
- rebased on top of Ricky's summary button patch. A lot of code is much simpler now when all app state is in Redux and most UI is React.
- converted the "toggleSidebar" action into a thunk that converts it into "openSidebar". This means that the "toggleSidebar" action can be processed in reducers that don't know about the sidebar state - cool!
- distributed the selectors into several modules, "index.js" just puts them together now
- fixed all remaining mochitest failures (mostly outside the netmonitor directory)
- improved how "onKeyDown" is handled in request-list-content.js - the code that's in the connect() call is now much smaller. Once we convert the sidebar, we can use bindActionCreators for everything.
Hi Shako,

We'd like to compare devtools/network list loadtime between this patch and the nightly to find out if there's any performance regression.


As we discussed offline, you and askeing have develop some automation tool that can make the comparison easier, could you help us check the load time between nightly and the artifact in try server?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fcc6083ab22 (or you can find the latest on mozreview board)

The steps:
1. use shortcut Ctrl+Shift+Q to open network panel
2. load a web page and wait the list rendering complete
Flags: needinfo?(sho)
Here is a small static Node server that can be used to test the Netmonitor performance:

https://github.com/jsnajdr/netmonitor-load-tester

Feel free to add other useful pages there and submit a PR.
(In reply to Jan Honza Odvarko [:Honza] from comment #29)
> It looks like the load-event (red vertical line rendered across all entries)
> isn't properly displayed

Fixed in the latest interdiff (7-8). The load event happened a moment after the last request finished, so the line was outside the bounds of the background image.

I changed the computation of the waterfall scaling factor - it now takes the dom-content-loaded and load event times into account.

The patch is rebased on top of patches from bug 1285173 and bug 1309194 - these are not in m-c yet.
(In reply to Jarda Snajdr [:jsnajdr] from comment #36)
> Fixed in the latest interdiff (7-8). The load event happened a moment after
> the last request finished, so the line was outside the bounds of the
> background image.

Yep, I can confirm, works well now.
Honza
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review93328

::: devtools/client/netmonitor/actions/sort-by.js:13
(Diff revision 8)
> +
> +function sortBy(sortType) {
> +  return { type: SORT_BY, sortType };
> +}
> +
> +module.exports = {

I think split request-menu related actions into separate files are not very meaningful though, for me they all actions around requests. And its hard to realize sort-by, timing-marker,selection actions are all just related to request-menu. It also takes extra time to find out a single action in sort-by, timing-marker.

Maybe its just the naming issue, or we can put request-menu related actions  into a single or two action files.

::: devtools/client/netmonitor/components/request-list-item.js:280
(Diff revision 8)
> +    return boxes;
> +  }
> +
> +  if (eventTimings) {
> +    // Add a set of boxes representing timing information.
> +    for (let key of ["blocked", "dns", "connect", "send", "wait", "receive"]) {

lets define the array as a const at the top

::: devtools/client/netmonitor/components/request-list.js:17
(Diff revision 8)
> +const RequestListContent = createFactory(require("./request-list-content"));
> +
> +/**
> + * Renders the request list - header, empty text, the actual content with rows
> + */
> +const RequestList = function (props) {

instead of passing all props, in es6 we can pass `function({isEmpty})` to destruct interest param from props. This way is align with other existing components.

::: devtools/client/netmonitor/requests-menu-view.js:21
(Diff revision 8)
> -       getUriNameWithQuery,
> -       getUriHostPort,
> -       getUriHost,
>         loadCauseString} = require("./request-utils");
> -const Actions = require("./actions/index");
> +const {EVENTS} = require("./events");
> +const { createElement, createFactory } = require("devtools/client/shared/vendor/react");

nit: remove spaces inside of {} for consistency

::: devtools/client/netmonitor/requests-menu-view.js:23
(Diff revision 8)
> -       getUriHost,
>         loadCauseString} = require("./request-utils");
> -const Actions = require("./actions/index");
> +const {EVENTS} = require("./events");
> +const { createElement, createFactory } = require("devtools/client/shared/vendor/react");
> +const ReactDOM = require("devtools/client/shared/vendor/react-dom");
> +const { Provider } = require("devtools/client/shared/vendor/react-redux");

nit: remove spaces inside of {} for consistency

::: devtools/client/netmonitor/requests-menu-view.js:75
(Diff revision 8)
>     */
>    initialize: function (store) {
>      dumpn("Initializing the RequestsMenuView");
>  
> +    // this.allowFocusOnRightClick = true;
> +    // this.maintainSelectionVisible = true;

remove unused comments or add some description about why keep them

::: devtools/client/netmonitor/waterfall-background.js:31
(Diff revision 8)
> +];
> +
> +/**
> + * Creates the background displayed on each waterfall view in this container.
> + */
> +function WaterfallBackground(document) {

according to the source, `moving waterfall-background.js out of request-menu.js` seems can be a separate bug to reduce the size of this huge patch.
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review93328

> I think split request-menu related actions into separate files are not very meaningful though, for me they all actions around requests. And its hard to realize sort-by, timing-marker,selection actions are all just related to request-menu. It also takes extra time to find out a single action in sort-by, timing-marker.
> 
> Maybe its just the naming issue, or we can put request-menu related actions  into a single or two action files.

I have different opinion on it. Ideally, redux actions is supposed be be more generic and UI irrelevant. As a result, we are able to reuse it when there comes a new design that a new component requires sorting feature.

Let's keep UI relevant actions as simple as possible. Like I did in bug 1309194 which moved sidebar action into ui action and just remain open state for each UI component.

BTW, there is a naming nit: I prefer to keep the file name as short as possible. Let's change the file name from sort-by.js to sort.js in order to cover all sorting behaviors within one file.

And keep all the action creators like this:

return {
  type: SORT_BY,
  sortType,
};

> instead of passing all props, in es6 we can pass `function({isEmpty})` to destruct interest param from props. This way is align with other existing components.

Argee. Let's keep our coding style as the same with other existing code. Take an example for filter-buttons.js.

function FilterButtons({
  filterTypes,
  triggerFilterType,
}) {
 ...
}

rather than

const RequestList = function (props) {
  ...
};

I noticed that RequestList.displayName = "RequestList"; is a good for debugging if we aren't using JSX. We could add it for the rest of components in a separate bug.
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review93396

I noticed that we're using == rather than === when comparing things. I'd like to advocate to use strict equality (===) if you know for certain that you don't need type coercion. It's always be slightly faster and at least as fast as ==.

::: devtools/client/netmonitor/actions/enhancers.js:14
(Diff revision 8)
> +/**
> + * Process multiple actions at once as part of one dispatch, and produce only one
> + * state update at the end. This action is not processed by any reducer, but by a
> + * special store enhancer.
> + */
> +function batchActions(actions) {

nit:

return {
  type: BATCH_ACTIONS,
  actions,
};

::: devtools/client/netmonitor/actions/index.js:15
(Diff revision 8)
> +const selection = require("./selection");
> +const sortBy = require("./sort-by");
> +const timingMarkers = require("./timing-markers");
>  const ui = require("./ui");
>  
> -module.exports = Object.assign({}, filters, requests, ui);
> +Object.assign(exports,

nit: I still prefer to former pattern:

module.exports = Object.assign(
  a,
  b
);

since it would be easy to be transformed into an elegant object spread syntax. 

https://github.com/sebmarkbage/ecmascript-rest-spread


module.exports = {
  ...a,
  ...b,
};

::: devtools/client/netmonitor/reducers/requests.js:8
(Diff revision 8)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const I = require("devtools/client/shared/vendor/immutable");
> -const {
> +const { ADD_REQUEST,

nit: align with the existing code.

const {
  ADD_REQUEST,
  ...,
} = require("xxx");

::: devtools/client/netmonitor/reducers/timing-markers.js:7
(Diff revision 8)
> + * 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";
> +
> +const I = require("devtools/client/shared/vendor/immutable");
> +const { ADD_TIMING_MARKER,

nit: likewise. keep the same code style.

::: devtools/client/netmonitor/requests-menu-view.js:27
(Diff revision 8)
> +const ReactDOM = require("devtools/client/shared/vendor/react-dom");
> +const { Provider } = require("devtools/client/shared/vendor/react-redux");
> +const RequestList = createFactory(require("./components/request-list"));
>  const RequestListContextMenu = require("./request-list-context-menu");
> +const Actions = require("./actions/index");
> +const {getActiveFilters,

likewise. keep coding style the same.

::: devtools/client/netmonitor/selectors/index.js:11
(Diff revision 8)
>  
> -const { createSelector } = require("devtools/client/shared/vendor/reselect");
> -
> -/**
> - * Gets the total number of bytes representing the cumulated content size of
> - * a set of requests. Returns 0 for an empty set.
> +const filters = require("./filters");
> +const requests = require("./requests");
> +const ui = require("./ui");
> +
> +Object.assign(exports,

nit: I still prefer to former pattern:

module.exports = Object.assign(
  a,
  b
);

since it would be easy to be transformed into an elegant object spread syntax. 

https://github.com/sebmarkbage/ecmascript-rest-spread


module.exports = {
  ...a,
  ...b,
};

::: devtools/client/netmonitor/selectors/ui.js:13
(Diff revision 8)
> +
> +function isSidebarToggleButtonDisabled(state) {
> +  return getDisplayedRequests(state).isEmpty();
> +}
> +
> +const EPSILON = 0.001;

nit: move the constant to constants.js
(In reply to Jan Honza Odvarko [:Honza] from comment #46)
> Jarda, here is another try-push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=77e69d3280ade0ee17281e905fda4ad056803f9a&selectedJob=3
> 1214137

I think the browser_net_content-type.js test simply times out because it's running too slowly in debug mode. I'll try to make it run faster.
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review93684

::: devtools/client/netmonitor/store.js:17
(Diff revision 8)
> +/**
> + * A enhancer for the store to handle batched actions. For action of the BATCH_ACTIONS,
> + * the reducer is called successively on the array of batched actions, resulting in
> + * only one state update.
> + */
> +function batchingEnhancer(next) {

The enhancer looks similar to webconsole's `enableBatching`

https://github.com/mozilla/gecko-dev/blob/master/devtools/client/webconsole/new-console-output/store.js#L52

We should consider reuse it or at least align the code pattern
(In reply to Jarda Snajdr [:jsnajdr] from comment #47)
> I think the browser_net_content-type.js test simply times out because it's
> running too slowly in debug mode. I'll try to make it run faster.

The test was fixed in bug 1260403. The brotli part was moved to its own test and the content-type test now does half the work.
Latest patch version, interdiff 8-9:
- rebased on top of bug 1260403 and bug 1308507, make sure you have them
- solved many code style issues from Ricky's and Fred's review, e.g., renamed sort-by.js to sort.js
- removed code that was using Prefs.statistics value and tried to enable/disable the perf statistics feature. Now the feature is always enabled. Disabling it made sense only on Firefox OS anyway.
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review94164

::: devtools/client/netmonitor/prefs.js
(Diff revision 9)
>   */
>  
>  exports.Prefs = new PrefsHelper("devtools.netmonitor", {
>    networkDetailsWidth: ["Int", "panes-network-details-width"],
>    networkDetailsHeight: ["Int", "panes-network-details-height"],
> -  statistics: ["Bool", "statistics"],

Make sure to remove the pref from here as well:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/preferences/devtools.js#170
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review94374

Everything looks great to me. Thank you for the great work!

I added some nits and noticed there are still using == rather than ===.

I haven't opened all the issues for ==, so please make sure fix them in one sweep.

::: devtools/client/netmonitor/reducers/requests.js:15
(Diff revision 9)
> -} = require("../constants");
> +        CLEAR_REQUESTS,
> +        SELECT_REQUEST,
> +        PRESELECT_REQUEST,
> +        CLONE_SELECTED_REQUEST,
> +        REMOVE_SELECTED_CUSTOM_REQUEST,
> +        OPEN_SIDEBAR } = require("../constants");

nit: keep code style consistent.

const {
  ADD_REQUEST,
  UPDATE_REQUEST,
  CLEAR_REQUESTS,
  SELECT_REQUEST,
  PRESELECT_REQUEST,
  CLONE_SELECTED_REQUEST,
  REMOVE_SELECTED_CUSTOM_REQUEST,
  OPEN_SIDEBAR
} = require("../constants");

::: devtools/client/netmonitor/reducers/requests.js:56
(Diff revision 9)
>  
>  const Requests = I.Record({
> -  items: [],
> +  // The request list
> +  requests: I.List(),
> +  // Selection state
> +  selected: null,

nit: I can't figure out what is the exactly type of selected would be (a single request object or an index or an Id) at the first glance until I traced into reducer.

I prefer to use selectedId and preSelectedId so that we can understand that more easily.

::: devtools/client/netmonitor/reducers/requests.js:141
(Diff revision 9)
> +                lastEndedMillis = Math.max(lastEndedMillis, endedMillis);
> +                break;
> +              case "requestPostData":
> +                newData.set("requestHeadersFromUploadStream", {
> +                  headers: [],
> +                  headersSize: 0

nit: add a trailing comma for the last property.

{
  ...
  headersSize: 0,
}

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

::: devtools/client/netmonitor/reducers/requests.js:174
(Diff revision 9)
> +
> +      if (!selected) {
> +        return state;
> +      }
> +
> +      let clonedIdx = requests.findIndex(r => r.id == selected);

nit: === instead of ==

::: devtools/client/netmonitor/reducers/requests.js:175
(Diff revision 9)
> +      if (!selected) {
> +        return state;
> +      }
> +
> +      let clonedIdx = requests.findIndex(r => r.id == selected);
> +      if (clonedIdx == -1) {

nit: === instead of ==

::: devtools/client/netmonitor/reducers/requests.js:206
(Diff revision 9)
> +
> +      if (!selected) {
> +        return state;
> +      }
> +
> +      let removedRequest = requests.find(r => r.id == selected);

nit: === instead of ==

::: devtools/client/netmonitor/reducers/requests.js:214
(Diff revision 9)
> +      if (!removedRequest || !removedRequest.isCustom) {
> +        return state;
> +      }
> +
> +      return state.withMutations(st => {
> +        st.requests = requests.filter(r => r !== removedRequest);

nit: === instead of ==

::: devtools/client/netmonitor/requests-menu-view.js:318
(Diff revision 9)
> -          isXHR: isXHR,
> -          cause: cause,
> -          fromCache: fromCache,
> -          fromServiceWorker: fromServiceWorker
> -        }
> +    }
> -      });
> +    return getSortedRequests(state).findIndex(r => r.id == state.requests.selected);

nit: use === instead of ==.
Hi Fred,

  The test result as below:

# Test Case
STR
1. Open the browser
2. Open the network view of devtool console
3. Go to the devtools requestMenu sample server (hosted in LAN)

# Hardware
OS: Window 7
CPU: i7-3770 3.4GMhz
Memory: 16GB Ram
Hard Drive: 1TB SATA HDD
Graphics: GK107 [GeForce GT 640]/ GF108 [GeForce GT 440/630]

# Browsers
Firefox version: 53.0a1

# Result
Test Case                  | Try-Build(rev 15b26c9ca2ff) | Nightly (2016-11-16-03-02-12-mozilla-central)
devtool_network_view_test  | 40175.0 ms                  | 10700.0 ms
Flags: needinfo?(sho)
Flags: needinfo?(jsnajdr)
Sorry for press enter key too quickly...

Here are more context of comment 55, 

I refer shako with last Thursday’s window build in try server
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15b26c9ca2ff

And help him setup test loading server mentioned in comment 34.

The result loading time is 400% lower than the nightly browser. Shako mentioned some logs are shown in terminal when open the testing build. Do you left some debugging logs in requestMenu?
Shako, I haven't seen any log in Jarda's patch, so I'm curious what terminal logs you saw when running pref test?
Flags: needinfo?(sho)
Hi Ricky.

Please check the link below:

https://gist.github.com/ShakoHo/a5c789e9a585222f7697d3a59922de01
Flags: needinfo?(sho)
Ah, I think test result was incorrect since we ran it with debug build. 

According to the result of https://treeherder.mozilla.org/#/jobs?repo=try&revision=15b26c9ca2ff on comment 56, I noticed the try syntax is "try: -b d ..." which points out that we ran on debug build. So, that's why we saw the logs output from terminal.

Shako, would you do me a favor to pref testing again with optimization mode "try: -b o ..."? thanks!
Flags: needinfo?(jsnajdr) → needinfo?(sho)
I've re-triggered try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d11fc1f9724

Please wait for the build artifact and verify again :)
New patch (interdiff 9-10) with a lot of big changes:

1. Big refactoring of the code that batches the addRequest and updateRequest actions:

The batching queue is now in its own middleware. Dispatching an action with field meta: { batch: true } will put the action into a queue instead of dispatching it immediately. The dispatch() method returns a promise that resolves after the action is really dispatched and the store is updated. The promise is used to replace callbacks at several places.

I removed the batching enhancer, and converted it into a reducer. I realized we don't need an enhancer here, because all the enhancer does is that it decorates the reducer. Now, the batching actions and reducer are almost identical to this module:

https://github.com/tshelburne/redux-batched-actions

More updates are now batched, for example, the one that constructs the responseContentDataUri field for image thumbnails. Should make the rendering visibly faster.

2. RequestListItem now has a much smarter shouldComponentUpdate method that avoids approx 40% of component rerenders.

3. The RequestsListItem rendering functions need to extract some details about the URL. Up to now, they called extractUrlInfo over and over on every render, although the URL field never changes for a given request. Moved the "urlDetails" computation to the addRequest and updateRequest reducers - compute it once and then save it in the Request record.

4. When running in devel mode, React complained about missing "key" properties at certain places. I added them to the array of timing labels in RequestListHeader, and to the array of timing boxes in a WaterfallColumn.

5. Cleaned up a lot of obsolete unused code in requests-menu-view.js

6. Removed the devtools.netmonitor.statistics pref (see comment 53)
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review94544
Interdiff 10-11:
- fixed review issues reported by Ricky - renamed "selected" to "selectedId", fixed == vs === etc.
- improved the code for updating a request item - more efficient now

I also filed bug 1319062 to move the batching actions/reducers/middleware to a shared component.
Hi Ricky,

The updated result as below:
# Result
Test Case                  | Try-Build(rev 15b26c9ca2ff) | Nightly (2016-11-16-03-02-12-mozilla-central)
devtool_network_view_test  | 14098 ms                    | 10700 ms
Flags: needinfo?(sho)
I understand this big patch is also ready to land soon, so it makes sense to not to introduce so many changes in bug 1319010.

Jarda, bug 1319010 focus on moving sidebar-view and details-view out of netmonitor-view, please rebase it once it is landed. thanks!
Current status:

The try run is failing on unhandled promise errors from the batching middleware - I'm investigating that. Doesn't happen at all on my local machine.

I did some profiling of the performance regressions, here are my observations:

On load, a lot of time is spent loading the netmonitor-controller.js (45ms) and netmonitor-view.js (100ms) modules. Splitting the files into many small modules apparently costs something. In netmonitor-view.js, most time is spent loading react.js. That could be remedied by reusing already loaded React from the parent toolbox, instead of loading a new instance.

On reload, most of the time is spent inside React updating the view. The profile doesn't show any big dominant task that could be optimized. It's a big hairball of React updates, layouts, style calculations and garbage collections. What could be improved here?

- upgrade to React 15
- use react-virtualized to avoid rendering all items in the list
- I noticed that the code that handles scrolling (check if scrolled to bottom, scroll after row added) triggers a lot of layouts and style recalculations. Maybe we can improve on that
- I'm surprised to see shouldComponentUpdate calls taking ~5ms and being very visible in the JS flame chart. That code calls several get()'s into a Immutable.Record and compares properties.
Recent changes:

Main patch, interdiff 11-12:
- rebased on top of other patches
- got rid of window.isRTL - except the waterfall drawing, it's not needed anymore - I made the code RTL agnostic.
- handled potential exceptions when methods like getResponseContent return an error. Was happening in tests when the toolbox is closing and RDP packets are still coming in.

interdiff 12-13:
- improved the clear button component - don't call NetMonitorView directly, but dispatch an action.
Added two more patches that optimize the loading process - reuse already loaded React libraries from the toolbox, and don't load a duplicate instance to the netmonitor.xul iframe.

First, I added a new option to BrowserLoader - commonLibRequire. If a module from the common directory (devtools/client/shared/vendor) is loaded, the load is delegated to the toolbox.browserRequire loader. This way, the common modules are shared and what's nice, it's completely transparent to the developer. No need to pass "React" instances around - just call require() and the loader will do the right thing.

Then I refactored the Netmonitor code to use this new loader option. It has a catch - the netmonitor-controller.js and netmonitor-view.js cannot be loaded as global script in the netmonitor.xul document, because the "toolbox" object is not available at that time. We must wait until the panel's init() method is called with the "toolbox" as a parameter.

So I converted the controller and the view to normal CommonJS modules, and implemented the loading in a new module, netmonitor.js. It's now very similar to what Inspector does.

The bulk of the patch is just shuffling global variables around. Variables defined in netmonitor-controller.js and netmonitor-view.js are not automatically global any more. So I need to assign them explicitly to the "window" object. Note that many global variables (EVENTS, Prefs, ...) are needed only by the tests and can be gradually removed, too.
Comment on attachment 8813713 [details]
Bug 1309866 - Add option for loading common libraries to BrowserLoader

James, Julian, what do you think about adding the new option to the BrowserLoader? I see there are other attempts to solve the problem with sharing one React instance, and they mostly do some special "bootstrap" call where React modules are treated in a special different way.

Patching the BrowserLoader is completely transparent to the developer - it's the loader's job to load the right thing.
Attachment #8813713 - Flags: feedback?(jlong)
Attachment #8813713 - Flags: feedback?(jdescottes)
(In reply to Jarda Snajdr [:jsnajdr] from comment #81)
> Comment on attachment 8813713 [details]
> Bug 1309866 - Add option for loading common libraries to BrowserLoader
> 
> James, Julian, what do you think about adding the new option to the
> BrowserLoader? I see there are other attempts to solve the problem with
> sharing one React instance, and they mostly do some special "bootstrap" call
> where React modules are treated in a special different way.
> 
> Patching the BrowserLoader is completely transparent to the developer - it's
> the loader's job to load the right thing.

Just a note since I've discussed this a bit before (with James, I think): we currently reuse the toolbox window's React instance inside the inspector, and it seems to work fine.  I have heard that we shouldn't be doing that (something maybe with event listeners where react might be using the wrong global), but I haven't heard of any inspector bugs as-is.  It's certainly faster to reuse it since executing the script a second time is non-trivial - and I'm not sure if it gets better or worse with 15.
(In reply to Brian Grinstead [:bgrins] from comment #82)
> Just a note since I've discussed this a bit before (with James, I think): we
> currently reuse the toolbox window's React instance inside the inspector,
> and it seems to work fine.

I think there is complete agreement that we should have only one React instance. There are different methods how to achieve that, however. I tried to bake this support directly into BrowserLoader and it seems to work beautifully. I'm seeking feedback on this solution.

> I have heard that we shouldn't be doing that
> (something maybe with event listeners where react might be using the wrong
> global), but I haven't heard of any inspector bugs as-is.

I noticed that React attaches a scroll listener to the window and keeps track of the window scroll position. It uses this to compute 'pageX' and 'pageY' MouseEvent properties in case the browser doesn't provide them natively. Which is not the case of Firefox.

Also, there is some code that works with selection that uses window. But again, it is used as a fallback if more modern APIs are not available.

So, it seems safe to use React loaded in parent iframe inside a child window. But I'm not 100% sure, of course.

> It's certainly faster to reuse it since executing the script a second time is non-trivial -
> and I'm not sure if it gets better or worse with 15.

Yes, it makes a big difference in DAMP tests in talos and the react.js load is very visible in the Performance tool profile. In React 15, the react.js file is 10% bigger (780 kB vs 710 kB), so the load is likely to be even slower.
Depends on: 1312236
Brian, I've pushed the current patches (including patches from bug 1312236 - React 15) to Talos and there are some performance regressions (see link in comment #86).

It's mostly related to Network panel close/open/reload

Some of the regressions are hard to fix. For instance Netmonitor close action seems to be slower because React's clean-up (specifically unmounting components) while pretty much nothing was happening there before.

We are also planning to do perf improvements in the future (e.g. using react-virtualized for the list of requests). But, these things should be done as follow-ups since this patch is already big.

Is it reasonable to say that the Talos regressions are ok now and we'll be improving that in the future?

Honza
Flags: needinfo?(bgrinstead)
Comment on attachment 8813713 [details]
Bug 1309866 - Add option for loading common libraries to BrowserLoader

https://reviewboard.mozilla.org/r/95106/#review95524

The patch looks good to me but, I'd like to get some feedback from James before r+

Honza
Comment on attachment 8813714 [details]
Bug 1309866 - Use toolbox loader to load common libs into Netmonitor

https://reviewboard.mozilla.org/r/95108/#review95532

I really like this patch!

Just one inline comment.

Honza

::: devtools/client/netmonitor/netmonitor-controller.js:27
(Diff revision 1)
> +const { Prefs } = require("./prefs");
>  
> -XPCOMUtils.defineConstant(this, "EVENTS", EVENTS);
> -XPCOMUtils.defineConstant(this, "ACTIVITY_TYPE", ACTIVITY_TYPE);
> -XPCOMUtils.defineConstant(this, "Editor", Editor);
> -
> +XPCOMUtils.defineConstant(window, "EVENTS", EVENTS);
> +XPCOMUtils.defineConstant(window, "ACTIVITY_TYPE", ACTIVITY_TYPE);
> +XPCOMUtils.defineConstant(window, "Editor", Editor);
> +XPCOMUtils.defineConstant(window, "Prefs", Prefs);

Why 'Prefs' is defined? Note that there is also 'Prefs' variable defined above. I am not seeing it used in this module.
Attachment #8813714 - Flags: review?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #91)
> Why 'Prefs' is defined? Note that there is also 'Prefs' variable defined
> above. I am not seeing it used in this module.

This global (and several others) is defined only to keep the tests happy, without introducing another big diff that would change how the tests access the Prefs module.

These globals are temporary and are going away in one of follow-up commits.
Latest version of the patch cancels the queued addRequest and updateRequest actions when the Netmonitor is being closed. This is an attempt to solve the talos regression for the complicated.netmonitor.close test. But it doesn't look successful - the results still show a slowdown:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=cd600761d2467beb264caae13f44d04e7b1a6bf4&newProject=try&newRevision=8ac0935b6e52576bb6aa66d55f0fa97cc0e82690&framework=1&filter=damp&showOnlyImportant=0

So I'm afraid that the regression is caused by the fact that before the patch, the close action did virtually nothing, but now it calls unmountComponentAtNode for all mounted React components. Hard to improve this.
Comment on attachment 8813713 [details]
Bug 1309866 - Add option for loading common libraries to BrowserLoader

Re-requesting feedback - the flag was reset by MozReview.
Attachment #8813713 - Flags: feedback?(jlong)
Attachment #8813713 - Flags: feedback?(jdescottes)
Comment on attachment 8813714 [details]
Bug 1309866 - Use toolbox loader to load common libs into Netmonitor

https://reviewboard.mozilla.org/r/95108/#review95552
Attachment #8813714 - Flags: review?(odvarko) → review+
Comment on attachment 8813713 [details]
Bug 1309866 - Add option for loading common libraries to BrowserLoader

Thanks Jarda. I agree with the overall approach here, makes sense to me as long as reusing the same React instance across windows is not an issue. 

The changeset looks good AFAICT, we could even go a step further and add a helper on the toolbox that would create a BrowserLoader for a provided window, and would automatically add the commonLibRequire parameter.
Attachment #8813713 - Flags: feedback?(jdescottes) → feedback+
(In reply to Jan Honza Odvarko [:Honza] from comment #88)
> Brian, I've pushed the current patches (including patches from bug 1312236 -
> React 15) to Talos and there are some performance regressions (see link in
> comment #86).
> 
> It's mostly related to Network panel close/open/reload
> 
> Some of the regressions are hard to fix. For instance Netmonitor close
> action seems to be slower because React's clean-up (specifically unmounting
> components) while pretty much nothing was happening there before.

Do we really have to unmount all the components if the window is being unloaded?  Is this because we are using a parent window's react instance?

> We are also planning to do perf improvements in the future (e.g. using
> react-virtualized for the list of requests). But, these things should be
> done as follow-ups since this patch is already big.
> 
> Is it reasonable to say that the Talos regressions are ok now and we'll be
> improving that in the future?

Not too worried about the close time - it's a less important measurement and it's still in line with other tools (although of course if you have ideas to improve you should handle those in follow ups).

The most concerning numbers are the ~30% regression I'm seeing on 'complicated.netmonitor.reload.DAMP opt e10s'.  Just because reloading the page is a thing you'd be very likely to do while having the netmonitor opened.  And note that the 'reload' measurement is how long it takes the page to report the 'load' event [0] - so this means the page load is actually slower with the netmonitor opened and presumably the measurements that you care about are being affected (slowed down) by the tool being opened.  This also indicates to me that things like the perf issue in Bug 1176050 might become worse.

If you want to test with the exact same page that the complicated.netmonitor measurement is using (which has about 280 requests), you can download the tp5 zip [1] and open bild.de/www.bild.de/index.html.

Ultimately I'll leave it up to you and Jarda to decide how to proceed here since it's primarily affecting the netmonitor (the total damp regression is low enough to fly under the radar and it doesn't seem to be hitting any other tools).  A few things to think about:
* Are there going to be more things being converted in the short term that might compound any regression?
* What specifically is slowing this down when reloading the page?  Is it just slower to draw each request row?  Do we have to re-render the whole row on every event update?

Unfortunately we don't have Windows results on that push for comparison - I triggered g2 for win7 on https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ac0935b6e52576bb6aa66d55f0fa97cc0e82690, let's see if that also creates a windows build.  But might be best to just do a new comparison push / base to make sure we are accounting for windows.

[0]: https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/tests/devtools/addon/content/damp.js?offset=800#45
[1]: https://wiki.mozilla.org/Buildbot/Talos/Tests#tp5n_pages_set
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #100)
> Do we really have to unmount all the components if the window is being
> unloaded?  Is this because we are using a parent window's react instance?

Yes, we need to unmount all the components, because otherwise, the shared React instance would hold references to the mounted elements forever, which means leaking windows and many other objects.

> * Are there going to be more things being converted in the short term that
> might compound any regression?

The next thing to convert is the sidebar with the details tabs. We'll need to be careful not to add further regressions there.

> * What specifically is slowing this down when reloading the page?  Is it
> just slower to draw each request row?  Do we have to re-render the whole row
> on every event update?

The XUL Netmonitor maintains its state (model data) inside a mutable array that is mutated in-place without reallocation. Immutable.js adds some unavoidable overhead. When new data arrive from the server, the DOM updates are hand-coded and efficient in the XUL version. React, even when carefully optimized, has some overhead when re-rendering the DOM tree and doing reconciliation.

React, Redux and immutability involves a tradeoff - you get a much better architecture and cleaner code, and you pay some penalty in performance.

Yet, there is still a lot of room for improvement and we are committed to getting the performance back to the original level, and maybe even better, but it becomes very impractical to do this as part of this bug. The patch is already huge and it blocks other work by not being landed yet. We'd rather accept a temporary regression and the fix it in several followups.

> Unfortunately we don't have Windows results on that push for comparison

The results are almost identical on different platforms. We ran Windows in many previous talos runs, but running them just on Linux proved to be representative enough.
Comment on attachment 8813713 [details]
Bug 1309866 - Add option for loading common libraries to BrowserLoader

https://reviewboard.mozilla.org/r/95106/#review96214

::: devtools/client/shared/browser-loader.js:22
(Diff revision 2)
>    "resource://devtools/client/shared/vendor",
>    "resource://devtools/client/shared/redux",
>  ];
>  
> +const COMMON_LIBRARY_DIRS = [
> +  "resource://devtools/client/shared/vendor",

Why is this list different than `BROWSER_BASED_DIRS`? Can you explain a little what this is trying to do, as it's not obvious to me.
(In reply to James Long (:jlongster) from comment #102)
> Why is this list different than `BROWSER_BASED_DIRS`? Can you explain a
> little what this is trying to do, as it's not obvious to me.

The BrowserLoader checks if the required module's path is inside BROWSER_BASED_DIRS. If it is, it means that the module needs access to the window object. Therefore, its instance is loaded into the BrowserLoader, with the right global. A different BrowserLoader, in a different window, will need its own instance - they can't be shared.

If the module's path is not inside BROWSER_BASED_DIRS, it means it doesn't need access to window. In that case, the require() call is delegated to the global devtools loader. Modules loaded into the global loader are shared by all tools.

Now, we need a third option, something not provided by the two options above. A module that is shared by all tools, as one singleton instance, but which, at the same time, has a window object as its global. That's where COMMON_LIBRARY_DIRS comes handy. If the module is inside COMMON_LIBRARY_DIRS, and the BrowserLoader was created with the commonLibRequire option, the BrowserLoader from the parent window (i.e., the toolbox's "about-devtools" window) is used to load the module.

This way, we can have React (and anything else in the vendor dir) shared by several tools.

Redux doesn't need to be loaded by the BrowserLoader at all, because it doesn't need window or document anywhere.
Iteration: 53.1 - Nov 28 → 53.2 - Dec 12
Comment on attachment 8807207 [details]
Bug 1309866 - Migrate RequestsMenuView to a React component with Redux store

https://reviewboard.mozilla.org/r/90444/#review96364

R+ assuming try is green.

Page reload performance (as discussed) should be continuously improved in follow ups e.g. using react-virtualized, keep re-rendering to minimum, optimize using shouldComponentUpdate, etc.

Honza
Attachment #8807207 - Flags: review?(odvarko) → review+
(In reply to Jarda Snajdr [:jsnajdr] from comment #103)
> 
> This way, we can have React (and anything else in the vendor dir) shared by
> several tools.

This seems like it could be dangerous, are you totally sure that React supports this? It seems like there could be obscure bugs, particularly with the event system, to not host React within the same window. The long term goal should be to not run individual tools in iframes, but I know that's not a practical solution right now.

It's very confusing that there is not two lists of directories that basically intend the same thing. Can you not just reuse the list of BROWSER_BASED_DIRS? Even though redux doesn't need to be loaded locally, I'd rather load things into the window by default and make special exceptions for not doing that. The reason is that it's very surprising when you do try to use something that you'd assume is available in the window context, but it's not. The `vendor` library includes libs that definitely need to be loaded in the window, and some of the redux middleware probably assumes the window context as well.

Why is the net monitor using a different pattern than we've already established with the console, debugger, and memory tools? You should just be creating your own BrowserLoader to be used locally within the tool.
(In reply to James Long (:jlongster) from comment #105)
> This seems like it could be dangerous, are you totally sure that React
> supports this? It seems like there could be obscure bugs, particularly with
> the event system, to not host React within the same window.

Inspector is already using this pattern for some time. They're using the React and ReactDOM instances from the toolbox window, but they need to be always very explicit about which loader is used to load what.

This BrowserLoader extension just makes this approach more transparent and usable.

I did some research into what exactly React uses the window object for and I found out that reusing an instance from another window should be fine - see comment 83.

> It's very confusing that there is not two lists of directories that
> basically intend the same thing. Can you not just reuse the list of
> BROWSER_BASED_DIRS?

I could, if we get rid of the two tool-specific directories that shouldn't really be there:

resource://devtools/client/inspector/layout
resource://devtools/client/jsonview

The fact that they need to be hardcoded into the BrowserLoader indicates that there is something wrong with the BrowserLoader API.

> Even though redux doesn't need to be loaded locally, I'd
> rather load things into the window by default and make special exceptions
> for not doing that.

Note that this has performance and memory implications. If you load modules into the window, it means that every window (i.e., every tool) needs to have its own instance of every module and they can't share them.

> The reason is that it's very surprising when you do try
> to use something that you'd assume is available in the window context, but
> it's not. The `vendor` library includes libs that definitely need to be
> loaded in the window, and some of the redux middleware probably assumes the
> window context as well.

No module that's currently in the devtools/client/shared/redux directory needs access to window. Note that you can still register your own window-using middleware and reducers, when creating the store. Nothing prevents you from doing that.

Loading a module into a window just to be on the safe side slows down the load time for the tools.

> Why is the net monitor using a different pattern than we've already
> established with the console, debugger, and memory tools? You should just be
> creating your own BrowserLoader to be used locally within the tool.

Because we've run into performance regressions in talos - opening Netmonitor is significantly slower than it was before, and a large part of that slowdown was loading React, which is a big 700kB JS file.

I don't know why console, debugger and memory tools are not worried about that. Once they will try to improve the startup time, I guess they will be definitely interested in this BrowserLoader improvement.

The new console frontend is not being tested in DAMP at this time - it's disabled and the old one is measured instead - bug 1306780. Brian, do you have any data on how it performs?

Inspector is already using the shared React instance, although their approach is rather hacky. It'll be very easy to clean up if the BrowserLoader changes are landed.
Flags: needinfo?(bgrinstead)
You make good points and I won't respond to each of them individually because I mostly agree.

My ideal world is that we don't use iframes at all. Then all of this just goes away. It would be nice to talk more seriously about that.

Until then, you may be right that the perf wins are good enough to warrant this complexity.

The thing that I don't like is the inconsistency. We now have to think about 3 types of modules: does it work without any window context, or does it need the toolbox's window, or does it need the current window? Many libs will fail when trying to use across windows, but maybe not as much as I think.

I'd prefer to choose one solution for loading libraries. If we want to do it this way, shouldn't we try it for all libraries? immutable.js, etc. But I'm not sure it would work for all of them.

> No module that's currently in the devtools/client/shared/redux directory needs access to window. Note that you can still register your own window-using middleware and reducers, when creating the store. Nothing prevents you from doing that.

Note that the the `console` referenced in the logger middleware won't be what you expect anymore, it'll just log to stdout as far as I know. That would be very annoying, I use the real console a lot for inspecting actions.


> I could, if we get rid of the two tool-specific directories that shouldn't really be there:
> 
> resource://devtools/client/inspector/layout
> resource://devtools/client/jsonview
> 
> The fact that they need to be hardcoded into the BrowserLoader indicates that there is something wrong with the BrowserLoader API.

That's not a deficiency of the API, the current API is more straight-forward then what you're proposing, but it's a deficiency with how we've structured our files. Those files should be within one of the folders that is meant to be loaded in the window, not in a tool-specific directory.

Do you think it's possible to use a common window context for all libs so we only have 2 "modes" to think about? The `BrowserLoader` is becoming incredibly complex so it'd be nice to see if we can make is simpler somehow. If some of the libraries really need to be loaded in the same window context as they are used, I suppose we'll have to go with your patch.
(In reply to James Long (:jlongster) from comment #107)
> The thing that I don't like is the inconsistency. We now have to think about
> 3 types of modules: does it work without any window context, or does it need
> the toolbox's window, or does it need the current window? Many libs will
> fail when trying to use across windows, but maybe not as much as I think.

It's entirely possible that the only library that really needs the toolbox window is React and ReactDOM. Because it's big and used by almost all tools, it's worthwhile to share one instance between them. And it needs the window object.

Other libraries either are not big enough, so loading them multiple times into each iframe is not an issue, or don't need the window object. After all, JS libraries like Immutable.js or Redux are designed to work server-side in node.js. The only issue then is having the right "console" object.

> Note that the the `console` referenced in the logger middleware won't be
> what you expect anymore, it'll just log to stdout as far as I know. That
> would be very annoying, I use the real console a lot for inspecting actions.

Oh, that's an interesting point. So, the global devtools loader (Loader.jsm) doesn't have a window, and its console global is the Console.jsm module. That's the wrong one you don't want to use, because it logs only to stdout? It doesn't log to the Browser Toolbox console? And it doesn't have methods like console.table? (looking at bug 1224751...)

On the other hand, the BrowserLoader's console is the window's console, which is the standards compliant one defined in Console.webidl. By "real console", you mean the console in Browser Toolbox?

> That's not a deficiency of the API, the current API is more straight-forward
> then what you're proposing, but it's a deficiency with how we've structured
> our files. Those files should be within one of the folders that is meant to
> be loaded in the window, not in a tool-specific directory.

On the other hand, the tool should have some flexibility in how it structures its directories. And it should be able to tell the BrowserLoader what the structure is. The fact that jsonview and inspector directories got hardcoded suggests that the API to tell the BrowserLoader about the dir structure is not sufficient.

> Do you think it's possible to use a common window context for all libs so we
> only have 2 "modes" to think about?

I can change the patch so that the toolbox loader is used only for React, not for the whole "vendor" directory, if that helps anything.

Other than that, I'm afraid we'll have to live with 3 modes at least for a while. React needs to be loaded in the toolbox window. Most tool-specific modules need to be loaded in the tool's iframe's window. They commonly attach mouse listeners to the window, look at the window dimensions etc. That would likely break a lot of things, if the window was suddenly a different object. Not the tool's iframe (e.g., netmonitor.xul), but the parent "about:devtools-toolbox" frame.

Maybe we could get rid of the mode that doesn't have any window - loading a module either in the toolbox BrowserLoader (shared), or in the tools' BrowserLoader (private to the tool). I think that's a good option for the frontend - it's very close to the enviroment that's in a non-privileged content tab. That's a task for a separate bug, however.

> The `BrowserLoader` is becoming incredibly complex so it'd be nice to see if we can make is simpler somehow.

After staring at for few hours, and swearing a lot, I realized that it's actually not that complex. It's poorly documented, has an inelegant API, and no unit tests. A few cleanup patches should make things a lot better.

> If some of the libraries really need to be loaded in the same window context
> as they are used, I suppose we'll have to go with your patch.

Maybe not the shared 3rd party libraries, but many tool-specific modules definitely depend on the window object being the tool's iframe, not about:devtools-toolbox.

My aim was to verify that from these two approaches:

1. BrowserLoader is the place that knows how to load a particular module, and the tool code just calls "require" for everything.
2. The tool code has several versions of "require" available, and it's the tool author's responsibility to load modules with the right one - used in inspector at places like [1]

the approach #1 is the better one. And that it's compatible with the devtools.html world, where tools live in an unprivileged content tab, where the module loading is handled by webpack, and where ES6 "import" statement is used.

Is that true? If yes, we can clean up the BrowserLoader and make it easy to understand.

[1] http://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#417-440
> On the other hand, the BrowserLoader's console is the window's console, which is the standards compliant
> one defined in Console.webidl. By "real console", you mean the console in Browser Toolbox?

Yes, and there are other weird quirks with the custom jsm console and I can't remember all of them now. But the window's console is a lot more predictable.

> Maybe we could get rid of the mode that doesn't have any window

I'm afraid we can't really do that either, because we'd load in all of the system deps for each tool instead of sharing them with the devtools loader and you'd have an even worse memory problem.

> On the other hand, the tool should have some flexibility in how it structures its directories.

Flexibility is not always good. In fact, poor APIs often happen because they choose to be too flexible and provide tons of configuration options. Originally the BrowserLoader was very simple but since then it's been refactored a lot and has gotten quite complicated, but for necessary reasons.

> the approach #1 is the better one. And that it's compatible with the devtools.html world, where tools live
> in an unprivileged content tab, where the module loading is handled by webpack, and where ES6 "import"
> statement is used.

Yes, I agree with that. It should be the single require that all frontend uses and just work. We just need to make sure we also make it clear how to add 3rd party libraries and actually manage the libs that its loading.

> I can change the patch so that the toolbox loader is used only for React, not for the whole "vendor" directory, if that helps anything.

I think that could make the code more explicit and there could be a clear comment as to why its specialized it. But I'll leave it up to you. I just wanted to talk this through, but I'm not going to block it. Feel free to land your patch, or tweak it to only load React as a common lib (I would prefer that).
Flags: needinfo?(bgrinstead)
Flags: needinfo?(bgrinstead)
Comment on attachment 8813713 [details]
Bug 1309866 - Add option for loading common libraries to BrowserLoader

https://reviewboard.mozilla.org/r/95106/#review96772
Attachment #8813713 - Flags: review+
Attachment #8813713 - Flags: feedback?(jlong) → feedback+
Comment on attachment 8813713 [details]
Bug 1309866 - Add option for loading common libraries to BrowserLoader

https://reviewboard.mozilla.org/r/95106/#review96774
Attachment #8813713 - Flags: review?(odvarko) → review+
(In reply to James Long (:jlongster) from comment #109)
> > Maybe we could get rid of the mode that doesn't have any window
> 
> I'm afraid we can't really do that either, because we'd load in all of the
> system deps for each tool instead of sharing them with the devtools loader
> and you'd have an even worse memory problem.

There would be the shared BrowserLoader in the toolbox window, shared by all the tools.

> > I can change the patch so that the toolbox loader is used only for React,
> > not for the whole "vendor" directory, if that helps anything.
> 
> I think that could make the code more explicit and there could be a clear
> comment as to why its specialized it. But I'll leave it up to you. I just
> wanted to talk this through, but I'm not going to block it. Feel free to
> land your patch, or tweak it to only load React as a common lib (I would
> prefer that).

OK, I'll do that in a followup. I'll be busy with other things for the next few days, and landing this bug now will unblock a lot of other work that's waiting for it.

I'm also thinking about providing symbolic names to the React libraries, so that they can be loaded with require("react") or require("react-dom"). Let's discuss that elsewhere.

Thanks for looking at this and for your comments!
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/75d37553b7cd
Migrate RequestsMenuView to a React component with Redux store r=Honza
https://hg.mozilla.org/integration/autoland/rev/e0e09f7f22a3
Add option for loading common libraries to BrowserLoader r=Honza,jlongster
https://hg.mozilla.org/integration/autoland/rev/30baf39ed89f
Use toolbox loader to load common libs into Netmonitor r=Honza
(In reply to Jarda Snajdr [:jsnajdr] from comment #106)
> Because we've run into performance regressions in talos - opening Netmonitor
> is significantly slower than it was before, and a large part of that
> slowdown was loading React, which is a big 700kB JS file.
> 
> I don't know why console, debugger and memory tools are not worried about
> that. Once they will try to improve the startup time, I guess they will be
> definitely interested in this BrowserLoader improvement.
> 
> The new console frontend is not being tested in DAMP at this time - it's
> disabled and the old one is measured instead - bug 1306780. Brian, do you
> have any data on how it performs?
> 
> Inspector is already using the shared React instance, although their
> approach is rather hacky. It'll be very easy to clean up if the
> BrowserLoader changes are landed.

We actually tried to switch the inspector to use its own browser loader in Bug 1310416 but ended up backing it out as perf regressions.

For the console, it does use its own instance right now.  It provides a trade off, that I've seen in DAMP. When using our own instance, the startup time is significantly slower, but the close time is significantly faster - since in the current frontend we do a bunch of DOM cleanup at destroy but with the new one just let the window unload and don't need to clean up any of the react components.  If we switch to a shared React, I'm assuming we'll need to be more careful and unmount components on the frame's destroy.  On balance though, I'd rather have a fast startup than destroy.

The console has a unique difference also, which is that it sometimes loads without a toolbox (in the Browser Console), so relying on the toolbox's React when the toolbox exists could work, but it'll just need to handle the non-toolbox case as well.  It's definitely going to be worth doing some talos pushes to compare using the toolbox instance vs not.
Flags: needinfo?(bgrinstead)
https://hg.mozilla.org/mozilla-central/rev/75d37553b7cd
https://hg.mozilla.org/mozilla-central/rev/e0e09f7f22a3
https://hg.mozilla.org/mozilla-central/rev/30baf39ed89f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1321749
Depends on: 1303439
Depends on: 1321434
I backed these out in https://hg.mozilla.org/integration/mozilla-inbound/rev/32dba5242f0f0d1fb75ec533ad507456ff6d3633 for all of the test failures that have popped up since.
Status: RESOLVED → REOPENED
Flags: needinfo?(jsnajdr)
Resolution: FIXED → ---
Target Milestone: Firefox 53 → ---
Depends on: 1321436
All the failures are only on linux32/debug, and there are three tests failing:

1. browser_net_frame.js
2. browser_net_filter-01.js

Caused by test running for too long and timing out. Many requests are executed, and the UI is in a "lazyUpdate = false" mode, which makes it much slower. Solved by disabling these tests on linux32/debug

3. browser_net_filter-01.js

Window leak, the same one as in bug 1317867. Easily fixed.

Submitted a try run, will land again after approved by Honza.
Flags: needinfo?(jsnajdr)
I'm also abusing the reopening of this bug to add a few little bugfixes that I have ready in my repo:

- trigger resize of the waterfall column when sidebar is opened or closed. That changes the width of the list.
- splitter element triggers resize on mouseup event, not mousemove
- fix the widths of list columns when the list width is <700px. In that case, there is a @media selector to hide the waterfall column. The visible columns' widths should add up to 100% (they use the vw units). They didn't and I fixed that.
- fixed a bug when resizing the waterfall column to a smaller size. In that case, the "tick" markers can make the header element inflexible, and it's not resized properly.

The interdiff (14-15) also shows some bogus changes in devtools/client/preferences/devtools.js that were caused by a rebase. Ignore them.
Pushed by jsnajdr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/abcb8d9139a2
Migrate RequestsMenuView to a React component with Redux store r=Honza
https://hg.mozilla.org/integration/autoland/rev/ec1ebd2666c7
Add option for loading common libraries to BrowserLoader r=Honza,jlongster
https://hg.mozilla.org/integration/autoland/rev/f09e9013044d
Use toolbox loader to load common libs into Netmonitor r=Honza
https://hg.mozilla.org/mozilla-central/rev/abcb8d9139a2
https://hg.mozilla.org/mozilla-central/rev/ec1ebd2666c7
https://hg.mozilla.org/mozilla-central/rev/f09e9013044d
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
sorry had to back this out because after merging to m-c we had mac os static debug build bustage like described in bug 1322685

I'm not sure if this patches here need to change or a releng fix is needed but to have a working tree i had to back this out to fix the static bustage
Flags: needinfo?(jsnajdr)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/f74a13482aa7
Backed out changeset f09e9013044d for causing Bug 1322685 after merge
Backout by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/6a75cf810d96
Backed out changeset ec1ebd2666c7 
https://hg.mozilla.org/mozilla-central/rev/8404d26166a3
Backed out changeset abcb8d9139a2 to have complete backout
Depends on: 1322685
Patches are OK, a releng fix is needed (see bug 1322685). Some JS files were moved around and it seems that in some cases, there are two copies at both the old and new location.
Flags: needinfo?(jsnajdr)
Due to backout - should this be re-opened ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73f371377987
Migrate RequestsMenuView to a React component with Redux store r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/afe9d8d704fa
Add option for loading common libraries to BrowserLoader r=Honza,jlongster
https://hg.mozilla.org/integration/mozilla-inbound/rev/5581c5f4da7a
Use toolbox loader to load common libs into Netmonitor r=Honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/28445d6f646b
touch CLOBBER
Depends on: 1323059
Duplicate of this bug: 1303439
Duplicate of this bug: 1321434
Duplicate of this bug: 1321436
I've managed to perform tests around this issue on Windows 10 x64, Ubuntu 16.04 LTS x64 and Mac OS X 10.11.6. 

Marking this accordingly since RequestsMenuView is working as expected on latest Nightly 53.0a1 (2016-12-18) and based on the fact that, some performance regressions caused by this bug, will be fixed in another patches. (bug 1321749)
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1336203
Depends on: 1325730
Duplicate of this bug: 1308499
Duplicate of this bug: 1324189
Depends on: 1353289
Depends on: 1371494
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.