1336384 - Implement top level NetworkMonitor component

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

Architecture reactify step for creating top level NetworkMonitor react component.

* Implement NetworkMonitor component instead of netmonitor-view.js

Comment 1

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1336384 - Implement top level NetworkMonitor component

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

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

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

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

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

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

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

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

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

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

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

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

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

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

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

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

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

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111996/diff/9-10/

Comment 11

[Ricky Chien [:rickychien] (inactive)]

This patch depends on the patch in bug 1309183, so please apply bug 1309183 in advance if you would like to do manual testing.

Comment 12

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

I am getting following collisions (the splitter patch applied) when applying the patch:

$ hg qpush
applying monitorcomp
patching file devtools/client/netmonitor/components/monitor-panel.js
Hunk #2 FAILED at 47
Hunk #3 FAILED at 114
2 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/components/monitor-panel.js.rej
patching file devtools/client/netmonitor/components/request-list-header.js
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/components/request-list-header.js.rej
patching file devtools/client/netmonitor/netmonitor-view.js
Hunk #1 FAILED at 0
1 out of 1 hunks FAILED -- saving rejects to file devtools/client/netmonitor/netmonitor-view.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh monitorcomp

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111996/diff/10-11/

Comment 14

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111996/diff/11-12/

Comment 15

[[e-mail]]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

https://reviewboard.mozilla.org/r/111996/#review114016

LGTM, awesome!

::: devtools/client/netmonitor/components/monitor-panel.js:64
(Diff revision 12)
>      let {
>        formDataSections,
>        requestHeaders,
>        requestHeadersFromUploadStream,
>        requestPostData,
> -    } = request;
> +    } = request || {};

keeep using `request = {}` in params instead of `request || {}`

::: devtools/client/netmonitor/components/network-monitor.js:34
(Diff revision 12)
> +}
> +
> +NetworkMonitor.displayName = "NetworkMonitor";
> +
> +NetworkMonitor.propTypes = {
> +  statisticsOpen: PropTypes.bool,

missing .isRequired

::: devtools/client/netmonitor/panel.js
(Diff revision 12)
>    this.panelDoc = iframeWindow.document;
> -  this._toolbox = toolbox;
> -
> +  this.toolbox = toolbox;
> +  this.netmonitor = new iframeWindow.Netmonitor(toolbox);
> -  this._netmonitor = new iframeWindow.Netmonitor(toolbox);
> -
> -  EventEmitter.decorate(this);

surprisingly after remove that, test still works. debugger/new/panel also didn't include that

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111996/diff/12-13/

Comment 17

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111996/diff/13-14/

Comment 18

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

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

https://reviewboard.mozilla.org/r/111996/#review114166

Great job here!

R+ assuming Try is green

Honza

::: devtools/client/netmonitor/panel.js:17
(Diff revision 13)
> -exports.NetMonitorPanel = NetMonitorPanel;
> -
>  NetMonitorPanel.prototype = {
> -  /**
> -   * Open is effectively an asynchronous constructor.
> -   *
> +  open: async function () {
> +    if (!this.toolbox.target.isRemote) {
> +      await this.toolbox.target.makeRemote();

I am not sure if there is a concensus about using async/await instead of Task.async

I have been chatting on IRC about this and nobody seems to be against, but clear agreement seems to be missing.

I personally really like async/await, but we should be ready to revert this if somebody has objections.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Thanks! I can file a patch for reverting all async/await to Task.async if any objection arises.

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8836599 [details]
Bug 1336384 - Implement top level NetworkMonitor component

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/111996/diff/14-15/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/53698c71b190
Implement top level NetworkMonitor component r=gasolin,Honza

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/53698c71b190

Comment 23

[Ciprian Georgiu, Desktop QA]

This issue is verified fixed on latest Nightly 54.0a1 (2017-03-06) using Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.11.6. Marking here accordingly.