Closed Bug 1362036 Opened 7 years ago Closed 7 years ago

Implement http inspection in new console

Categories

(DevTools :: Console, enhancement, P1)

Product:
DevTools
Component:
Console
Type:
enhancement

Tracking

(firefox57 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Flags:
Tracking Status
firefox57 --- verified

People

(Reporter: bgrins, Assigned: Honza)

References

(Depends on 1 open bug)

Details

(Whiteboard: [reserve-console-html] )

Attachments

(6 files)

httpi.png
77.45 KB, image/png
Details
Bug 1362036 - Implement http inspection in new console;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1362036 - Remember active network tab id;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1362036 - Open in network panel;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1362036 - Implement tests;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details
Bug 1362036 - Generate stubs;
59 bytes, text/x-review-board-request
nchevobbe
: review+
Details 
With the new netmonitor frontend we should hopefully be able to reuse a component from that tool for the new console, instead of relying on xhrspy as in the old console.

This was originally decided to not block enabling the new console by default (https://bugzilla.mozilla.org/show_bug.cgi?id=1308219#c2), but it does seem especially important to the browser console, so blocking at least Bug 1362023.
Blocks: enable-new-console
Priority: -- → P3
Whiteboard: [reserve-console-html]
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: iulia.cristescu
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Iteration: --- → 57.1 - Aug 15
Priority: P3 → P1
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
As reported in https://bugzilla.mozilla.org/show_bug.cgi?id=1318315#c2, we should make sure the new inspection UI doesn't incorrectly enable the 'Store as global variable' context menu item.
See Also: → 1318315
Attached image httpi.png — 
Some CSS needed, but this is the current status.

Honza
Current patch wip (not ready for review yet)

Honza
The patch is ready for review.

I am working on a test now.

Honza
Comment on attachment 8899478 [details]
Bug 1362036 - Implement http inspection in new console;

https://reviewboard.mozilla.org/r/170758/#review176744

::: devtools/client/webconsole/new-console-output/components/message-types/network-event-details.js:46
(Diff revision 3)
> + * This component is responsible for rendering HTTP details
> + * data visible to the user if the parent HTTP request
> + * is expanded. It uses the same set of components as
> + * the Net panel in the Side bar.
> + */
> +function NetworkEventDetails(props) {

This component is largely duplicated from https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/src/components/tabbox-panel.js

The only difference being the props being passed in.

I think it would be better to modify and use the netmonitor component. Otherwise, if we change the set of tabs in the netmonitor, they won't automatically be up-to-date with the console.

::: devtools/client/webconsole/webconsole.xhtml:16
(Diff revision 3)
>      <link rel="stylesheet" href="chrome://devtools/skin/webconsole.css"/>
>      <link rel="stylesheet" href="chrome://devtools/skin/components-frame.css"/>
>      <link rel="stylesheet" href="resource://devtools/client/shared/components/reps/reps.css"/>
> +    <link rel="stylesheet" href="resource://devtools/client/shared/components/tabs/tabs.css"/>
> +    <link rel="stylesheet" href="resource://devtools/client/shared/components/tabs/tabbar.css"/>
> +    <link rel="stylesheet" href="chrome://devtools/content/netmonitor/src/assets/styles/netmonitor.css"/>

I'm not sure loading the complete netmonitor stylesheet is a good idea. It would be better to split the parts used by the netmonitor only from the parts that can be shared.
I've tested this, and it works pretty nicely!


I did notice one bug though: "Edit and Resend" doesn't work, it should probably be hidden from the console Http Inspector.
Thanks for looking at the patch Tim.
Good points, I am working on it.
Honza
Comment on attachment 8899478 [details]
Bug 1362036 - Implement http inspection in new console;

https://reviewboard.mozilla.org/r/170758/#review177250

The code looks good to me, and it's working quite well.
r- because there are some glitches: 
 - The panel can feel a bit small,maybe we could assign it a max-height of 80% or something alike so we take advantage of the availabel space. In the old console, the panel could be quite tall
 - When the request inspector is narrow, there is an overflow button so the user can access hidden tabs. But when I click on it the menu is at the wrong place (https://files.slack.com/files-pri/T3V7H1198-F6TTQCTNJ/screen_shot_2017-08-24_at_10.21.32.png)
 - The response tab can't be scrolled (https://files.slack.com/files-pri/T3V7H1198-F6T39NU12/screen_shot_2017-08-24_at_10.23.53.png)
 - I think we should remove the click handler on the request which takes the user to the netmonitor. Clicking the request should expand/collapse the request inspector
 - the "Edit and resend" button does nothing


Not really in the scope of this review sicne it's also the case in netmonitor panel, but the "raw headers" button keeps the same look, even when raw headers are displayed. We should style it with the `.active` class (so it is blue, like filter buttons in the console)

::: devtools/client/netmonitor/src/components/tabbox-panel.js:116
(Diff revision 4)
>    (state) => ({
> -    activeTabId: state.ui.detailsPanelSelectedTab,
> -    request: getSelectedRequest(state),
>    }),

Is this function still needed since it does not do anything now ? `connect` accepts null or undefined for this argument if we don't want to map the state to props.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:95
(Diff revision 4)
> +        fromCache,
> +        fromServiceWorker,
> +      },
> +      true,
> +    )
> +    .then(() => window.emit(EVENTS.REQUEST_ADDED, id));

nit: can this be turned into an async/await function, like the others function of this class ?

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:188
(Diff revision 4)
> +    if (requestPostData && requestPostData.postData) {
> +      let { text } = requestPostData.postData;
> +      let postData = await this.getLongString(text);
> +      const headers = CurlUtils.getHeadersFromMultipartText(postData);
> +      const headersSize = headers.reduce((acc, { name, value }) => {
> +        return acc + name.length + value.length + 2;

nit: could there be a comment here explaining why we add "2" to the addition ?

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:252
(Diff revision 4)
> +   * Packet order of "networkUpdateEvent" is predictable, as a result we can wait for
> +   * the last one "eventTimings" packet arrives to check payload is ready.

I don't think this is true, we had problem in the test generating network stubs because of this false assumption.

Don't you have intermittents that could be caused by that ?

::: devtools/client/webconsole/new-console-output/components/console-output.js:70
(Diff revision 4)
>  
>      const lastChild = outputNode.lastChild;
>      const visibleMessagesDelta =
>        nextProps.visibleMessages.length - this.props.visibleMessages.length;
>      const messagesDelta =
> -      nextProps.messages.length - this.props.messages.length;
> +      nextProps.messages.size - this.props.messages.size;

Thanks for this, I saw those warning messages too :)

::: devtools/client/webconsole/new-console-output/components/message-types/network-event-message.js:85
(Diff revision 4)
> +  const attachment = dom.div({className: "network-info devtools-monospace"},
> +    open && TabboxPanel({

could we even have attachment to `null` if `open` is falsy ? Or do we need the `div.network-info` even if the user did not expanded the message ?

::: devtools/client/webconsole/new-console-output/components/message-types/network-event-message.js:87
(Diff revision 4)
>    const messageBody = [method, xhr, url, statusBody];
>  
> +  // Only render the attachment content if the network-event is actually opened.
> +  const attachment = dom.div({className: "network-info devtools-monospace"},
> +    open && TabboxPanel({
> +      activeTabId: "headers",

could this be a constant somewhere in netmonitor ? I fear that someday the name changes and this won't be catched.

::: devtools/client/webconsole/new-console-output/components/message-types/network-event-message.js:90
(Diff revision 4)
> +      cloneSelectedRequest: () => {},
> +      selectTab: (tabId) => {},

are these needed ? Could we test in TabboxPanel that we do have those props before calling them instead ?

::: devtools/client/webconsole/new-console-output/components/message-types/network-event-message.js:103
(Diff revision 4)
>      source,
>      type,
>      level,
>      indent,
> +    collapsible: true,
> +    open: open,

nit: only use `open,`

::: devtools/client/webconsole/new-console-output/reducers/messages.js:205
(Diff revision 4)
>              ...messagesToShow,
>              ...visibleMessages.slice(insertIndex),
>            ]);
>          }
> +
> +        // If the current message is a network event mark it as opened-once,

nit: Add a comma in "…is a network event, mark it as…"

::: devtools/client/webconsole/new-console-output/reducers/messages.js:205
(Diff revision 4)
> +        // If the current message is a network event mark it as opened-once,
> +        // so HTTP details are not fetched again the next time the user
> +        // opens the log.
> +        if (currMessage.source == "network") {
> +          record.set("messagesById",
> +            messagesById.set(
> +              action.id, Object.assign({},
> +                currMessage, {
> +                  openedOnce: true
> +                }
> +              )
> +            )
> +          );

I am not sure about this, can't we just check the store to see if we have the data already, and if not, fetch it ?

::: devtools/client/webconsole/new-console-output/reducers/messages.js:284
(Diff revision 4)
> +          switch (key) {
> +            case "securityInfo":
> +              request.securityState = value.state;
> +              break;
> +            case "requestPostData":
> +              request.requestHeadersFromUploadStream = {
> +                headers: [],
> +                headersSize: 0,
> +              };
> +              break;
> +          }

do we have the same thing in netmonitor too ? It seems like a good thing to put on a helper or something so future addition/fixes will be shared

::: devtools/client/webconsole/new-console-output/store.js:162
(Diff revision 4)
> +        addRequest: (id, data, batch) => {
> +          // Handled by `WebConsoleConnectionProxy` and this
> +          // middleware is only responsible for fetching
> +          // HTTP details through `updateRequest` method.
> +        },

I don't understand why we need to have an empty function here.
If it's somehow needed by the caller, I'd rather test there that actions.addRequest exists before calling it than having a stub here.

::: devtools/client/webconsole/new-console-output/store.js:189
(Diff revision 4)
> +
> +      // If network message has been opened, fetch all
> +      // HTTP details from the backend.
> +      if (type == MESSAGE_OPEN) {
> +        let message = getMessage(state, action.id);
> +        if (!message.openedOnce && message.source == "network") {

related to my previous comment about openedOnce, maybe we could check here that we have some data on the store. This can be tricky since we do have network update even if we don't open the message (for timing update I think).
Feel free to let this as is if another approach feels clunkier.

::: devtools/client/webconsole/panel.js:91
(Diff revision 4)
>          return this;
>        }, (reason) => {
>          let msg = "WebConsolePanel open failed. " +
>                    reason.error + ": " + reason.message;
>          dump(msg + "\n");
> -        console.error(msg);
> +        console.error(msg, reason);

is this wanted ?
Attachment #8899478 - Flags: review?(nchevobbe) → review-
Comment on attachment 8900671 [details]
Bug 1362036 - Open in network panel;

https://reviewboard.mozilla.org/r/172100/#review177380

::: devtools/client/webconsole/new-console-output/components/message-types/network-event-message.js:82
(Diff revision 1)
>  
>    if (httpVersion && status && statusText !== undefined && totalTime !== undefined) {
>      statusInfo = `[${httpVersion} ${status} ${statusText} ${totalTime}ms]`;
>    }
>  
> -  const openNetworkMonitor = serviceContainer.openNetworkPanel
> +  const expand = () => {

nit: we could call this toggle or something alike, since it would better reflect what the function can do
Attachment #8900671 - Flags: review?(nchevobbe) → review+
Comment on attachment 8900670 [details]
Bug 1362036 - Remember active network tab id;

https://reviewboard.mozilla.org/r/172098/#review177382

The code is looking good to me. We should have tests for that, at least for checking that the store behaves like it should

::: devtools/client/webconsole/new-console-output/reducers/ui.js:19
(Diff revision 1)
>  
>  const UiState = Immutable.Record({
>    filterBarVisible: false,
>    filteredMessageVisible: false,
>    timestampsVisible: true,
> +  networkMessageActiveTabId: "headers",

Like said in the other review, we should use a constant for each id in http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/tabbox-panel.js#57-100 , so we can re-use them from anywhere.

So here, we would have NETWORK_CONSTANTS.TABS.HEADERS for example.

::: devtools/client/webconsole/new-console-output/store.js:56
(Diff revision 1)
>        net: Services.prefs.getBoolPref(PREFS.FILTER.NET),
>        netxhr: Services.prefs.getBoolPref(PREFS.FILTER.NETXHR),
>      }),
>      ui: new UiState({
>        filterBarVisible: Services.prefs.getBoolPref(PREFS.UI.FILTER_BAR),
> +      networkMessageActiveTabId: "headers",

so here we could use the constants (e.g. NETWORK_CONSTANTS.TABS.HEADER)
Attachment #8900670 - Flags: review?(nchevobbe) → review+
Thanks for the review!

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
> Comment on attachment 8899478 [details]
> Bug 1362036 - Implement http inspection in new console;
> 
> https://reviewboard.mozilla.org/r/170758/#review177250
> 
> The code looks good to me, and it's working quite well.
> r- because there are some glitches: 
>  - The panel can feel a bit small,maybe we could assign it a max-height of
> 80% or something alike so we take advantage of the availabel space. In the
> old console, the panel could be quite tall
The current height is 250px. The limit is mainly for the Response tab.
Not sure how we could use 80%, the Console panel content doesn't have
fix height.

>  - When the request inspector is narrow, there is an overflow button so the
> user can access hidden tabs. But when I click on it the menu is at the wrong
> place
We should file a bug for this and fix as part of the Net panel.

> (https://files.slack.com/files-pri/T3V7H1198-F6TTQCTNJ/screen_shot_2017-08-
> 24_at_10.21.32.png)
>  - The response tab can't be scrolled
We should file a bug in the Network component.

> (https://files.slack.com/files-pri/T3V7H1198-F6T39NU12/screen_shot_2017-08-
> 24_at_10.23.53.png)
>  - I think we should remove the click handler on the request which takes the
> user to the netmonitor. Clicking the request should expand/collapse the
Done

> request inspector
>  - the "Edit and resend" button does nothing
The button is hidden now

> Not really in the scope of this review sicne it's also the case in
> netmonitor panel, but the "raw headers" button keeps the same look, even
> when raw headers are displayed. We should style it with the `.active` class
> (so it is blue, like filter buttons in the console)
Yep, we should file a bug in Network component.

> ::: devtools/client/netmonitor/src/components/tabbox-panel.js:116
> (Diff revision 4)
> >    (state) => ({
> > -    activeTabId: state.ui.detailsPanelSelectedTab,
> > -    request: getSelectedRequest(state),
> >    }),
> 
> Is this function still needed since it does not do anything now ? `connect`
> accepts null or undefined for this argument if we don't want to map the
> state to props.
Fixed

> ::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:95
> (Diff revision 4)
> > +        fromCache,
> > +        fromServiceWorker,
> > +      },
> > +      true,
> > +    )
> > +    .then(() => window.emit(EVENTS.REQUEST_ADDED, id));
> 
> nit: can this be turned into an async/await function, like the others
> function of this class ?
Done

> ::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:188
> (Diff revision 4)
> > +    if (requestPostData && requestPostData.postData) {
> > +      let { text } = requestPostData.postData;
> > +      let postData = await this.getLongString(text);
> > +      const headers = CurlUtils.getHeadersFromMultipartText(postData);
> > +      const headersSize = headers.reduce((acc, { name, value }) => {
> > +        return acc + name.length + value.length + 2;
> 
> nit: could there be a comment here explaining why we add "2" to the addition
> ?
Done

> ::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:252
> (Diff revision 4)
> > +   * Packet order of "networkUpdateEvent" is predictable, as a result we can wait for
> > +   * the last one "eventTimings" packet arrives to check payload is ready.
> 
> I don't think this is true, we had problem in the test generating network
> stubs because of this false assumption.
> 
> Don't you have intermittents that could be caused by that ?
This needs another bug report. There is more related to this e.g.

const NUMBER_OF_NETWORK_UPDATE = 8;
if (res.networkInfo.updates.length === NUMBER_OF_NETWORK_UPDATE) {
  batchedMessageAdd(actions.networkMessageUpdate(message));
  this.jsterm.hud.emit("network-message-updated", res);
}

> devtools/client/webconsole/new-console-output/components/message-types/
> network-event-message.js:85
> (Diff revision 4)
> > +  const attachment = dom.div({className: "network-info devtools-monospace"},
> > +    open && TabboxPanel({
> 
> could we even have attachment to `null` if `open` is falsy ? Or do we need
> the `div.network-info` even if the user did not expanded the message ?
Done

> > +    open && TabboxPanel({
> > +      activeTabId: "headers",
> 
> could this be a constant somewhere in netmonitor ? I fear that someday the
> name changes and this won't be catched.
Done


> network-event-message.js:90
> (Diff revision 4)
> > +      cloneSelectedRequest: () => {},
> > +      selectTab: (tabId) => {},
> 
> are these needed ? Could we test in TabboxPanel that we do have those props
> before calling them instead ?
Fixed

> devtools/client/webconsole/new-console-output/components/message-types/
> network-event-message.js:103
> (Diff revision 4)
> >      source,
> >      type,
> >      level,
> >      indent,
> > +    collapsible: true,
> > +    open: open,
> 
> nit: only use `open,`
Done

> ::: devtools/client/webconsole/new-console-output/reducers/messages.js:205
> (Diff revision 4)
> >              ...messagesToShow,
> >              ...visibleMessages.slice(insertIndex),
> >            ]);
> >          }
> > +
> > +        // If the current message is a network event mark it as opened-once,
> 
> nit: Add a comma in "…is a network event, mark it as…"
Done

> ::: devtools/client/webconsole/new-console-output/reducers/messages.js:205
> (Diff revision 4)
> > +        // If the current message is a network event mark it as opened-once,
> > +        // so HTTP details are not fetched again the next time the user
> > +        // opens the log.
> > +        if (currMessage.source == "network") {
> > +          record.set("messagesById",
> > +            messagesById.set(
> > +              action.id, Object.assign({},
> > +                currMessage, {
> > +                  openedOnce: true
> > +                }
> > +              )
> > +            )
> > +          );
> 
> I am not sure about this, can't we just check the store to see if we have
> the data already, and if not, fetch it ?
I was looking for better solution, but this seems to be the best for now.
We should check together with the NUMBER_OF_NETWORK_UPDATE issue.

> ::: devtools/client/webconsole/new-console-output/reducers/messages.js:284
> (Diff revision 4)
> > +          switch (key) {
> > +            case "securityInfo":
> > +              request.securityState = value.state;
> > +              break;
> > +            case "requestPostData":
> > +              request.requestHeadersFromUploadStream = {
> > +                headers: [],
> > +                headersSize: 0,
> > +              };
> > +              break;
> > +          }
> 
> do we have the same thing in netmonitor too ? It seems like a good thing to
> put on a helper or something so future addition/fixes will be shared
Could be a follow up here (related to different data-structure between 
the Console and Net panels)

> ::: devtools/client/webconsole/new-console-output/store.js:162
> (Diff revision 4)
> > +        addRequest: (id, data, batch) => {
> > +          // Handled by `WebConsoleConnectionProxy` and this
> > +          // middleware is only responsible for fetching
> > +          // HTTP details through `updateRequest` method.
> > +        },
> 
> I don't understand why we need to have an empty function here.
> If it's somehow needed by the caller, I'd rather test there that
> actions.addRequest exists before calling it than having a stub here.
Removed

> ::: devtools/client/webconsole/panel.js:91
> (Diff revision 4)
> >          return this;
> >        }, (reason) => {
> >          let msg = "WebConsolePanel open failed. " +
> >                    reason.error + ": " + reason.message;
> >          dump(msg + "\n");
> > -        console.error(msg);
> > +        console.error(msg, reason);
> 
> is this wanted ?
This is actually quite useful, the original error reporting didn't provide a stack trace.


(In reply to Nicolas Chevobbe [:nchevobbe] from comment #18)
> Comment on attachment 8900671 [details]
> Bug 1362036 - Open in network panel;
> 
> https://reviewboard.mozilla.org/r/172100/#review177380
> 
> :::
> devtools/client/webconsole/new-console-output/components/message-types/
> network-event-message.js:82
> (Diff revision 1)
> >  
> >    if (httpVersion && status && statusText !== undefined && totalTime !== undefined) {
> >      statusInfo = `[${httpVersion} ${status} ${statusText} ${totalTime}ms]`;
> >    }
> >  
> > -  const openNetworkMonitor = serviceContainer.openNetworkPanel
> > +  const expand = () => {
> 
> nit: we could call this toggle or something alike, since it would better
> reflect what the function can do
Done


(In reply to Nicolas Chevobbe [:nchevobbe] from comment #19)
> Like said in the other review, we should use a constant for each id in
> http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/
> components/tabbox-panel.js#57-100 , so we can re-use them from anywhere.
> 
> So here, we would have NETWORK_CONSTANTS.TABS.HEADERS for example.

> ::: devtools/client/webconsole/new-console-output/store.js:56
> (Diff revision 1)
> >        net: Services.prefs.getBoolPref(PREFS.FILTER.NET),
> >        netxhr: Services.prefs.getBoolPref(PREFS.FILTER.NETXHR),
> >      }),
> >      ui: new UiState({
> >        filterBarVisible: Services.prefs.getBoolPref(PREFS.UI.FILTER_BAR),
> > +      networkMessageActiveTabId: "headers",
> 
> so here we could use the constants (e.g. NETWORK_CONSTANTS.TABS.HEADER)
Yep, done


I am working on tests now.

Honza
Comment on attachment 8899478 [details]
Bug 1362036 - Implement http inspection in new console;

https://reviewboard.mozilla.org/r/170758/#review177998

Looks good.
Let's push this when we have tests for it :)

::: devtools/client/netmonitor/src/components/tabbox-panel.js:117
(Diff revision 6)
>    (state) => ({
> -    activeTabId: state.ui.detailsPanelSelectedTab,
> -    request: getSelectedRequest(state),
>    }),

Do we still need this empty function (we could replace it with null, or add a comment to explain why we keep it)
Attachment #8899478 - Flags: review?(nchevobbe) → review+
Comment on attachment 8899478 [details]
Bug 1362036 - Implement http inspection in new console;

https://reviewboard.mozilla.org/r/170758/#review178002

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:269
(Diff revision 6)
> +   * @param {string} id request id
> +   * @return {boolean} return whether a specific networkEvent has been updated completely.
> +   */
> +  isQueuePayloadReady(id) {
> +    let queuedPayload = this.getPayloadFromQueue(id);
> +    return queuedPayload && queuedPayload.payload.eventTimings;

Could we add a TODO which say that we should find a better solution since it might happen that eventTimings is not the last update ?
Comment on attachment 8900670 [details]
Bug 1362036 - Remember active network tab id;

https://reviewboard.mozilla.org/r/172098/#review178014

::: devtools/client/webconsole/new-console-output/reducers/ui.js:19
(Diff revision 2)
>  
>  const UiState = Immutable.Record({
>    filterBarVisible: false,
>    filteredMessageVisible: false,
>    timestampsVisible: true,
> +  networkMessageActiveTabId: "headers",

this should use the new constant PANELS.HEADERS , I'm not sure if there this change somewhere
> ::: devtools/client/netmonitor/src/components/tabbox-panel.js:117
> (Diff revision 6)
> >    (state) => ({
> > -    activeTabId: state.ui.detailsPanelSelectedTab,
> > -    request: getSelectedRequest(state),
> >    }),
> 
> Do we still need this empty function (we could replace it with null, or add
> a comment to explain why we keep it)
Removed

> ::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:269
> (Diff revision 6)
> > +   * @param {string} id request id
> > +   * @return {boolean} return whether a specific networkEvent has been updated completely.
> > +   */
> > +  isQueuePayloadReady(id) {
> > +    let queuedPayload = this.getPayloadFromQueue(id);
> > +    return queuedPayload && queuedPayload.payload.eventTimings;
> 
> Could we add a TODO which say that we should find a better solution since it
> might happen that eventTimings is not the last update ?
Done

> ::: devtools/client/webconsole/new-console-output/reducers/ui.js:19
> (Diff revision 2)
> >  
> >  const UiState = Immutable.Record({
> >    filterBarVisible: false,
> >    filteredMessageVisible: false,
> >    timestampsVisible: true,
> > +  networkMessageActiveTabId: "headers",
> 
> this should use the new constant PANELS.HEADERS , I'm not sure if there this
> change somewhere
Done


Try green:
treeherder.mozilla.org/#/jobs?repo=try&revision=968d0a5c6f9b0da5468aa6884a8b46fd32852bcb

Working on tests.

Honza
Comment on attachment 8902305 [details]
Bug 1362036 - Implement tests;

https://reviewboard.mozilla.org/r/173830/#review179386

Some minor comments but I think everything is fine overall. r+ with the comments adressed :)

::: devtools/client/webconsole/new-console-output/new-console-output-wrapper.js:239
(Diff revision 1)
> +    // Fire an event indicating that all data fetched from
> +    // the backend has been received. This is based on
> +    // 'FirefoxDataProvider.isQueuePayloadReady', see more
> +    // comments in that method.
> +    // (netmonitor/src/connector/firefox-data-provider).
> +    this.jsterm.hud.emit("network-request-updated", {id, data});

Maybe we should fire a distinct event ? something like network-request-queue-payload-ready ?
I fear that firing the same event for different purpose will cause trouble in the future.

::: devtools/client/webconsole/new-console-output/test/components/network-event-message.test.js:120
(Diff revision 1)
> +      const message = stubPreparedMessages.get("XHR POST request");
> +      const update = stubPreparedMessages.get("XHR POST request update");
> +      const wrapper = render(NetworkEventMessage({
> +        message,
> +        serviceContainer,
> +        networkMessageUpdate: update,

does the message needs to have the update to be expandable ? If so we could test that network messages without updates are not expandable too.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_expand.js:6
(Diff revision 1)
> +const TEST_URI = "data:text/html;charset=utf8,Test that clicking on a network message " +
> +                 "in the console opens the netmonitor panel.";

s/opens the netmonitor panel/toggle the HTTP inspection.

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_expand.js:33
(Diff revision 1)
> +  await Promise.all([
> +    waitForNetworkUpdates(toolbox),
> +    testNetworkMessage(toolbox, hud, documentUrl)
> +  ]);

I may be wrong, but I think the goal here is to wait for network-request-updated so we know we have everything and we can expand the http calls.

If it is what we want, I think we could split this in 2 separate await, because here we run things in parallel, and it might occurs that the network-request-updated event isn't fired before we get into testNetworkMessage, which would cause intermittent

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_expand.js:39
(Diff revision 1)
> +    waitForNetworkUpdates(toolbox),
> +    testNetworkMessage(toolbox, hud, documentUrl)
> +  ]);
> +});
> +
> +async function testNetworkMessage(toolbox, hud, url) {

I think `toolbox` isn't used in the function we can remove it

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_openinnet.js:1
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +"use strict";
> +
> +const TEST_URI = "data:text/html;charset=utf8,Test that clicking on a network message " +
> +                 "in the console opens the netmonitor panel.";
> +
> +const TEST_FILE = "test-network-request.html";

Is this file  is browser_webconsole_network_messages_click.js renamed ? 
Not sure if reviewboard hides this, but it should be renamed with `hg mv` so we keep the history

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_openinnet.js:6
(Diff revision 1)
> +const TEST_URI = "data:text/html;charset=utf8,Test that clicking on a network message " +
> +                 "in the console opens the netmonitor panel.";

Say that it tests the context menu entry

::: devtools/client/webconsole/new-console-output/test/mochitest/browser_webconsole_network_messages_openinnet.js:53
(Diff revision 1)
> +  let onNetmonitorSelected = new Promise((resolve) => {
> +    toolbox.once("netmonitor-selected", (event, panel) => {
> +      resolve(panel);
> +    });

I think we can directly use toolbox.once without wrapping it in a promise, since it already returns a promise http://searchfox.org/mozilla-central/source/devtools/shared/event-emitter.js#117

::: devtools/client/webconsole/new-console-output/test/store/network-messages.test.js:17
(Diff revision 1)
> +} = require("devtools/client/webconsole/new-console-output/test/helpers");
> +const { stubPackets } = require("devtools/client/webconsole/new-console-output/test/fixtures/stubs/index");
> +
> +const expect = require("expect");
> +
> +describe.only("Network message reducer:", () => {

Remove the `only` :)

::: devtools/client/webconsole/package.json:37
(Diff revision 1)
>      "react-dom": "=15.3.2",
>      "react-redux": "=5.0.3",
>      "redux": "^3.6.0",
>      "require-hacker": "^2.1.4",
> -    "sinon": "^1.17.5"
> +    "sinon": "^1.17.5",
> +    "webpack": "^2.2.0"

why is this needed ?
Attachment #8902305 - Flags: review?(nchevobbe) → review+
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #35)
> Maybe we should fire a distinct event ? something like
> network-request-queue-payload-ready ?
Renamed to `network-request-payload-ready`

> does the message needs to have the update to be expandable ?
No, removed


> s/opens the netmonitor panel/toggle the HTTP inspection.
Done

> If it is what we want, I think we could split this in 2 separate await,
Done

> > +async function testNetworkMessage(toolbox, hud, url) {
> 
> I think `toolbox` isn't used in the function we can remove it
Done


> Is this file  is browser_webconsole_network_messages_click.js renamed ? 
> Not sure if reviewboard hides this, but it should be renamed with `hg mv` so
> we keep the history
Done

> > +const TEST_URI = "data:text/html;charset=utf8,Test that clicking on a network message " +
> > +                 "in the console opens the netmonitor panel.";
> 
> Say that it tests the context menu entry
Done

> devtools/client/webconsole/new-console-output/test/mochitest/

> I think we can directly use toolbox.once without wrapping it in a promise,
> since it already returns a promise
> http://searchfox.org/mozilla-central/source/devtools/shared/event-emitter.
> js#117
Done
 
> Remove the `only` :)
Done

> why is this needed ?
Removed

Thanks for the review,
Honza
Comment on attachment 8902603 [details]
Bug 1362036 - Generate stubs;

https://reviewboard.mozilla.org/r/174220/#review179426
Attachment #8902603 - Flags: review?(nchevobbe) → review+
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
ASAN failures like https://treeherder.mozilla.org/logviewer.html#?job_id=127145612&repo=autoland seem to have started around when this landed. Backing these patches out to see if they go away.
Flags: needinfo?(odvarko)
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d2c77bb9a4ce
Backed out 5 changesets for being the likely cause of asan leaks a=backout CLOSED TREE
Lots of "INFO - GECKO(14272) | JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/netmonitor/src/connector/firefox-connector.js, line 74: TypeError: this.tabTarget is null" spam in the logs during the netmonitor tests too.
This backout didn't fix things, so feel free to reland this whenever you're ready.
Flags: needinfo?(odvarko)
Depends on: 1395806
Depends on: 1395800
Depends on: 1395759
Investigated the bug on 57.0a1 (2017-09-15), using Windows 10 x64, Ubuntu 16.04 x64 and macOS 10.12.6 and found some potential issues:
- the Response tab is not horizontally scrollable 
- in the Headers, Params and Security tabs, the entries appear that can be edited when they are selected
- the links from the the Headers tab can be accessed only one time (the user have to switch to another tab and then to came back in order to be able to access them again) 
  - see the screencast for the last 2 mentioned issues https://goo.gl/kdtrUY
- in the Timings tab, the Learn More corresponding link covers the entire line, not juts the button
- hovering the links from the Stack Trace tab states "View source in Debugger", but clicking then nothing happens
Any thoughts about these?
Flags: needinfo?(odvarko)
(In reply to Iulia Cristescu, QA [:JuliaC] (PTO until September 25) from comment #52)
> Investigated the bug on 57.0a1 (2017-09-15), using Windows 10 x64, Ubuntu
> 16.04 x64 and macOS 10.12.6 and found some potential issues:
> - the Response tab is not horizontally scrollable 
This should be covered by bug 1391329

> - in the Headers, Params and Security tabs, the entries appear that can be
> edited when they are selected
This is a feature allowing to copy value (into clipboard) easily.

> - the links from the the Headers tab can be accessed only one time (the user
> have to switch to another tab and then to came back in order to be able to
> access them again) 
Can you please file a bug for this one.

>   - see the screencast for the last 2 mentioned issues https://goo.gl/kdtrUY
> - in the Timings tab, the Learn More corresponding link covers the entire
> line, not juts the button
Also needs a report.

> - hovering the links from the Stack Trace tab states "View source in
> Debugger", but clicking then nothing happens
> Any thoughts about these?
Yep, needs a bug report.

You can Cc me on all of those reports.

Thanks for the testing!

Honza
Flags: needinfo?(odvarko)
(In reply to Jan Honza Odvarko [:Honza] from comment #53)
> (In reply to Iulia Cristescu, QA [:JuliaC] (PTO until September 25) from
> comment #52)

> 
> > - the links from the the Headers tab can be accessed only one time (the user
> > have to switch to another tab and then to came back in order to be able to
> > access them again) 
> Can you please file a bug for this one.
> 

I realized that this is a result of the feature allowing to copy a value (into clipboard) easily (you mentioned it in the previous comment). Also I noticed that the links can be easily accessed again if the user selects another line and then comes back. So my fault, sorry for that!

> >   - see the screencast for the last 2 mentioned issues https://goo.gl/kdtrUY
> > - in the Timings tab, the Learn More corresponding link covers the entire
> > line, not juts the button
> Also needs a report.
> 

Logged bug 1403883 for this. 

> > - hovering the links from the Stack Trace tab states "View source in
> > Debugger", but clicking then nothing happens
> > Any thoughts about these?
> Yep, needs a bug report.
> 

Logged bug 1403898 for this. 

> You can Cc me on all of those reports.
> 

Done!

> Thanks for the testing!
> 
> Honza
Marking this as verified fixed on Fx 57 build. Bug 1403883 will be assessed separately.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
Flags: qe-verify+
Depends on: 1417805
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: