Closed
Bug 1362036
Opened 8 years ago
Closed 7 years ago
Implement http inspection in new console
Categories
(DevTools :: Console, enhancement, P1)
DevTools
Console
Tracking
(firefox57 verified)
Tracking | Status | |
---|---|---|
firefox57 | --- | verified |
People
(Reporter: bgrins, Assigned: Honza)
References
(Depends on 1 open bug)
Details
(Whiteboard: [reserve-console-html] )
Attachments
(6 files)
(deleted),
image/png
|
Details | |
(deleted),
text/x-review-board-request
|
nchevobbe
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
nchevobbe
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
nchevobbe
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
nchevobbe
:
review+
|
Details |
(deleted),
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.
Reporter | ||
Updated•8 years ago
|
Updated•8 years ago
|
Flags: qe-verify?
Updated•7 years ago
|
Flags: qe-verify? → qe-verify+
QA Contact: iulia.cristescu
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Updated•7 years ago
|
Iteration: --- → 57.1 - Aug 15
Priority: P3 → P1
Updated•7 years ago
|
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29
Reporter | ||
Comment 2•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
Thanks for looking at the patch Tim. Good points, I am working on it. Honza
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•7 years ago
|
||
mozreview-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 19•7 years ago
|
||
mozreview-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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
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•7 years ago
|
||
mozreview-review |
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 25•7 years ago
|
||
mozreview-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 26•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
> ::: 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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 35•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 36•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8902603 [details] Bug 1362036 - Generate stubs; https://reviewboard.mozilla.org/r/174220/#review179426
Attachment #8902603 -
Flags: review?(nchevobbe) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 45•7 years ago
|
||
Pushed by jodvarko@mozilla.com: 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
Updated•7 years ago
|
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)
Comment 47•7 years ago
|
||
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
Comment 48•7 years ago
|
||
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.
Comment 50•7 years ago
|
||
Pushed by jodvarko@mozilla.com: 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
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(odvarko)
![]() |
||
Comment 51•7 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 52•7 years ago
|
||
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)
Assignee | ||
Comment 53•7 years ago
|
||
(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)
Comment 54•7 years ago
|
||
(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•7 years ago
|
||
Marking this as verified fixed on Fx 57 build. Bug 1403883 will be assessed separately.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•