Closed
Bug 1307892
Opened 8 years ago
Closed 8 years ago
Support network event update messages #196
Categories
(DevTools :: Console, enhancement, P1)
DevTools
Console
Tracking
(firefox54 verified)
Tracking | Status | |
---|---|---|
firefox54 | --- | verified |
People
(Reporter: linclark, Assigned: rickychien)
References
Details
Attachments
(1 file, 1 obsolete file)
Originally posted by:linclark see https://github.com/devtools-html/gecko-dev/issues/196 Parent issue: #140 Where it is received: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.js#3348 Where it comes from: 10 different places, starting here https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/webconsole.js#2064
Reporter | ||
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: new-console
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•8 years ago
|
||
Move PR from https://github.com/devtools-html/gecko-dev/issues/196 to bugzilla.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Blocks: enable-new-console
Updated•8 years ago
|
Flags: qe-verify+
Priority: P2 → P3
Whiteboard: new-console → [reserve-new-console]
Updated•8 years ago
|
QA Contact: iulia.cristescu
Updated•8 years ago
|
Status: ASSIGNED → NEW
Comment 6•8 years ago
|
||
Hi Ricky, We are currently planning the remaining work for the new webconsole. It would be great to resume the work on this bug, since you already did most of the work! Do you think that before resuming here, we should think more about sharing a component between the netmonitor and the console to display network messages? I think this topic was already discussed for inspecting network messages in the console, but maybe it also makes sense for the component represent the network message itself ? (added Honza in cc too) If not, would be great to get rebased version of your patches so that we can kick off some reviews.
Updated•8 years ago
|
Flags: needinfo?(rchien)
Assignee | ||
Comment 7•8 years ago
|
||
Cool! I'm waiting this for a long time.
> maybe it also makes sense for the component represent the network message itself ?
I don't understand what you mean here. Could you explain more about it?
Flags: needinfo?(rchien)
Comment 8•8 years ago
|
||
(In reply to Ricky Chien [:rickychien] from comment #7) > Cool! I'm waiting this for a long time. > > > maybe it also makes sense for the component represent the network message itself ? > > I don't understand what you mean here. Could you explain more about it? Not sure this explains the quoted part but just for some extra context - we were discussing the HTTP inspector today and how we might want to share a component between netmonitor and console for request inspection porting the current inspector over to the new console. This was based on your investigation in the console fork: https://github.com/devtools-html/original-console-fork/issues/213#issuecomment-250067212. I'm still interested in doing this if it fits in with the current plans for the netmonitor.
Assignee | ||
Comment 9•8 years ago
|
||
There are three steps need to be done for supporting network message log as well as inline HTTP inspector. 1. Support network event message (which has been done last year) https://github.com/devtools-html/original-console-fork/pull/262 2. Support network event update message (which is this bug) 3. Incorporate HTTP inspector https://github.com/devtools-html/original-console-fork/issues/213 - append an inline HTTP inspector under network message log. I think you're asking the third one IIUC. If so, it's irrelevant to this bug. But yes, the HTTP inspector is something that we'd like to share with netmonitor. We might want to merge the work from bug 1328197 which I'm working on. IIRC, integration of HTTP inspector should be based on some code cleanup and refactoring in net-request.js since it hasn't react + redux friendly yet. So I think it's worth to kick off some clean up and refactoring before waiting for bug 1328197. It would help with smoothing integration progress a lots.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8800067 -
Attachment is obsolete: true
Attachment #8800067 -
Flags: review?(lclark)
Assignee | ||
Comment 11•8 years ago
|
||
This is a Chinese new year patch which rebased old patch on top of latest m-c. Manual testing works perfectly and console network status displayed as expected. Patch also came along with a little fix for layout issue. So far the longer message is broken and it exceeds over the console panel, but right now the problem has gone away after applying this patch. Nicolas, I think you might be the best person who is familiar with new webconsole architecture. Please take a look thanks!
Comment 12•8 years ago
|
||
mozreview-review |
Comment on attachment 8800066 [details] Bug 1307892 - Add support for network event update message https://reviewboard.mozilla.org/r/85092/#review109204 Thanks Ricky (and happy new year !) ::: devtools/client/webconsole/new-console-output/reducers/messages.js:103 (Diff revision 3) > + return state.withMutations(function (record) { > + record.set("messagesById", messagesById.map((message, key) => { > + if (message.id === updateMessage.id) { > + return updateMessage; > + } > + return message; > + })); I think we could use `record.set("messagesUiById", messagesUiById.set(updateMessage.id, updateMessage))` , don't you think ? ::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_update_stubs_network_event.js:12 (Diff revision 3) > +// A typical network event request will follow by 8 types of different network > +// event update packets > +const NETWORK_EVENT_UPDATE_COUNT = 8; It seems a little brittle to test the number of update. It would be nice if we could have a specific event emitted when all the updates are done. Is something like this possible ? ::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_update_stubs_network_event.js:41 (Diff revision 3) > + let networkEventUpdate = new Promise(resolve => { > + let i = 0; > + toolbox.target.activeConsole.on(`${TARGET}Update`, (type, res) => { > + stubs.packets.push(formatPacket(`${keys[0]} ${res.packet.updateType}`, res)); > + stubs.preparedMessages.push( > + formatNetworkEventStub(`${keys[0]} ${res.packet.updateType}`, res)); > + if (++i === NETWORK_EVENT_UPDATE_COUNT) { > + resolve(); > + } > + }); > + }); so this work if we assume there is always only one item in `keys`. I know it might not be common, but we should handle it properly in case of several items in `keys`.
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8800066 [details] Bug 1307892 - Add support for network event update message https://reviewboard.mozilla.org/r/85092/#review109228 r+ with fixes on my comments and a green TRY :) ::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:165 (Diff revision 4) > + if (res.packet.updateType === "eventTimings") { > + this.jsterm.hud.emit("network-message-updated", res); > + } nit: could we add a comment saying that the eventTiming update is the last one of 8 updates hapenning on network message update and that we do that for testing purpose ? ::: devtools/client/webconsole/new-console-output/test/fixtures/stub-generators/browser_webconsole_update_stubs_network_event.js:40 (Diff revision 4) > + ui.jsterm.hud.on("network-message-updated", function onNetworkUpdated(event, res) { > + ui.jsterm.hud.off("network-message-updated", onNetworkUpdated); > + stubs.preparedMessages.push( > + formatNetworkEventStub(`${keys[i]} ${res.packet.updateType}`, res)); > + resolve(); in here, we should check that `i === keys.length` before calling resolve (same thing that we do for the upper `networkEvent` promise)
Attachment #8800066 -
Flags: review?(chevobbe.nicolas) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 16•8 years ago
|
||
Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/286078321ed3 Add support for network event update message r=nchevobbe
Updated•8 years ago
|
Iteration: --- → 54.1 - Feb 6
Priority: P3 → P1
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/286078321ed3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 18•8 years ago
|
||
The bug is verified fixed on latest Nightly 54.0a1 (2017-02-08) using Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.
Updated•8 years ago
|
Whiteboard: [reserve-new-console]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•