As Bug #1406312 we found Waterfall's timingBoxes is slow because of its l10n calls. We could make tooltip computation lazily by computed only when user onhover in the column. http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/request-list-column-status.js#48 That will make big performance difference when we have lots of requests in netmonitor.

Hi Fred, Can I work on this one?

I would love to work on this as I'm new to contributing to Mozilla and this seems like a good way to start! Please let me know if I can work on this :)

Hi Abhinav, thanks for provide the patch! Please take a look on test result https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef827040d20c&selectedJob=136385522 Some tests related to tooltip are broken because we now show tooltip on mouse hover. You can reference how we test image tooltip and use similar way to mimic mouse hover event to test tooltips http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_image-tooltip.js#84 nikshepsvn, thanks for interesting on contribute to devtools, please glance on http://bugs.firefox-dev.tools/?easy&tool=all and you could find other available bugs.

Hi Fred, I'm a bit stuck at this one. This is what I am trying. let target = document.querySelectorAll(".requests-list-column.requests-list-status")[1]; let win = target.ownerDocument.defaultView; EventUtils.synthesizeMouseAtCenter(target, { type: "mousemove" }, win); yield waitUntil(() => target.title != ""); I add this above this line - https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_brotli.js#34 So I am trying to hover and make sure title attribute is populated for all the `status` columns before verification. I see the hover happening correctly but it keeps waiting and the condition of target.title != "" never gets satisfied. Interestingly, when I put debug points, I am able to see that the control reaches my method of getColumnTitle but still even after the method execution, title attribute isn't set. Can you help me understand where I am going wrong?

FYR here's how I passed the test by add the mouse hover simulation ``` let request = document.querySelectorAll(".request-list-item")[0]; let requestsListStatus = request.querySelector(".requests-list-status"); EventUtils.synthesizeMouse(requestsListStatus, 0, 0, { type: "mousemove" }, monitor.panelWin); yield waitUntil(() => document.querySelector(".requests-list-column.requests-list-status[title]")); ```

Thanks Fred. I have updated my PR with many changes. However, I am still facing a weird issue. So I have updated verifyRequestItemTarget to first do a mouseHover and then proceed. Interestingly, when I run browser_net_brotli.js in isolation, all tests pass but when I run the full suite, I still see 21 failures. Moreover, when I run browser_net_brotli.js with --verify option, some tests fail. So I am guessing this patch still needs some rework. Can you help me in understanding why test fails with --verify option?

passing `monitor` param seems eats the benifit of doing mouseHover in verifyRequestItemTarget. I suggest to fix only ~5 test files by adding mouseHover before verifyRequestItemTarget instead https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef827040d20c&selectedJob=136385522

Hi Fred, As per your suggestion, I have fixed around 5 tests by adding mouseHover before verifyRequestItemTarget. I ran `./mach test <test_name>` for each test I fixed and saw that there are no failures now. Can you take a quick look and tell me if this is the way forward?

Comment on attachment 8917512 [details] Bug 1407561 - Lazy loading of tooltip text when user hovers the status column. https://reviewboard.mozilla.org/r/188466/#review196616 Looks good, just some small test changes needed before landing. I triggered auto test and we can see if anything left. ::: devtools/client/netmonitor/test/browser_net_brotli.js:39 (Diff revision 3) > + let request = document.querySelector(".request-list-item"); > + let requestsListStatus = request.querySelector(".requests-list-status"); > + EventUtils.synthesizeMouse(requestsListStatus, 0, 0, { type: "mousemove" }, > + monitor.panelWin); > + yield waitUntil(() => > + document.querySelector(".requests-list-column.requests-list-status[title]")); or we can use more specific scope like `requestsListStatus.title` or `request.querySelector(.requests-list-status[title])` if above not work. Same for the rest test cases ::: devtools/client/netmonitor/test/browser_net_filter-01.js:166 (Diff revision 3) > is(!!document.querySelector(".network-details-panel"), true, > "The network details panel should render correctly."); > > // First test with single filters... > testFilterButtons(monitor, "all"); > + yield hoverOverRequests(); can we move `hoverOverRequests` into `testContents` if all testContents need to be done after `hoverOverRequests`? You can define `testContents` as generator via `function* testContents` and do `yield testContents`

Hi Fred, Thanks for the review. Please let me know if any other tests need fixing.

Hi Abhinav, recent update rename all react component file name so `request-list-column-status` now become `RequestListColumnStatus`. Could you help update accordingly?

Hi Fred, Sure, I will rename the file as well while I am addressing the review-comments. Is there any other test that we need to fix apart from the ones I have already fixed in the last patch?

I can't apply the patch yet since the file name is changed, I'll test again when the new PR is available. Thanks.

Netmonitor is now changed to ES6 class, so you might need to rebase again

Hi Fred, I have rebased with the latest code. Can you please check once from your side? Thanks Abhinav

Comment on attachment 8917512 [details] Bug 1407561 - Lazy loading of tooltip text when user hovers the status column. https://reviewboard.mozilla.org/r/188466/#review199902 Thanks for update! Local test passed and only 1 thing need to be addressed before landing. ::: devtools/client/netmonitor/src/components/RequestListColumnStatus.js:63 (Diff revision 4) > + div({ className: "requests-list-status-icon", "data-code": code }), > + div({ className: "requests-list-status-code" }, status) > + ) > + ); > > - if (statusText) { > + function getColumnTitle() { we can move getColumnTitle out of render function and pass the item param like http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListColumnWaterfall.js#67 Therefore the function can be offloaded from the render function ::: devtools/client/netmonitor/test/browser_net_brotli.js:34 (Diff revision 4) > yield ContentTask.spawn(tab.linkedBrowser, {}, function* () { > content.wrappedJSObject.performRequests(); > }); > yield wait; > > + let request = document.querySelector(".request-list-item"); nit: it will be nice to keep consistency name like `requestItem`across tests, but its fine to not change it.

based on test result, https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e5bf3a1acf0&selectedJob=140970700 we need to fix 3 more tests: * browser_net_cached-status.js * browser_net_filter-02.js * browser_net_filter-flags.js then we will good to go

Hi Fred, `browser_net_cached-status.js` was already fixed in the previous review request. However, this doesn't pass on TRY. Is it an intermittent issue or something genuine? In the latest review request, I have addressed the review comment, nit and have fixed browser_net_filter-02.js & browser_net_filter-flags.js.

With new result I saw more unexpected test cases failed https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fa5e53e9e3b&selectedJob=141258150 Could you help run `./mach test devtools/client/netmonitor/` locally and fix the rest test issues? The good news is the patch did show some perf gain (2~4% for simple netmonitor actions) https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=7fa5e53e9e3bf3019ed6742b05a80bd522be107b&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1&selectedTimeRange=172800

Hi Fred, I think I have understood the issue. There were 2 issues I feel: 1. This was kind of an implementation bug - Suppose I load some website and then even before the status code appears in the request item, I do a hover over the status. This results in "undefined" being shown as tooltip. Now after the status code is shown in the request item, if I hover again, then we can see the correct status displayed in the tooltip. To fix this, I have added a check of status & statusCode in the onMouseOver function. 2. I feel EventUtils.synthesizeMouse was causing some issue and maybe that's why some tests like "browser_net_cached-status.js" pass when run in isolation but fail when the whole suite is run. I have changed the implementation to use EventUtils.sendMouseEvent which passes all the tests now. On my local, with the new patch, all the tests are passing. Can we push to TRY once?

Comment on attachment 8917512 [details] Bug 1407561 - Lazy loading of tooltip text when user hovers the status column. https://reviewboard.mozilla.org/r/188466/#review200806 Thanks for figure out and fix the test issue, looks good to me! Only a nit need to be addressed before landing. ::: devtools/client/netmonitor/src/components/RequestListColumnStatus.js:55 (Diff revision 6) > - if (statusText) { > + return ( > + div({ > + className: "requests-list-column requests-list-status", > + onMouseOver: function ({ target }) { > + if (status && statusText) { > + if (!target.title) { conditions can be in the same line, like `if (status && statusText && !target.title)` ::: devtools/client/netmonitor/src/components/RequestListColumnStatus.js:71 (Diff revision 6) > +} > + > +function getColumnTitle(item) { > + let { fromCache, fromServiceWorker, status, statusText } = item; > + let title; > + if (status && statusText) { don't need double check here

Thanks Fred, I have fixed the same.

Comment on attachment 8917512 [details] Bug 1407561 - Lazy loading of tooltip text when user hovers the status column. https://reviewboard.mozilla.org/r/188466/#review200866 Looks good, let's wait the test result and land it! Thanks for the great contribution!

Pushed by flin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/38eb28830691 Lazy loading of tooltip text when user hovers the status column. r=gasolin

Backed out for failing /browser_net_filter-02.js Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141691302&repo=autoland&lineNumber=2350 Backout: https://hg.mozilla.org/integration/autoland/rev/6b456d6a8752ecf43452e9089b20848a1b92e2c5

Hi Andreaa, Fred, I'm sorry but I think this failed because I missed changing `EventUtils.synthesizeMouse` to `EventUtils.sendMouseEvent` for 2 tests - filter-02.js and filter-flags.js and both of them failed on autoland. I have fixed both of these tests to use `EventUtils.sendMouseEvent`. Fred, the above change should fix the issue, right?

Previous try test are all green before land https://treeherder.mozilla.org/#/jobs?repo=try&revision=41b7c09ded7c Though synthesizeMouse mimic more like actual user's behavior, it seems not stable enough for the hover event I think its right to use sendMouseEvent instead. push another try and wait for the test result. Thanks Abhinav for quickly address the test issue!

Pushed by flin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2c77266e1b40 Lazy loading of tooltip text when user hovers the status column. r=gasolin