1336383 - Implement RequestList component

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

Architecture reactify step for creating top level NetworkMonitor react component.

* Implement RequestListPanel component instead of requests-menu-view.js
* Remove requests-menu-view.js

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1336383 - Implement RequestsList component

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

Comment 2

[Ricky Chien [:rickychien] (inactive)]

In order to implement a react friendly Splitbox in bug 1309183, I'd prefer to get rid of requests-menu-view.js. Splitbox component will build on top of both RequestList (this one) and NetworkDetailsPanel and adopt full react style.

This is a big patch with a bunch of files changed because we used NetmonitorView.RequestsMenu global property in many mochitest files. It took a lot of my time to check and fix all these test failures step by step.

Comment 3

[Ricky Chien [:rickychien] (inactive)]

P.S. 

This is a great step to have a patch for getting rid of entire requests-menu-view.js. My feeling is that the requests-menu-view.js is the most dirty and worst modules in netmonitor. 

Architecture and code quality will be improved so much and it will be easier to introduce new feature such as React Virtualize.

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8835916 [details]
Bug 1336383 - Implement RequestsList component

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

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8835916 [details]
Bug 1336383 - Implement RequestsList component

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

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8835916 [details]
Bug 1336383 - Implement RequestsList component

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

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8835916 [details]
Bug 1336383 - Implement RequestsList component

https://reviewboard.mozilla.org/r/111472/#review112852

Great job here Ricky!

Just a few comments inline.

There is a lot of changes in the tests but, but they seem to follow the same pattern so, I scanned it quickly.

Honza

::: devtools/client/framework/test/browser_ignore_toolbox_network_requests.js:27
(Diff revision 4)
>  
>    is(panel.UI.editors.length, 1, "correct number of editors opened");
>  
>    let monitor = yield toolbox.selectTool("netmonitor");
> -  let { RequestsMenu } = monitor.panelWin.NetMonitorView;
> -  is(RequestsMenu.itemCount, 0, "No network requests appear in the network panel");
> +  let { gStore, windowRequire } = monitor.panelWin;
> +  let Actions = windowRequire("devtools/client/netmonitor/actions/index");

The `Actions` object is never used.

::: devtools/client/netmonitor/components/request-list-context-menu.js:41
(Diff revision 4)
> +}) {
> +  this.cloneSelectedRequest = cloneSelectedRequest;
> +  this.openStatistics = openStatistics;
> +}
> +
> +RequestListContextMenu.prototype = {

This isn't real React component. I think it shouldn't be in `components` directory

::: devtools/client/netmonitor/netmonitor-controller.js:701
(Diff revision 4)
> +            true));
> +        }
> +      }
> +    }
> +  }),
> +

Adding the `addRequest` and `updateRequest` methods in this module now is ok but, we should think about how to refactor the netmonitor-controller.js module. It's getting big.

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8835916 [details]
Bug 1336383 - Implement RequestsList component

https://reviewboard.mozilla.org/r/111472/#review112852

> This isn't real React component. I think it shouldn't be in `components` directory

Nice catch! I will move request-list-tooltip.js out of components dir as well.

> Adding the `addRequest` and `updateRequest` methods in this module now is ok but, we should think about how to refactor the netmonitor-controller.js module. It's getting big.

Right! I have to figure out how to improve netmonitor-controller.js in someday.

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8835916 [details]
Bug 1336383 - Implement RequestsList component

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

Comment 10

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8835916 [details]
Bug 1336383 - Implement RequestsList component

https://reviewboard.mozilla.org/r/111472/#review112866

R+ assuming Try is green.
Thanks!

Honza

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eee8f9b6791d
Implement RequestsList component r=Honza

Comment 12

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

Backed out for failing browser_net_har_post_data.js and more:

https://hg.mozilla.org/integration/autoland/rev/55916d685e88b8915fdda658c8dc8f0fadb0bfb4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=eee8f9b6791deff3db5fb7a3423ed04fdc4e6725&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=76371486&repo=autoland

[task 2017-02-10T16:35:58.632625Z] 16:35:58     INFO - TEST-PASS | devtools/client/netmonitor/har/test/browser_net_har_post_data.js | Request to reconfigure the tab was recorded. - 
[task 2017-02-10T16:35:58.632699Z] 16:35:58     INFO - Buffered messages logged at 16:35:58
[task 2017-02-10T16:35:58.632801Z] 16:35:58     INFO - Network monitor pane shown successfully.
[task 2017-02-10T16:35:58.632840Z] 16:35:58     INFO - Starting test... 
[task 2017-02-10T16:35:58.632986Z] 16:35:58     INFO - Buffered messages finished
[task 2017-02-10T16:35:58.633209Z] 16:35:58     INFO - TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/har/test/browser_net_har_post_data.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/har/test/browser_net_har_post_data.js:18 - TypeError: RequestsMenu is undefined
[task 2017-02-10T16:35:58.633252Z] 16:35:58     INFO - Stack trace:
[task 2017-02-10T16:35:58.633317Z] 16:35:58     INFO -     @chrome://mochitests/content/browser/devtools/client/netmonitor/har/test/browser_net_har_post_data.js:18:3
[task 2017-02-10T16:35:58.633372Z] 16:35:58     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-02-10T16:35:58.633431Z] 16:35:58     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-02-10T16:35:58.633504Z] 16:35:58     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59
[task 2017-02-10T16:35:58.633553Z] 16:35:58     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
[task 2017-02-10T16:35:58.633608Z] 16:35:58     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
[task 2017-02-10T16:35:58.633800Z] 16:35:58     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:744:59

Also:

TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/har/test/browser_net_har_throttle_upload.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/har/test/browser_net_har_throttle_upload.js:44 - TypeError: RequestsMenu is undefined

TEST-UNEXPECTED-FAIL | devtools/client/netmonitor/test/browser_net_copy_headers.js | Uncaught exception - at chrome://mochitests/content/browser/devtools/client/netmonitor/test/browser_net_copy_headers.js:46 - TypeError: monitor._toolbox.doc.querySelector(...) is null

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8835916 [details]
Bug 1336383 - Implement RequestsList component

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/030a883d87a7
Implement RequestsList component r=Honza

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/030a883d87a7

Comment 16

[[e-mail]]

There are some Warning when enable `ac_add_options --enable-debug-js-modules` in mozconfig

Warning: Failed prop type: Required prop `contextMenu` was not specified in `RequestListContent`.
    in RequestListContent (created by Connect(RequestListContent))
    in Connect(RequestListContent) (created by RequestList)
    in div (created by RequestList)
    in RequestList (created by Connect(RequestList))
    in Connect(RequestList)
    in Provider

Comment 17

[[e-mail]]

Warning: Failed prop type: Required prop `tooltip` was not specified in `RequestListContent`.
    in RequestListContent (created by Connect(RequestListContent))
    in Connect(RequestListContent) (created by RequestList)
    in div (created by RequestList)
    in RequestList (created by Connect(RequestList))
    in Connect(RequestList)
    in Provider

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Thanks for reporting this. Warnings will be addressed in bug 1309183.

Comment 19

[Ciprian Georgiu, Desktop QA]

This issue is verified fixed on latest Nightly 54.0a1 (2017-02-22) using the following platforms:
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS