This is a breakdown task for bug 1308450 in order to integrate HTTP inspector into the Net panel. - Implement Params Panel

I played with current (master branch) params panel several times and found that it's buggy. * When you click a request very quickly after netmonitor opened, error message shows up in browser toolbox `TypeError: can't access dead object` in actors/tab.js:1656:1 (it's probably a bug in tab.js because it happened for all panels) * Query string sometimes doesn't appear when switching to different requests * Searchbox issue which doesn't update the search result after switching to another request. I made a testing jsbin site: https://jsbin.com/vamobuvoke/1/edit?html,output

WIP patch has uploaded and there is still buggy and some features haven't supported yet. * header panel renders incorrectly * scroll doesn't support * form data sometimes doesn't show properly, the editor appears instead.

After applying scrollbar on tree table itself, the layout in CodeMirror editor is broken. I suspect that issue probably because I created a html:div and a XUL iframe within a parent html:div to render request payload editor. My current approach to support scrollbar is to set position: absolute; and fix top, right, bottom, left position in `.detailsTreeView .treeTable` (It's bad I know, but I can't figure out other better way to do that), and after then the editor iframe looked weird somehow. Please apply my patch and open https://jsbin.com/vamobuvoke/1/edit?html,output with netmonitor params panel and click xxx?view=request-payload request to reproduce the issue.

Change ni? to r? flag. I think it's time to ask the first round review because most of features are completed.

* Used TreeView component * Fixed existing bug in searchbox which doesn't update the search result after switching to another request. * Fixed header panel renders empty data when user first time click a request. * Support scrollbar for TreeView UI There are slightly differences in New UI and Old UI after importing TreeView component instead, but I think most of the features work great and I can't find out any usability regression after playing with it for a while. Please see comment 6 for layout problem.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100134 ::: devtools/client/netmonitor/details-view.js:99 (Diff revision 4) > * Initialization function, called when the network monitor is started. > */ > initialize: function (store) { > dumpn("Initializing the DetailsView"); > > + this.store = store; this.store is not needed ::: devtools/client/netmonitor/shared/components/params-panel.js:88 (Diff revision 4) > + > + // FIXME: Workaround for CodeMirror editor > + // Append the editor in this way isn't very performant because it > + // doesn't benefit from react Virtual DOM optimization. > + // In the ideal case, editor should be replaced by ReactCodemirror. > + NetMonitorView.editor("#react-params-tabpanel-hook") try load sourceeditor via `appendToLocalElement`, which load editor into HTML element instead of XUL element

Thanks Fred for pointing out there has a HTML source editor. The CSS layout issue still remains after applying HTML element instead of XUL, which needs to be addressed in another way.

(In reply to Ricky Chien [:rickychien] from comment #6) > Created attachment 8820162 [details] > broken-layout-xul-iframe-and-html-div.png > > After applying scrollbar on tree table itself, the layout in CodeMirror > editor is broken. I suspect that issue probably because I created a html:div > and a XUL iframe within a parent html:div to render request payload editor. > > My current approach to support scrollbar is to set position: absolute; and > fix top, right, bottom, left position in `.detailsTreeView .treeTable` (It's > bad I know, but I can't figure out other better way to do that), and after > then the editor iframe looked weird somehow. I came across this before. Unfortunately, using position: absolute; might be your best bet here (maybe Jarda has a better solution ?). I'm fine with position: absolute; as long as it has a TODO comment to get it removed once everything gets changed to HTML markup.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100196 ::: devtools/client/netmonitor/actions/filters.js:60 (Diff revision 6) > +function setParamsFilterText(text) { > + return { > + type: SET_PARAMS_FILTER_TEXT, > + text, > + }; > +} We'll probably want to keep these actions/reducers within the shared/ folder so we can actually make the components reusable within the console. ::: devtools/client/netmonitor/shared/components/params-panel.js:85 (Diff revision 6) > + object[jsonScopeNameStr] = json; > + } else { > + object[paramsPostPayloadStr] = null; > + > + // FIXME: Workaround for CodeMirror editor > + // Append the editor in this way isn't very performant because it Doesn't debugger-html have a component for the editor that we can reuse here? ::: devtools/client/netmonitor/shared/components/params-panel.js:100 (Diff revision 6) > + editorContainer.style.display = "block"; > + } > + } > + > + return div({ className: "detailsTreeView" }, > + SearchBox({ You'll probably want to move out the current default prop values from the search-box component. ::: devtools/client/themes/netmonitor.css:1156 (Diff revision 6) > > +.detailsTreeView .treeTable { > + display: block; > + position: absolute; > + overflow-y: auto; > + top: 48px; 48 looks like a magic number, can you avoid it (by setting position: relative; on the relevant parent el? Also, please add a TODO comment to get the hack removed.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100204

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100196 > Doesn't debugger-html have a component for the editor that we can reuse here? source-editor in debugger.html is a wrapper of CodeMirror https://github.com/devtools-html/debugger.html/blob/ad275f626c40f6e25e497b5520b8a8d168021673/src/utils/source-editor.js we had a similar wrapper of CodeMirror implementation in devtools http://searchfox.org/mozilla-central/source/devtools/client/sourceeditor/editor.js AFAIK, most of differences between them are extraKeys which defined a set of key shortcuts, but there may be more differences. In my opinion, it would introduce lots of works to ensure all features support if we decide to reuse it from debugger.html, but it still be able to create a shared source-editor react component which wraps sourceeditor/editor.js like we did in debugger.html.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100374 Posting first batch of my review comments. When a CodeMirror editor is supposed to be displayed, the UI is completely broken for me. I'll have a look at that in the morning, and I'll also try to find a solution to the ugly position:absolute hacks. There must be a better way to do that. My biggest concern right now is that the Params and Cookies tabs are very similar to each other, and yet the implementation is completely different. One uses TreeTable, the other uses Accordion component + custom InfoParams component. There are also significant differences how the two panels are styled. We need to agree on a common solution before proceeding with these two tabs. And Headers tab will be very similar, too. I think that both TreeTable and Accordion/InfoParams should work just fine if properly implemented and if attention is paid to details. I don't have a favorite. But I want all the panels to behave consistently and share code and styling. ::: devtools/client/netmonitor/request-utils.js:226 (Diff revision 6) > > /** > * Parse a url's query string into its components > * > * @param {string} query - query string of a url portion > - * @return {array} array of query params { name, value } > + * @return {object} object of query params { name, value } The original comment is right: the return value is an array of {name,value} objects. ::: devtools/client/netmonitor/shared/components/params-panel.js:47 (Diff revision 6) > + > + // FIXME: Workaround for CodeMirror editor > + // Append the editor in this way isn't very performant because it > + // doesn't benefit from react Virtual DOM optimization. > + // In the ideal case, editor should be replaced by ReactCodemirror. > + const editorContainer = $("#request-payload-editor"); You should not do this in the render() method. If you want to insert the editor element, do it in componentDidMount() method, and destroy the editor in componentWillUnmount(). See here how debugger.html works with CodeMirror editor in React: https://github.com/devtools-html/debugger.html/blob/master/src/components/Editor.js#L325 ::: devtools/client/netmonitor/shared/components/params-panel.js:59 (Diff revision 6) > + // Query String section > + if (query) { > + object[paramsQueryStringStr] = > + parseQueryString(query) > + .reduce((acc, { name, value }) => > + Object.assign(acc, { [name]: value }) nit: just do simple "acc[name] = value" here. ::: devtools/client/netmonitor/shared/components/params-panel.js:67 (Diff revision 6) > + // Form Data section > + if (formDataSections && formDataSections.length > 0) { > + object[paramsFormDataStr] = > + parseQueryString(formDataSections.join("")) > + .reduce((acc, { name, value }) => > + Object.assign(acc, { [name]: value }) Same here. ::: devtools/client/netmonitor/shared/components/params-panel.js:80 (Diff revision 6) > + } catch (ex) { > + // Continue regardless of parsing error > + } > + > + if (json) { > + object[jsonScopeNameStr] = json; For displaying a JSON object, a Rep should be used. Please investigate how hard it would be to use and object Rep here. ::: devtools/client/netmonitor/shared/components/params-panel.js:115 (Diff revision 6) > + width: "100%", > + }], > + decorator: { > + getRowClass, > + }, > + renderRow: renderRow.bind(null, paramsFilterText), nit: I think that props => renderRow(paramsFilterText, props) is much more readable. ::: devtools/client/netmonitor/shared/components/params-panel.js:140 (Diff revision 6) > + const sections = [ > + paramsQueryStringStr, > + paramsFormDataStr, > + jsonScopeNameStr, > + paramsPostPayloadStr, > + ]; nit: extract this constant array out of the function, so that it doesn't need to be recreated on every call. ::: devtools/client/netmonitor/shared/components/params-panel.js:147 (Diff revision 6) > + if (sections.includes(object.name)) { > + return "treeSection"; > + } nit: I'd use a simple ternary operator here. ::: devtools/client/netmonitor/shared/components/params-panel.js:199 (Diff revision 6) > + updateParamsFilter: (url) => { > + dispatch(Actions.setParamsFilterText(url)); > + }, two nits: - the property should be called setParamsFilterText, identical to the action creator name. Allows using bindActionCreators later. - remove the braces and do an one-line lambda: url => dispatch(...) ::: devtools/client/shared/components/tree/tree-header.js:83 (Diff revision 6) > - div({ className: visible ? "treeHeaderCellBox" : "" }, > - visible ? col.title : "" > - ) > + visible ? div({ className: "treeHeaderCellBox"}, > + col.title > + ) : "", What does this change do?

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100196 > source-editor in debugger.html is a wrapper of CodeMirror > > https://github.com/devtools-html/debugger.html/blob/ad275f626c40f6e25e497b5520b8a8d168021673/src/utils/source-editor.js > > we had a similar wrapper of CodeMirror implementation in devtools > > http://searchfox.org/mozilla-central/source/devtools/client/sourceeditor/editor.js > > AFAIK, most of differences between them are extraKeys which defined a set of key shortcuts, but there may be more differences. In my opinion, it would introduce lots of works to ensure all features support if we decide to reuse it from debugger.html, but it still be able to create a shared source-editor react component which wraps sourceeditor/editor.js like we did in debugger.html. debugger.html reuse the sourceeditor when it embeded into firefox. It also use `appendToLocalElement` to attach codemirror into debugger panel

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100196 > We'll probably want to keep these actions/reducers within the shared/ folder so we can actually make the components reusable within the console. That's a good question! I'd prefer to drop this change because it's not that easy to reuse the redux store between different projects. The way we construct the application state for netmonitor will be distinct from debugger.html. If we would like to share anything as many as possible, it would need to separate constants SET_PARAMS_FILTER_TEXT in shared/ folder and even keep selectors in shared/ as well. However, I'm still in favor of sharing UI components if possible but not redux logic.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100374 > nit: just do simple "acc[name] = value" here. `acc[name] = value` returns the value but not acc object itself. If we'd like to use `acc[name] = value`, the code would look like: ``` .reduce((acc, { name, value }) => { acc[name] = value; return acc; }, {}) ``` I'd prefer to stick with using `Object.assign(acc, { [name]: value })` since it's functional and that can be replaced by Object Rest/Spread syntax easily in the future. http://redux.js.org/docs/recipes/UsingObjectSpreadOperator.html > For displaying a JSON object, a Rep should be used. Please investigate how hard it would be to use and object Rep here. We can't invoke `Rep(json)` here directly. Instead, the right way to do that is in renderValue() like http://searchfox.org/mozilla-central/source/devtools/client/webconsole/net/components/response-tab.js#109 it used TreeView + Rep to render json object. I don't know what's benefit from using the Rep, TreeView component is able to render object pretty well. > What does this change do? I added a css rule ".detailsTreeView .treeTable tr:not(:last-child) td:not(:empty)" in netmonitor.css for adding border-bottom to every table row if they're not empty. (the table header might be empty if we don't pass header = true prop) However, I found that there always remain an empty and useless div within thead element even if you don't set header = true. In addition, the difficulty of adding border-bottom to non-empty td elements would increase if we don't move the `visible` condition out like this way.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100468 ::: devtools/client/netmonitor/shared/components/params-panel.js:10 (Diff revision 6) > +"use strict"; > + > +const { createFactory, DOM, PropTypes } = require("devtools/client/shared/vendor/react"); > +const { connect } = require("devtools/client/shared/vendor/react-redux"); > +const Editor = require("devtools/client/sourceeditor/editor"); > +const SearchBox = createFactory(require("devtools/client/shared/components/search-box")); please put all custom components declaration after `const { div, input } = DOM;`

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100578 ::: devtools/client/netmonitor/reducers/filters.js:32 (Diff revision 7) > }); > > const Filters = I.Record({ > types: new FilterTypes({ all: true }), > text: "", > + paramsFilterText: "", I wonder if the reducer/action stuff should be in shared/ as well. ::: devtools/client/netmonitor/shared/components/params-panel.js:152 (Diff revision 7) > + > + if (open && typeof value === "object") { > + return; > + } > + > + return Rep(Object.assign({}, props, { I wonder why we need reps for this ? ::: devtools/client/sourceeditor/codemirror/codemirror.bundle.js:1244 (Diff revision 7) > - if (found) > + if (found) { > gutterWrap.appendChild(elt("div", [found], "CodeMirror-gutter-elt", "left: " + > dims.gutterLeft[id] + "px; width: " + dims.gutterWidth[id] + "px")); > - } > + } > - } > + } > - } > + } > + } Is this intentional ? ::: devtools/client/themes/netmonitor.css:1156 (Diff revision 7) > > +.detailsTreeView .treeTable { > + display: block; > + overflow-y: auto; > + /* Minus 72px * 3 for toolbox height + tabpanel height + searchbox height */ > + max-height: calc(100vh - 72px); Nice. Have you checked if that affects scrolling somewhere else? Usually, when I come across XUL-HTML bugs, the whole viewport dimensions get bigger, so it might be worth checking if nothing else is broken. ::: devtools/client/themes/netmonitor.css:1160 (Diff revision 7) > + /* Minus 72px * 3 for toolbox height + tabpanel height + searchbox height */ > + max-height: calc(100vh - 72px); > +} > + > +.detailsTreeView .devtools-searchbox input { > + margin-right: 6px; margin-inline-end;

I don't really have anything to add to the patch, I think Jarda will have more useful comments :)

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100578 > Nice. Have you checked if that affects scrolling somewhere else? > > Usually, when I come across XUL-HTML bugs, the whole viewport dimensions get bigger, so it might be worth checking if nothing else is broken. After wrapping codemirror as a react component, XUL editor has gone away. HTML editor works perfectly with ```max-height: calc(100vh - 72px);```. I also checked RWD when toolbox shrinks into smaller size and, it looks great to me.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100606 I can't get the Editor to display anything - it only shows some "x" placeholder. I'll attach a screenshot in a followup comment. Also, the tests are not yet updated, are they? ::: devtools/client/netmonitor/shared/components/params-panel.js:31 (Diff revisions 6 - 7) > const paramsQueryStringStr = L10N.getStr("paramsQueryString"); > const paramsFormDataStr = L10N.getStr("paramsFormData"); > const jsonScopeNameStr = L10N.getStr("jsonScopeName"); > const paramsPostPayloadStr = L10N.getStr("paramsPostPayload"); > const paramsEmptyText = L10N.getStr("paramsEmptyText"); > +const sections = [ nit: constants like this usually have uppercase names: SECTION_NAMES, EXPANDED_NODES, ... ::: devtools/client/netmonitor/requests-menu-view.js:213 (Diff revision 7) > + const request = getRequestById(this.store.getState(), action.id); > > - let { responseContent, requestPostData } = action.data; > + if (!request) { At this time, it's not at all guaranteed that the request is already in the state. The addRequest and updateRequest action are batched, and are processed only after some timeout. This code drops a lot of updates. We'll need to find a better solution how to do this. ::: devtools/client/netmonitor/requests-menu-view.js:225 (Diff revision 7) > - let request = getRequestById(this.store.getState(), action.id); > let { text, encoding } = responseContent.content; > - if (request) { > - let { mimeType } = request; > + let { mimeType } = request; > > + if (mimeType) { Is this check necessary? At this time, after the "responseContent" update was processed by the reducer, mimeType should always have a valid string value - the reducer sets a "text/plain" default value. If it's undefined, it's definitely a bug. ::: devtools/client/netmonitor/shared/components/editor.js:56 (Diff revision 7) > + } > + > + return ( > + div({ className: "devtools-monospace" }, > + div({ > + id: "editor-mount", Use className instead of id - that allows creating multiple editor components without having elements with duplicate ids. ::: devtools/client/netmonitor/shared/components/params-panel.js:69 (Diff revision 7) > + , {}); > + } > + // Form Data section > + if (formDataSections && formDataSections.length > 0) { > + object[paramsFormDataStr] = > + parseQueryString(formDataSections.join("")) You shouldn't just join the parameters together. "formDataSections" is an array of independent x-www-form-urlencoded strings: [ "a=b&c=d", "e=f&g=h" ] and should be shown as two "form data" sections, one with properties "a","c", second with properties "e","g". Joining them leads to parsing "a=b&c=de=f&g=h", which is wrong. ::: devtools/client/netmonitor/shared/components/params-panel.js:152 (Diff revision 7) > + > + if (open && typeof value === "object") { > + return; > + } > + > + return Rep(Object.assign({}, props, { This code is showing Reps even for things that are not JS objects at all - for example, "Request payload" has a "null" rep next to it - makes no sense.

This is how the Params tab looks for me when I execute a POST request with x-www-form-urlencoded body. fetch("/", { method: "POST", headers: { "content-type":"application/x-www-form-urlencoded" }, body: "foo=bar&loo=mar\r\ngoo=kar" })

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100642 ::: devtools/client/netmonitor/requests-menu-view.js:212 (Diff revision 8) > this.store.dispatch(action).then(() => window.emit(EVENTS.REQUEST_ADDED, action.id)); > }, > > updateRequest: Task.async(function* (id, data) { > const action = Actions.updateRequest(id, data, true); > - yield this.store.dispatch(action); > + this.store.dispatch(action); It's important to wait until this action is really dispatched. That guarantees that the getRequestById that follows returns the correctly updated request object. Any reason why the "yield" was removed here? Adding the "yield" back solves the issue with lost updates.

(In reply to Ricky Chien [:rickychien] from comment #21) > `acc[name] = value` returns the value but not acc object itself. If we'd > like to use `acc[name] = value`, the code would look like: OK, I didn't notice that Object.assign also returns the result. > > For displaying a JSON object, a Rep should be used. Please investigate how hard it would be to use and object Rep here. > > We can't invoke `Rep(json)` here directly. Instead, the right way to do that > is in renderValue() like > > http://searchfox.org/mozilla-central/source/devtools/client/webconsole/net/ > components/response-tab.js#109 > > it used TreeView + Rep to render json object. The Rep displays arrays better, distinguishes between number and string values, formats null values... Using it in renderValue works reasonably well, but I see you removed it again in the latest version of the patch? > I don't know what's benefit from using the Rep, TreeView component is able > to render object pretty well. TreeView does the same task as a Rep, but not so well and there are differences in behavior. For example, execute this fetch with a JSON body: fetch("/", { method: "POST", body: JSON.stringify({a:1,b:{c:2,d:{e:3,f:{g:null}}}}) }) Now display the JSON object in the TreeView and try to expand the properties. See how the a,c,e values move to the right as the tree is expanded, although they don't need to? Reps don't have this bug.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100606 > You shouldn't just join the parameters together. "formDataSections" is an array of independent x-www-form-urlencoded strings: > > [ "a=b&c=d", "e=f&g=h" ] > > and should be shown as two "form data" sections, one with properties "a","c", second with properties "e","g". > > Joining them leads to parsing "a=b&c=de=f&g=h", which is wrong. Oh, that's a good point! Do you know is there any good way to set the same key in TreeView object? The way we build "form data" section in TreeView is to set `object[paramsFormDataStr]` but it doesn't support multiple identical keys.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review100606 > At this time, it's not at all guaranteed that the request is already in the state. The addRequest and updateRequest action are batched, and are processed only after some timeout. > > This code drops a lot of updates. We'll need to find a better solution how to do this. Yeah, this might be the reason why sometimes you see the "x" placeholder on request payload section. Are these networkUpdate event dispatched in a certain order? ex: requestHeaders packet will arrive before requestPostData?

(In reply to Ricky Chien [:rickychien] from comment #32) > Do you know is there any good way to set the same key in TreeView object? > The way we build "form data" section in TreeView is to set > `object[paramsFormDataStr]` but it doesn't support multiple identical keys. I was wondering why these "form data sections" are even there - the standard for x-www-form-urlencoded doesn't mention that multiple sections separated by newlines are allowed. I tracked the code back to bug 879782. And it seems that its main goal was to support headers (Content-Type and Content-Length) directly embedded in the POST body. No evidence that multiple sections like this: a=b&c=d e=f&g=h are ever sent by anyone. So, I think that the right solution is to show all the sections in one "Form data" group. Parse the sections one by one by parseQueryString, and merge all name/value pairs into one object. The drawback is that duplicate names will be shown only once. But as multiple sections are extremely rare (if they even exist at all), it should not be a problem. It's unfortunate that the TreeView API doesn't allow for duplicate keys - it shouldn't be constrained this way.

(In reply to Ricky Chien [:rickychien] from comment #33) > Yeah, this might be the reason why sometimes you see the "x" placeholder on > request payload section. Are these networkUpdate event dispatched in a > certain order? ex: requestHeaders packet will arrive before requestPostData? Yes, after I added the "yield" to the action dispatch, the editor started working :) The order of update events is guaranteed. The problem is that if you check the state right after the updateRequest action was dispatched, the state is not updated yet. The updateRequest actions are put into a queue and are dispatched as a batch after a timeout. That's why the dispatch() returns a promise that resolves only after the action has really dispatched.

Current WIP patch on comment 36 is not so well but I've migrated to Rep for rendering value in TreeView. I replaced input with Rep for all TreeView cells, that means Query string, Form data and JSON sections will display their values with Rep. There is a note about current patch: * As attachment, a long value extends sidebar width unexpectedly. white-space: nowrap doesn't help, I can't figure out how to fix that. * After migrating to Rep, the value cell isn't a selectable input field. It will not act like current VariablesView anymore, because the value cell of Rep is built with spa and it cannot be replaced by input element. * Although CSS styling has slightly different from the old params-panel, the usability doesn't change.

Issues I came across: * As attachment, a long value extends sidebar width unexpectedly. * input field TreeCell doesn't support * Request payload (editor) shows `x` instead of real data when the first request you click is a payload request. * Editor config doesn't work after using HTML editor. Ex: line numbers and cursor doesn't show up. * Many `TypeError: document.body is undefined` error messages show in browser toolbox console (probably due to HTML editor?).

Ah, the root cause of showing `x` instead of real data is due to a sourceeditor bug. const SourceEditor = require("devtools/client/sourceeditor/editor"); componentDidMount() { this.editor = new SourceEditor({ lineNumbers: true, readOnly: true, value: text, }); } In my previous patch, I setup the initial value for editor like above way. The value props isn't correct that causes weird string appears on editor. (I copied the code from debugger.html, so that might be due to two different versions of CodeMirror) Invoke this.editor.setText(this.props.text); can solve the problem perfectly.

Another editor issue I just found: 1. perform a POST request with multiline text body: fetch("/api", { method: "POST", body: "body\n".repeat(5) }) 2. the "Request payload" section shows only the first line ("body") Also, it would be nice if the "Request payload" section was also collapsible - every other section is. I'm getting a lot of "TypeError: document.body is undefined", too. I'm not sure how much reviewing I'll be able to do in the following days - probably not much. I'll be back on January 3.

Note: The way to make a span looks like an input field http://codepen.io/anon/pen/KNYJvp

* Fixed the issue when displaying multiline text body * Fixed TypeError: document.body is undefined * Editor config works now, line number and cursor appear as expected. The root causes of above issues are due to CodeMirror APIs in sourceeditor/editor.js. Issues are addressed after using editor.appendTo() instead of editor.appendToLocalElement(). Wrap a iframe outside of CodeMirror element can render the editor correctly, but I don't know the root cause for why editor.appendToLocalElement() doesn't work :( Remaining issues: * As attachment, a long value extends sidebar width unexpectedly. * input field TreeCell doesn't support * Sometimes Data Form section shows as Editor rather than TreeView due to JSON parsing error.

I ended up coming up with a nice solution for mimicking VariablesView.jsm and there are other issues fixed, so I summarized them as follows: 
 * Used Rep for rendering tree cell values where the sections are key-value pairs such as query string section, form data string section or JSON section. * Mimics the TreeView styling to look like VariablesView. * Modified TreeView to support input field switching. * It’s a good approach to display and even select/copy longer string in TreeView cells. * This is a mechanism using in VariablesView.jsm as well, the TreeView cells are labels by default but they are able to switch from label to textbox when clicking on. I done this with the same way to switch Rep to input perfectly. * Good news!!! It fixed the issue which sidebar width will be extended unexpectedly by a long string. * Figured out a way to dispatch formDataSections. We don’t fetch formDataSections within updateRequest anymore. Instead, we subscribed a store listener to watch requestHeaders, requestHeadersFromUploadStream and requestPostData packets arrived in RequestsMenuView. * Converted updateRequest action to batch action for guaranteeing all actions are dispatched asynchronously in a sequential order. * Fixed the issue that form data section sometimes render as an editor instead of TreeView key-value cells. * Fixed the issue that sometimes form data section shows as Editor rather than TreeView due to JSON parsing error. * Wrapped editor.js as a react component to hide non-react style code when using in panel component. * Editor.js appendTo() support HTML iframe when the passing target is a HTML element, otherwise fall back to XUL iframe. * appendToLocalElement() is buggy and config doesn’t take any effect. * Editor component will keep a defer editor instance to ensure that we are able to get the editor instance properly even if componentDidUpdate() / componentWillUnmount() are invoked too quickly. * This fixed too many issues when you navigate different requests along with param panel opened. * It’s somehow hard to debug because it’s unpredictable and also affects CodeMirror behaviors. * SearchBox support number filtering. * Using onFilter instead of renderRow to filter tree data. * Fixed CodeMirror indent display issue. * Code style improvement. * Fixed mochitest failures Known tiny issues: * Color of Rep string is slightly different than the string color in VariablesView. * Should we change to new orange color which is the default color in TreeView or keep it as is (overwrite string color to white but don’t affect to other Rep types)? * Rep string in TreeCell doesn’t expend really well as XUL label since it is set to a fixed length by cropLimit: 60. * XUL label supports attribute crop=“center” which can extend the width to fill parent element pretty well. In HTML, it’s hard to implement this feature without writing Javascript code. * No support for keypress “enter” on a tree row to enter input field mode. * VariablesView is able to focus on a tree row when user selects a label and then press “enter” to enter a input (textbox) mode. It’s a nice to have feature. IMO, it makes sense to TreeView to support this feature in the near future.
 Most of the features / issues are completed. And I also learned a lots when addressing these unpredictable issues network packets update as well as react life cycle. It’s time to ask next round review to find out potential issues not easy to detect.

 Following link is my testing site for testing various views in ParamsPanel (Query String, Form Data, JSON View, Request Payload) https://rickychien.github.io/testing-site/ I haven't fixed the broken test cases, I'm going to fix that after second review.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review101630 ::: devtools/client/netmonitor/shared/components/params-panel.js:51 (Diff revision 12) > + */ > +function ParamsPanel({ > + query, > + postData, > + formDataSections, > + setParamsFilterText, follow a consistent order like alphabet or the params order you passed from connect() ::: devtools/client/netmonitor/shared/components/params-panel.js:66 (Diff revision 12) > + let json; > + > + // Query String section > + if (query) { > + object[paramsQueryStringStr] = > + parseQueryString(query) we can reuse the reduce part as `arrayToDict` function in shared/components/utils.js, shared with cookiesPanel ::: devtools/client/netmonitor/shared/components/params-panel.js:75 (Diff revision 12) > + } > + // Form Data section > + if (formDataSections && formDataSections.length > 0) { > + object[paramsFormDataStr] = > + parseQueryString(formDataSections.join("&")) > + .reduce((acc, { name, value }) => same here ::: devtools/client/netmonitor/shared/components/params-panel.js:85 (Diff revision 12) > + // Request payload section > + if (formDataSections && formDataSections.length === 0 && postData) { > + try { > + json = JSON.parse(postData); > + } catch (ex) { > + // Continue regardless of parsing error It's still nice to print some message here for debugging purose ::: devtools/client/netmonitor/shared/components/params-panel.js:100 (Diff revision 12) > + } > + > + return ( > + div({ className: "detailsTreeView" }, > + SearchBox({ > + delay: FILTER_SEARCH_DELAY, order in alphabet ::: devtools/client/netmonitor/shared/components/params-panel.js:131 (Diff revision 12) > +} > + > +ParamsPanel.displayName = "ParamsPanel"; > + > +ParamsPanel.propTypes = { > + query: PropTypes.string, alphabet order

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review101630 > we can reuse the reduce part as `arrayToDict` function in shared/components/utils.js, shared with cookiesPanel After offline discussion, we think that extracting reduce() logic to arrayToDict is not that useful because this part can be replaced by Object spread / rest operator once we move codebase to github and adapt babel to transpile ES7 syntax. > It's still nice to print some message here for debugging purose I still think that is a correct logic in this case because there is no other elegant way to validate a JSON string without surrounding a try catch. As a result, in order to not spam console output, we should ignore parse error here.

friendly remind to rebase the code since bug 1323933 is landed

Test failures are fixed.

I added a helper function waitForDOM() in framework/test/shared-head.js to observe DOM change for testing react + redux. I ran into a trouble when writing mochitest for react + redux architecture. Actor packets will update asynchronously and then follow by react component re-render, it causes that querySelector always returns null since mochitest runs faster and starts to query elements before component re-render. In this mechanism, I utilized the mutation observer to watch DOM event change for target element, and it is able to detect node event like additions, deletions as well as attribute changes. Everything works great, so maybe it's worth to share to the others who are writing mochitest to test react + redux.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel Fred, could you apply my patch and give me any feedback on UI/UX changes? I think almost of the works are complete and we can find out any bug / usability regression after introducing TreeView component.

I've applied the patch and haven't find major visual issues yet. One issue I found is when user 1. click the value field and the filed will turns to input field 2. click else where(ex the name part) the ui wont changed back, only click other values the previous one will changed back to text view

(In reply to Fred Lin [:gasolin] from comment #60) > I've applied the patch and haven't find major visual issues yet. > > One issue I found is > > when user > 1. click the value field and the filed will turns to input field > 2. click else where(ex the name part) the ui wont changed back, > only click other values the previous one will changed back to text view It's expected behavior that focus will not be changed if you don't focus on another focusable element. Played with old params panel and you can see the same result as well.

New update with adding collapsible button in post request data. I moved editor to renderRow() to display CodeMirror if the post data appears. In that way we can benefit from TreeView and support collapsible feature.

(In reply to Ricky Chien [:rickychien] from comment #58) > I added a helper function waitForDOM() in framework/test/shared-head.js to > observe DOM change for testing react + redux. I ran into a trouble when > writing mochitest for react + redux architecture. A better long-term solution is to avoid testing rendered DOM nodes in a mochitest. DOM rendering should be tested in an Enzyme unit test (i.e., test that given props and state lead to the expected DOM). Mochitests then only need to test the Redux state. The Redux state update is synchronous if the RequestsMenu.lazyUpdate flag is set to false. Future versions of React (Fiber) might make it even harder to test DOM reliably - they will divide the DOM update into several smaller batches and run them at different times. That could lead to a situation where the DOM observer reports a change at at time when the DOM update is not finished yet.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review102220 ::: devtools/client/netmonitor/test/browser_net_streaming-response.js:54 (Diff revision 29) > + yield panelWin.once(panelWin.EVENTS.TAB_UPDATED); > + wait = panelWin.once(panelWin.EVENTS.RESPONSE_BODY_DISPLAYED); > RequestsMenu.selectedIndex = 1; > yield panelWin.once(panelWin.EVENTS.TAB_UPDATED); > - yield panelWin.once(panelWin.EVENTS.RESPONSE_BODY_DISPLAYED); > + yield wait; I don't get this. Why are we waiting for two TAB_UPDATED events? I would understand if we needed to start waiting for the events before triggering a change: let onUpdated = once(UPDATED); let onDisplayed = once(DISPLAYED); doSomething(); yield onUpdated; yield onDisplayed; But this code is very confusing. nit: While touching this test, it would be nice to destructure the "EVENTS" variable and get rid of the verbose references to "panelWin.EVENTS". It would help if the test updates were in a separate microcommit. ::: devtools/client/sourceeditor/editor.js:261 (Diff revision 29) > - env = el.ownerDocument.createElementNS(XUL_NS, "iframe"); > - } > + let isXUL = el.toString() === "[object XULElement]"; > + > + env = el.ownerDocument.createElementNS(isXUL ? XUL_NS : HTML_NS, "iframe"); Use el.namespaceURI attribute to determine the namespace. This Editor fix could be landed as a separate bug, with appropriate explanation.

(In reply to Ricky Chien [:rickychien] from comment #46) > * Color of Rep string is slightly different than the string color in > VariablesView. > * Should we change to new orange color which is the default color in > TreeView or keep it as is (overwrite string color to white but don’t affect > to other Rep types)? I believe that the right style we should converge on is the one used in debugger.html and console.html, where objects are displayed using Reps. That's the style that is used in new components, and has been designed by Helen. Styling in TreeView and VariablesView is legacy and should be moved to the new one. > * XUL label supports attribute crop=“center” which can extend the width > to fill parent element pretty well. In HTML, it’s hard to implement this > feature without writing Javascript code. That's a common problem in the devtools.html refactoring project. Just use text-overflow:ellipsis and crop the labels either at the end, or at the start (using some RTL magic). Support for crop="center" will need to be added to platform (and the CSS standard) at some point. > https://rickychien.github.io/testing-site/ Thanks, this is very helpful for testing!

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review102330 ::: devtools/client/netmonitor/requests-menu-view.js:42 (Diff revision 29) > > // ms > const RESIZE_REFRESH_RATE = 50; > > // A smart store watcher to notify store changes as necessary > -function storeWatcher(initialValue, reduceValue, onChange) { > +function storeWatcher(initialValue, reduceValue, onChange, shouldUpdate) { You don't really need to extend the storeWatcher with a new "shouldUpdate" parameter to achieve your goal. Note that the function to compute the new value is called "reduceValue", and that it takes the previous value as a parameter. That's because it's supposed to act as a reducer. You can implement it like this: prevValue => { let newValue = computeNewValue(); return isEqual(prevValue, newValue) ? prevValue : newValue; } That is, do both the comparison and computation in one function. And like a Redux reducer, return the incoming state unchanged when you don't want to make a change. ::: devtools/client/netmonitor/shared/components/params-panel.js:99 (Diff revision 29) > + } else { > + postData = ""; > + } > + > + return ( > + div({ className: "networkDetails" }, The convention for CSS class names in Firefox is "network-details". That's used in Devtools, and also in Firefox frontend code in general. The "networkDetails" convention seems to be used in Firebug community. It should be converted to the Firefox standard, instead of being in conflict indefinitely.

Note: I cancel review request since I'm splitting this bug into smaller bugs / patches and land it incrementally. I'll keep update my patch here but please ignore any update notifications at this time.

I separated this big patch into three bugs (bug 1328498, bug 1328500, bug 1328532). I'd like to wait for all of them to be landed and later rebase m-c. I will split the rest of my patch here to make review more easier.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review103210 ::: devtools/client/netmonitor/shared/components/params-panel.js:19 (Diff revision 34) > +const Actions = require("../../actions/index"); > +const { getSelectedRequest } = require("../../selectors/index"); > +const { getUrlQuery, parseQueryString } = require("../../request-utils"); > + > +// Components > +const PropertiesView = createFactory(require("./properties-view")); PropertiesView file is missing ::: devtools/client/netmonitor/shared/components/params-panel.js:94 (Diff revision 34) > + } > + > + return ( > + PropertiesView({ > + object, > + filterText: paramsFilterText, though in all cases the search box is fixed and the treeView is scrollable, the Header panel have more content in the top region. It will be harder to make PropertiesView reused in Header panel. Therefore I suggest make the Searchbox and Treeview to separate parts to better support fix/scroll region

(In reply to Fred Lin [:gasolin] from comment #84) > Comment on attachment 8820118 [details] > Bug 1317650 - Implement Params Panel > > https://reviewboard.mozilla.org/r/99644/#review103210 > > ::: devtools/client/netmonitor/shared/components/params-panel.js:19 > (Diff revision 34) > > +const Actions = require("../../actions/index"); > > +const { getSelectedRequest } = require("../../selectors/index"); > > +const { getUrlQuery, parseQueryString } = require("../../request-utils"); > > + > > +// Components > > +const PropertiesView = createFactory(require("./properties-view")); > > PropertiesView file is missing Bug 1328828 > ::: devtools/client/netmonitor/shared/components/params-panel.js:94 > (Diff revision 34) > > + } > > + > > + return ( > > + PropertiesView({ > > + object, > > + filterText: paramsFilterText, > > though in all cases the search box is fixed and the treeView is scrollable, > the Header panel have more content in the top region. > It will be harder to make PropertiesView reused in Header panel. > > Therefore I suggest make the Searchbox and Treeview to separate parts to > better support fix/scroll region Having them in the same component doesn't really prevent you from doing the layout you want. There are 2 ways to achieve this: - Using `position: sticky` : https://jsfiddle.net/ntim/uL5b4ehp/ - Purely with flexbox: https://jsfiddle.net/ntim/tp3h1b3c/ Not sure if that will work in our XUL-HTML hybrid mixture, but I'm sure it's possible to work things out in CSS.

I'd like to cancel review for this bug since we are focusing on reviewing bug 1328828 at this time. This patch is unstable right now and is unable to be tested correctly without applying bug 1328828. Thus, you will run into an error `PropertiesView file is missing`.

(In reply to Fred Lin [:gasolin] from comment #84) > though in all cases the search box is fixed and the treeView is scrollable, > the Header panel have more content in the top region. > It will be harder to make PropertiesView reused in Header panel. > > Therefore I suggest make the Searchbox and Treeview to separate parts to > better support fix/scroll region Headers panel is able to reuse PropertiesView easily in this case. Headers panel has additional content but it doesn't relate to PropertiesView because PropertiesView will take the rest of the available space to render the scrollable TreeView along with a SearchBox. It's still reasonable to provide a SearchBox in PropertiesView since there are lots of cases (panels) used this SearchBox + TreeView features so that makes it worth to wrap them together into one component.

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review103678 This patch works reasonably well at this moment and I think it should be landed. There are two small and one big bug that I recommend to file as followups: 1. When I mouse over a section header, the text has an "underline" decoration. This is weird. It's a behavior of the TreeView component that can be observed also in DOM Panel and other usage sites. I think the idea behind this was to somehow indicate that an expandable tree item is "active", i.e., something happens after a click. However, I've never seen it before on any "expandable tree" UI. The expansion arrows are probably considered a sufficient affordance for expanding. I recommend removing this style - it just adds visual noise, in my opinion. 2. When the Params panel displays a JSON POST data in this format: { number: 123 } Then the expanded TreeView shows two rows - one "object", and one "number" under it. Now try to filter this on value "123". I expect to see the row with the "123" value. The actual result, however, is that the "123" row is collapsed, and only the "object" row is visible. This is confusing - it's unclear why the "object" row matches the searched value and why it's displayed. One of the requests on Ricky's test page (https://rickychien.github.io/testing-site/) can be used to reproduce this. 3. The big one: The iframe with the CodeMirror editor has fixed height 150px, although there is much more space that could be used to display the text rows. The editor should expand vertically to fill the available space. I believe that the issue #3 is unsolvable until we switch the whole Netmonitor from XUL to HTML. Until then, we need to use iframe to show the editor, and the height of the iframe can never automatically adapt to the contents. That's why I recommend the following plan: 1. Land this patch and the other tab panels ASAP, even with temporary quality regressions: I believe Nightly users can live with them for a few days. 2. Use a React component to switch between the tabs. 3. Now, all traces of XUL are finally gone. Convert netmonitor.xml to HTML. 4. Start fixing all the quality regressions - it should be much more easier now that we don't worry about XUL gotchas like flex layout etc.

(In reply to Jarda Snajdr [:jsnajdr] from comment #94) > Comment on attachment 8820118 [details] > Bug 1317650 - Implement Params Panel > > https://reviewboard.mozilla.org/r/99644/#review103678 > > This patch works reasonably well at this moment and I think it should be > landed. > > There are two small and one big bug that I recommend to file as followups: > > 1. When I mouse over a section header, the text has an "underline" > decoration. This is weird. It's a behavior of the TreeView component that > can be observed also in DOM Panel and other usage sites. I think the idea > behind this was to somehow indicate that an expandable tree item is > "active", i.e., something happens after a click. However, I've never seen > it before on any "expandable tree" UI. The expansion arrows are probably > considered a sufficient affordance for expanding. I recommend removing this > style - it just adds visual noise, in my opinion. I'll fix this small one before landing. > 2. When the Params panel displays a JSON POST data in this format: > > { number: 123 } > > Then the expanded TreeView shows two rows - one "object", and one "number" > under it. Now try to filter this on value "123". I expect to see the row > with the "123" value. The actual result, however, is that the "123" row is > collapsed, and only the "object" row is visible. This is confusing - it's > unclear why the "object" row matches the searched value and why it's > displayed. One of the requests on Ricky's test page > (https://rickychien.github.io/testing-site/) can be used to reproduce this. > > 3. The big one: The iframe with the CodeMirror editor has fixed height > 150px, although there is much more space that could be used to display the > text rows. The editor should expand vertically to fill the available space. > > I believe that the issue #3 is unsolvable until we switch the whole > Netmonitor from XUL to HTML. Until then, we need to use iframe to show the > editor, and the height of the iframe can never automatically adapt to the > contents. There is a separate bug 1329068 to address this issue which is branched from PropertiesView. Note that I'm not sure that issue is due to XUL, but it is an existing bug in old params panel and response panel. If old params panel's query string section accommodates too many properties which taller than available height, the scrollbar will show up. If source editor appends below at this time, source editor would be scrollable but query string section wouldn't.

(In reply to Jarda Snajdr [:jsnajdr] from comment #94) > 1. When I mouse over a section header, the text has an "underline" > decoration. This is weird. It's a behavior of the TreeView component that > can be observed also in DOM Panel and other usage sites. I think the idea > behind this was to somehow indicate that an expandable tree item is > "active", i.e., something happens after a click. However, I've never seen > it before on any "expandable tree" UI. The expansion arrows are probably > considered a sufficient affordance for expanding. I recommend removing this > style - it just adds visual noise, in my opinion. Ah, it's by design. DOM Panel has this "underline" decoration when the tree row is expandable. Find a variable like document in DOM panel to see how it looks. I think we should keep this look and feel as it is by using default style from TreeView. Now, DOM panel and JSON view both shared this style and we should stay consistent.

(In reply to Ricky Chien [:rickychien] from comment #95) > > 2. When the Params panel displays a JSON POST data in this format: > > > > { number: 123 } > > > > Then the expanded TreeView shows two rows - one "object", and one "number" > > under it. Now try to filter this on value "123". I expect to see the row > > with the "123" value. The actual result, however, is that the "123" row is > > collapsed, and only the "object" row is visible. This is confusing - it's > > unclear why the "object" row matches the searched value and why it's > > displayed. One of the requests on Ricky's test page > > (https://rickychien.github.io/testing-site/) can be used to reproduce this. Good catch! I think we could file a separate bug to deal with it. I'm expecting that could improve expandedNodes in properties-view.js. We can also reference the implementation of json-panel.js at http://searchfox.org/mozilla-central/source/devtools/client/jsonview/components/json-panel.js#66-86, so we are able to expand JSON object with a AUTO_EXPAND_MAX_LEVEL by default.

Updated my patch for addressing issue #2 in comment 94. As comment 97 mentioned, I copied getExpandedNodes() from json-panel.js directly and everything works perfectly. So I'd like to improve issue #2 here and stop filing a separate bug since it's not that hard to fix. I've triggered try test and wait for Honza's review.

Updated my patch for: * CodeMirror mimeType support * Hide SearchBox if there is no available properties except for editor config

Comment on attachment 8820118 [details] Bug 1317650 - Implement Params Panel https://reviewboard.mozilla.org/r/99644/#review103880 The code received many comments so, I am focusing on UI 1) Agree that the source editor should take all available vertical space. I also commeted this here: https://bugzilla.mozilla.org/show_bug.cgi?id=1328500#c13 It's ok if this is done in separate bug (is it bug bug 1329068?) 2) This might be minor but, if I set post data to: "100" (including the quotes), the JSON section is empty. Note that JSON.parse("100") returns 100 3) I am not seeing horizontal lines in-between the query string arguments (they were presented before). I'd actually prefere if these lines are *not* rendered, but we should keep the UI as is or at least be consistent between panels (e.g. Cookies, Headers and Params should be all consistent) None of these is a blocker so, R+ assuming try is green. Honza

Uploaded my patch along with fixing comment 104 issue 3. After discussing issue 2 with Honza on IRC, we'd like to file a separate bug for fixing this corner case since it's not easy to address.

Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/63836b06effb Implement Params Panel r=Honza,jsnajdr

I can confirm that Params Panel is working as expected on 53.0a2 (2017-01-25) using Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.11.6. Updating the flags accordingly.