1407561 - Lazy loading of tooltip text when user onhover the status column

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[[e-mail]]

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.

Comment 1

[Abhinav Koppula]

Hi Fred,
Can I work on this one?

Comment 2

[nikshepsvn]

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 :)

Comment 3

[Abhinav Koppula]

Attachment: Bug 1407561 - Lazy loading of tooltip text when user hovers the status column.

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

Comment 4

[[e-mail]]

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.

Comment 5

[Abhinav Koppula]

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?

Comment 6

[[e-mail]]

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]"));
```

Comment 7

[Abhinav Koppula]

Comment on attachment 8917512 [details]
Bug 1407561 - Lazy loading of tooltip text when user hovers the status column.

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

Comment 8

[Abhinav Koppula]

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?

Comment 9

[[e-mail]]

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

Comment 10

[Abhinav Koppula]

Comment on attachment 8917512 [details]
Bug 1407561 - Lazy loading of tooltip text when user hovers the status column.

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

Comment 11

[Abhinav Koppula]

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 12

[[e-mail]]

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`

Comment 13

[Abhinav Koppula]

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

Comment 14

[[e-mail]]

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

Comment 15

[Abhinav Koppula]

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?

Comment 16

[[e-mail]]

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

Comment 17

[[e-mail]]

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

Comment 18

[Abhinav Koppula]

Comment on attachment 8917512 [details]
Bug 1407561 - Lazy loading of tooltip text when user hovers the status column.

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

Comment 19

[Abhinav Koppula]

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

Thanks
Abhinav

Comment 20

[[e-mail]]

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.

Comment 21

[[e-mail]]

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

Comment 22

[Abhinav Koppula]

Comment on attachment 8917512 [details]
Bug 1407561 - Lazy loading of tooltip text when user hovers the status column.

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

Comment 23

[Abhinav Koppula]

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.

Comment 24

[[e-mail]]

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

Comment 25

[Abhinav Koppula]

Comment on attachment 8917512 [details]
Bug 1407561 - Lazy loading of tooltip text when user hovers the status column.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188466/diff/5-6/

Comment 26

[Abhinav Koppula]

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 27

[[e-mail]]

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

Comment 28

[Abhinav Koppula]

Comment on attachment 8917512 [details]
Bug 1407561 - Lazy loading of tooltip text when user hovers the status column.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188466/diff/6-7/

Comment 29

[Abhinav Koppula]

Thanks Fred, I have fixed the same.

Comment 30

[[e-mail]]

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!

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38eb28830691
Lazy loading of tooltip text when user hovers the status column. r=gasolin

Comment 32

[Andreea Pavel [:apavel]]

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

Comment 33

[Abhinav Koppula]

Comment on attachment 8917512 [details]
Bug 1407561 - Lazy loading of tooltip text when user hovers the status column.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188466/diff/7-8/

Comment 34

[Abhinav Koppula]

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?

Comment 35

[[e-mail]]

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!

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c77266e1b40
Lazy loading of tooltip text when user hovers the status column. r=gasolin

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2c77266e1b40

Comment 38

[Intermittent Failures Robot]

28 failures in 857 pushes (0.033 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-central: 28

Platform breakdown:
* windows10-64: 9
* linux64: 9
* windows7-32: 7
* osx-10-10: 3

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1407561&startday=2017-10-30&endday=2017-11-05&tree=all