1362036 - Implement http inspection in new console

Status: VERIFIED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 4

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

Attachment: httpi.png

Some CSS needed, but this is the current status.

Honza

Comment 5

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

Attachment: Bug 1362036 - Implement http inspection in new console;

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

Comment 6

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

Current patch wip (not ready for review yet)

Honza

Comment 7

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

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

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

Comment 8

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

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

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

Comment 9

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

The patch is ready for review.

I am working on a test now.

Honza

Comment 10

[Tim Nguyen :ntim]

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.

Comment 11

[Tim Nguyen :ntim]

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.

Comment 12

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

Thanks for looking at the patch Tim.
Good points, I am working on it.
Honza

Comment 13

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

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

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

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

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 ?

Comment 15

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

Attachment: Bug 1362036 - Remember active network tab id;

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

Comment 16

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

Attachment: Bug 1362036 - Open in network panel;

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

Comment 17

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

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

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

Comment 18

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 19

[Nicolas Chevobbe [:nchevobbe]]

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)

Comment 20

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

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

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

Comment 21

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

Comment on attachment 8900670 [details]
Bug 1362036 - Remember active network tab id;

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

Comment 22

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

Comment on attachment 8900671 [details]
Bug 1362036 - Open in network panel;

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

Comment 23

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

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 24

[Nicolas Chevobbe [:nchevobbe]]

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)

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

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 26

[Nicolas Chevobbe [:nchevobbe]]

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

Comment 27

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

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

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

Comment 28

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

Comment on attachment 8900670 [details]
Bug 1362036 - Remember active network tab id;

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

Comment 29

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

Comment on attachment 8900671 [details]
Bug 1362036 - Open in network panel;

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

Comment 30

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

> ::: 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 31

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

Attachment: Bug 1362036 - Implement tests;

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

Comment 32

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

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

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

Comment 33

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

Comment on attachment 8900670 [details]
Bug 1362036 - Remember active network tab id;

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

Comment 34

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

Comment on attachment 8900671 [details]
Bug 1362036 - Open in network panel;

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

Comment 35

[Nicolas Chevobbe [:nchevobbe]]

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 ?

Comment 36

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

(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 37

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

Attachment: Bug 1362036 - Generate stubs;

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

Comment 38

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

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

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

Comment 39

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8902603 [details]
Bug 1362036 - Generate stubs;

https://reviewboard.mozilla.org/r/174220/#review179426

Comment 40

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

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

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

Comment 41

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

Comment on attachment 8900670 [details]
Bug 1362036 - Remember active network tab id;

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

Comment 42

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

Comment on attachment 8900671 [details]
Bug 1362036 - Open in network panel;

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

Comment 43

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

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

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

Comment 44

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

Comment on attachment 8902603 [details]
Bug 1362036 - Generate stubs;

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

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0be2eaa3a35
Implement http inspection in new console; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/16c55731558c
Remember active network tab id; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/f0c72c9d2ae1
Open in network panel; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/efab54a746d2
Implement tests; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/d00a28769dbe
Generate stubs; r=nchevobbe

Comment 46

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

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.

Comment 47

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2c77bb9a4ce
Backed out 5 changesets for being the likely cause of asan leaks a=backout CLOSED TREE

Comment 48

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 49

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

This backout didn't fix things, so feel free to reland this whenever you're ready.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b46539446dc
Implement http inspection in new console; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/fb65bb381a43
Remember active network tab id; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/81ee90df6129
Open in network panel; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/e34f2132d15a
Implement tests; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/c93c993bf569
Generate stubs; r=nchevobbe

Comment 51

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5b46539446dc
https://hg.mozilla.org/mozilla-central/rev/fb65bb381a43
https://hg.mozilla.org/mozilla-central/rev/81ee90df6129
https://hg.mozilla.org/mozilla-central/rev/e34f2132d15a
https://hg.mozilla.org/mozilla-central/rev/c93c993bf569

Comment 52

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

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?

Comment 53

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

(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

Comment 54

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

(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

Comment 55

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Marking this as verified fixed on Fx 57 build. Bug 1403883 will be assessed separately.