Closed Bug 1307892 Opened 8 years ago Closed 7 years ago

Support network event update messages #196

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox54 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Iteration:
54.1 - Feb 6
Tracking Flags:
Tracking Status
firefox54 --- verified

People

(Reporter: linclark, Assigned: rickychien)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1307892 - Add support for network event update message
58 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Priority: -- → P2
Whiteboard: new-console
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: P2 → P3
Whiteboard: new-console → [reserve-new-console]
QA Contact: iulia.cristescu
Status: ASSIGNED → NEW
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.
Flags: needinfo?(rchien)
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)
(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.
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.
Attachment #8800067 - Attachment is obsolete: true
Attachment #8800067 - Flags: review?(lclark)
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 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 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+
Status: NEW → ASSIGNED
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/286078321ed3
Add support for network event update message r=nchevobbe
Iteration: --- → 54.1 - Feb 6
Priority: P3 → P1
https://hg.mozilla.org/mozilla-central/rev/286078321ed3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: qe-verify+
Whiteboard: [reserve-new-console]
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: