Closed
Bug 1336384
Opened 7 years ago
Closed 7 years ago
Implement top level NetworkMonitor component
Categories
(DevTools :: Netmonitor, defect, P1)
DevTools
Netmonitor
Tracking
(firefox54 verified)
Tracking | Status | |
---|---|---|
firefox54 | --- | verified |
People
(Reporter: rickychien, Assigned: rickychien)
References
(Blocks 1 open bug)
Details
(Whiteboard: [netmonitor])
Attachments
(1 file)
Architecture reactify step for creating top level NetworkMonitor react component. * Implement NetworkMonitor component instead of netmonitor-view.js
Updated•7 years ago
|
Flags: qe-verify?
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
Priority: -- → P3
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor-reserve]
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Iteration: --- → 54.2 - Feb 20
Whiteboard: [netmonitor-reserve] → [netmonitor]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
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•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 15•7 years ago
|
||
mozreview-review |
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
Attachment #8836599 -
Flags: review?(gasolin) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-review |
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.
Attachment #8836599 -
Flags: review?(odvarko) → review+
Assignee | ||
Comment 19•7 years ago
|
||
Thanks! I can file a patch for reverting all async/await to Task.async if any objection arises.
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/53698c71b190 Implement top level NetworkMonitor component r=gasolin,Honza
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/53698c71b190
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 23•7 years ago
|
||
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.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•