Closed Bug 1336383 Opened 7 years ago Closed 7 years ago

Implement RequestList component

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox54 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Iteration:
54.2 - Feb 20
Tracking Flags:
Tracking Status
firefox54 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

Bug 1336383 - Implement RequestsList component
59 bytes, text/x-review-board-request
Honza
: review+
Details 
Architecture reactify step for creating top level NetworkMonitor react component.

* Implement RequestListPanel component instead of requests-menu-view.js
* Remove requests-menu-view.js
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Priority: -- → P3
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Summary: Implement RequestListPanel component → Implement RequestList component
Blocks: 1309183
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.
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.
Iteration: --- → 54.2 - Feb 20
Priority: P3 → P1
Whiteboard: [netmonitor-reserve] → [netmonitor]
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.
Attachment #8835916 - Flags: review?(odvarko)
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 on attachment 8835916 [details]
Bug 1336383 - Implement RequestsList component

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

R+ assuming Try is green.
Thanks!

Honza
Attachment #8835916 - Flags: review?(odvarko) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eee8f9b6791d
Implement RequestsList component r=Honza
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
Flags: needinfo?(rchien)
Flags: needinfo?(rchien)
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/030a883d87a7
Implement RequestsList component r=Honza
https://hg.mozilla.org/mozilla-central/rev/030a883d87a7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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
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
Thanks for reporting this. Warnings will be addressed in bug 1309183.
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
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: