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

add nightly screenshot for record

From cookies tab perspective: The components that reusable: net-info-group.js net-info-params.js spinner.js Need lite change: net-info-group.css net-info-params.css (need custom the look and feel because the styles are different) Need rewrite: cookie-tab, only skeleton can be reused (because the layout is different) no need net-info-group-list because the ui won't show request and response headers together Missing for netmonitor: * Need a container component to serve fixed content at the top and the scrollable content at bottom, which is reusable for cookies, headers and params tab. * Need add search filter component into above container component Once the UI style change is ready, I will move net-info-group.js net-info-params.js from webconsole into shared/components folder if it is really reusable.

WIP patch that migrated cookies tab from `webconsole/net/`, as described above the style is not handled yet, search filter is missing

While visiting a random page#1, I saw a request contains both request cookies and response cookies, is it possible, or is it a bug? #1: http://stackoverflow.com/questions/30295872/html-layout-with-only-part-of-the-page-scrolling

Comment on attachment 8814011 [details] request menu WIP https://reviewboard.mozilla.org/r/95302/#review96470 I like the patch, I think it's the right way to go. UI comments: - The list of cookies should be indentet - There should be a space between cookie name and value Honza ::: devtools/client/netmonitor/components/moz.build:6 (Diff revision 2) > # This Source Code Form is subject to the terms of the Mozilla Public > # License, v. 2.0. If a copy of the MPL was not distributed with this > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > +DIRS += [ > + 'net', So, we should use 'http-inspector' directory I guess. ::: devtools/client/netmonitor/details-view.js:452 (Diff revision 2) > * The message received from the server. > * @return object > * Returns a promise that resolves upon the adding of cookies. > */ > - _addCookies: Task.async(function* (name, response) { > - let cookiesScope = this._cookies.addScope(name); > + _addCookies: Task.async(function* (name, data) { > + let nameId = name === 'Request Cookies' ? 'requestCookies' : 'responseCookies'; Not sure if this is intended. It looks like the 'name' passed in is localized string (e.g. result of L10N.getStr("requestCookies")) Honza

(In reply to Fred Lin [:gasolin] from comment #6) > Created attachment 8814809 [details] > req+rsp cookies > > While visiting a random page#1, I saw a request contains both request > cookies and response cookies, is it possible, or is it a bug? Not a bug, it's valid use case. Cookies can be sent and received within the same HTTP round-trip. Honza

The new WIP is based on bug 1309866 request menu with redux patch, The improvements are * use redux connector to get free response/request cookies data * show response/request cookies with net-group-list * convert net-info-group-list, net-info-group-param to stateless component * use `dissector` instead of `net` for HTTP inspector components folder TODO: * wiring filter input with net-info-params * apply scrollable layout on cookies params section * fix styling

New WIP is rebased on inbound since bug 1309866 is landed \o/ The improvements are * Add the search box * wired filter input with net-info-params * use shared/components folder to place http-inspector components TODO: * ignore tail strings to keep same sidebar width * consider port debugger.html's accordin for net-group's accordin * apply scrollable layout on cookies params section * fix styling * unit tests

While wiring input box with params filter, we found a related bug: 1. select an item from request menu to display the sidebar, switch to cookies tab 2. put some word in input box, the params will be filtered with the word 3. select other item, the text in input box is remained, but the filter does not applied to new item's params list. (need extra enter key in filter box) It's been solved in current WIP patch.

(In reply to Fred Lin [:gasolin] from comment #14) > * consider port debugger.html's accordin for net-group's accordin I really like this idea, but note that we should focus on removing XUL not on changing UI/UX. So, while I am happy to have this I also think we should be careful about not increasing our work-scope too much. Btw. it looks like the patch is missing moz.build in the 'shared' directory. Honza

current WIP * ported accordion with current arrows(.png) and style(padding: 3px instead of 5px) from debugging.html * applied styles on params flaws: 1. The net-info-params behavior is now still different from XUL's. (XUL's param allow tap, edit, or copy the param value), we need support that for consistency. 2. The net-info-params does not break-word when the param value is too long, need fix it either

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review98798 Good job for cookies panel! Shared folder has introduced in TimingsPanel which has landed yesterday. Remember to rebase to latest m-c. On the other hand, it would be better if we replace accordion by TreeView with a customized styling. ::: devtools/client/netmonitor/actions/ui.js:10 (Diff revision 4) > + SET_COOKIES_FILTER_TEXT, > } = require("../constants"); > > /** > * Change sidebar open state. > * > * @param {boolean} open - open state > */ > function openSidebar(open) { > return { > type: OPEN_SIDEBAR, > open, > }; > } > > /** > * Toggle sidebar open state. > */ > function toggleSidebar() { > return (dispatch, getState) => dispatch(openSidebar(!getState().ui.sidebarOpen)); > } > > /** > * Waterfall width has changed (likely on window resize). Update the UI. > */ > function resizeWaterfall(width) { > return { > type: WATERFALL_RESIZE, > width > }; > } > > +/** > + * Set filter text in cookies tabpanel. > + * > + * @param {string} text - A filter text is going to be set > + */ > +function setCookiesFilterText(text) { > + return { > + type: SET_COOKIES_FILTER_TEXT, > + text, > + }; > +} > + > module.exports = { > openSidebar, > toggleSidebar, > resizeWaterfall, > + setCookiesFilterText, How about moving filter relevant logic to actions/filters.js? Feel free to modify the variable or method names in filters.js if you think they are too generic or there are some naming conflicts. ::: devtools/client/netmonitor/constants.js:8 (Diff revision 4) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > "use strict"; > > const general = { > - FREETEXT_FILTER_SEARCH_DELAY: 200, > + FILTER_SEARCH_DELAY: 200, Good catch! new name is better. ::: devtools/client/netmonitor/netmonitor.xul:10 (Diff revision 4) > +<?xml-stylesheet href="resource://devtools/client/netmonitor/shared/components/accordion.css" type="text/css"?> > +<?xml-stylesheet href="resource://devtools/client/netmonitor/shared/components/net-info-params.css" type="text/css"?> > + I highly recommended that do not add new code in netmonitor.xul if possible, these CSS files could be imported in netmonitor.css. I also did the same thing in Security Panel. https://reviewboard.mozilla.org/r/95764/diff/13#index_header ::: devtools/client/netmonitor/reducers/ui.js:11 (Diff revision 4) > + SET_COOKIES_FILTER_TEXT, > } = require("../constants"); > > const UI = I.Record({ > sidebarOpen: false, > waterfallWidth: 300, > + cookiesText: "", > }); > > function openSidebar(state, action) { > return state.set("sidebarOpen", action.open); > } > > // Safe bounds for waterfall width (px) > const REQUESTS_WATERFALL_SAFE_BOUNDS = 90; > > function resizeWaterfall(state, action) { > return state.set("waterfallWidth", action.width - REQUESTS_WATERFALL_SAFE_BOUNDS); > } > > function ui(state = new UI(), action) { > switch (action.type) { > case OPEN_SIDEBAR: > return openSidebar(state, action); > case WATERFALL_RESIZE: > return resizeWaterfall(state, action); > + case SET_COOKIES_FILTER_TEXT: > + return state.set("cookiesText", action.text); How about moving filter relevant logic to reducers/filters.js? Feel free to modify the variable or method names in filters.js if you think they are too generic or there are some naming conflicts. ::: devtools/client/netmonitor/reducers/ui.js:17 (Diff revision 4) > } = require("../constants"); > > const UI = I.Record({ > sidebarOpen: false, > waterfallWidth: 300, > + cookiesText: "", nit: how about cookiesFilterText? ::: devtools/client/netmonitor/shared/components/cookies-panel.js:7 (Diff revision 4) > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +"use strict"; > + > +const {createFactory, DOM, PropTypes} = require("devtools/client/shared/vendor/react"); nit: For long destructuring assignment: ```js const { createFactory, DOM, PropTypes, } = require("devtools/client/shared/vendor/react"); ``` ::: devtools/client/netmonitor/shared/components/cookies-panel.js:16 (Diff revision 4) > +const { L10N } = require("../../l10n"); > +const Actions = require("../../actions/index"); > +const { FILTER_SEARCH_DELAY } = require("../../constants"); > +const { connect } = require("devtools/client/shared/vendor/react-redux"); > + > +// Shortcuts nit: This code is simple enough to explain itself, so the shortcuts is useless. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:20 (Diff revision 4) > + > +// Shortcuts > +const {div} = DOM; > + > +/** > + * This template represents 'Cookies' tab displayed when the user nit: 'Cookies' tab -> 'Cookies' panel

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review98828 ::: devtools/client/netmonitor/details-view.js:113 (Diff revision 4) > this._headers = new VariablesView($("#all-headers"), > Heritage.extend(GENERIC_VARIABLES_VIEW_SETTINGS, { > emptyText: L10N.getStr("headersEmptyText"), > searchPlaceholder: L10N.getStr("headersFilterText") > })); > - this._cookies = new VariablesView($("#all-cookies"), > + this._cookies = new VariablesView($("#react-cookies-tabpanel-hook"), According to introduction comment for VariablesView.jsm ``` A tree view for inspecting scopes, objects and properties. ``` IMO, it makes more sense to use the existing TreeView component to replace the VariablesView since it's a kind of tree view as well. And this line should also be removed after adopting new implementation and say good bye to VariablesView.jsm. After details-view refactoring is completed, all jsm files including VariablesView.jsm and VariablesViewController.jsm shouldn't stay in codebase anymore.

WIP: * fix searchbox style * fix light and dark theme * addressed above comments TODO: * params didn't break correctly * apply scrollable layout on cookies params section * unit tests

WIP: * lint fix * remove net-info-params.css since there's not much of it * move cookie selector to selectors/requests as `getSelectedCookies` * lint fix The most elegant way to scrolling only parts of the page seems to be the flex layout. Though the sample works on jsbin https://jsfiddle.net/przemcio/xLhLuzf9/3/, it has issue on sidepannel with `moz-box-flex: 1;` rule, need more investigation

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99232 ::: devtools/client/netmonitor/netmonitor.xul:211 (Diff revision 6) > </tabpanel> > <tabpanel id="cookies-tabpanel" > class="tabpanel-content"> > <vbox flex="1"> > - <vbox id="all-cookies" flex="1"/> > + <html:div xmlns="http://www.w3.org/1999/xhtml" > + id="react-cookies-tabpanel-hook" flex="1"/> nit: remove flex="1" ::: devtools/client/netmonitor/reducers/filters.js:78 (Diff revision 6) > return state.set("types", toggleFilterType(state.types, action)); > case ENABLE_FILTER_TYPE_ONLY: > return state.set("types", enableFilterTypeOnly(state.types, action)); > case SET_FILTER_TEXT: > return state.set("text", action.text); > + case SET_COOKIES_FILTER_TEXT: After introducing a new cookies filter in filters.js, I think it makes sense to change the generic filter name to requestFilter since it would be more easy to distinguish. setFilterText -> setRequestFilterText toggleFilterType -> toggleRequestFilterType enableFilterTypeOnly -> enableRequestFilterTypeOnly FilterTypes -> RequestFilterTypes ... I know there are lots of names need to change, but I think it's reasonable to build a good code quality in early phase. ::: devtools/client/netmonitor/selectors/requests.js:112 (Diff revision 6) > } > > return getRequestById(state, state.requests.selectedId); > } > > +// use in cookies panel nit: I prefer to remove this comment since selector is supposed to be reused everywhere not only in cookies panel ::: devtools/client/netmonitor/selectors/requests.js:114 (Diff revision 6) > + if (!state.requests.selectedId) { > + return { > + response: {cookies: []}, > + request: {cookies: []}, > + }; > + } > + > + const selectedRequest = getRequestById(state, state.requests.selectedId); It's unnecessary to check selectedId here. There has a getSelectedRequest() to return a selectedRequest if it is existed, otherwise return null. ::: devtools/client/netmonitor/selectors/requests.js:124 (Diff revision 6) > + } > + > + const selectedRequest = getRequestById(state, state.requests.selectedId); > + > + let response = selectedRequest && selectedRequest.responseCookies ? > + selectedRequest.responseCookies : {cookies: []}; Do not return an empty data if possible. It would be fine if we simply return the result from selectedRequest.responseCookies, and then ensure that we render empty list of data properly in CookiesPanel component. Option 1: checking props and render empty UI https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/shared/components/security-panel.js#l26 Option 2: setting defaultProps for in CookiesPanel ::: devtools/client/netmonitor/shared/components/cookies-panel.js:19 (Diff revision 6) > +const NetInfoParams = createFactory(require("./net-info-params")); > +const { L10N } = require("../../l10n"); > +const Actions = require("../../actions/index"); > +const { FILTER_SEARCH_DELAY } = require("../../constants"); > +const { getSelectedCookies } = require('../../selectors/index'); > +const { connect } = require("devtools/client/shared/vendor/react-redux"); nit: list libraries at the beginning and then follow other resources in netmonitor folder. I recommended that put it after react xx = require("devtools/client/shared/vendor/react"); xx = require("devtools/client/shared/vendor/react-redux") ::: devtools/client/netmonitor/shared/components/cookies-panel.js:21 (Diff revision 6) > +const Actions = require("../../actions/index"); > +const { FILTER_SEARCH_DELAY } = require("../../constants"); > +const { getSelectedCookies } = require('../../selectors/index'); > +const { connect } = require("devtools/client/shared/vendor/react-redux"); > + > +const {div} = DOM; nit: ```js const { div } = DOM; ``` ::: devtools/client/netmonitor/shared/components/cookies-panel.js:28 (Diff revision 6) > +/** > + * This template represents 'Cookies' panel displayed when the user > + * tap the request menu item in the network panel. It's responsible for rendering > + * sent and received cookies. > + */ > +function CookiesPanel({data, cookiesFilterText, cookiesFilterHandler}) { nit: for more than one prop in functional component function CookiesPanel({ data, cookiesFilterText, cookiesFilterHandler, }) { ... } ::: devtools/client/netmonitor/shared/components/cookies-panel.js:80 (Diff revision 6) > +}; > + > +module.exports = connect( > + state => { > + let data = getSelectedCookies(state); > + // selectCookiesById nit: unused comments

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99256 ::: devtools/client/netmonitor/details-view.js (Diff revision 6) > - let cookiesScope = this._cookies.addScope(name); > - cookiesScope.expanded = true; > - > - for (let cookie of response.cookies) { > - let cookieVar = cookiesScope.addItem(cookie.name, {}, {relaxed: true}); > - let cookieValue = yield gNetwork.getString(cookie.value); I think you forgot to deal with long string. We don't fetch full text for each request at the beginning. Sometimes cookies value can be very long, invoking gNetwork.getString(cookie.value) to update cookies values for each request is necessary, so we can make sure that cookies panel renders long value correctly.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review98798 > How about moving filter relevant logic to reducers/filters.js? > > Feel free to modify the variable or method names in filters.js if you think they are too generic or there are some naming conflicts. Make sense since there will be more filters in sidebar panels. The current toolbar filter name in ` reducers/filters.js` seems too generic, we may need rename it in a separate bug.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99232 > After introducing a new cookies filter in filters.js, I think it makes sense to change the generic filter name to requestFilter since it would be more easy to distinguish. > > setFilterText -> setRequestFilterText > toggleFilterType -> toggleRequestFilterType > enableFilterTypeOnly -> enableRequestFilterTypeOnly > FilterTypes -> RequestFilterTypes > ... > > I know there are lots of names need to change, but I think it's reasonable to build a good code quality in early phase. Yeah it worth to create another Bug 1323933 so the patch diff will be easier to follow. > Do not return an empty data if possible. > > It would be fine if we simply return the result from selectedRequest.responseCookies, and then ensure that we render empty list of data properly in CookiesPanel component. > > Option 1: checking props and render empty UI > https://hg.mozilla.org/mozilla-central/file/tip/devtools/client/netmonitor/shared/components/security-panel.js#l26 > > Option 2: setting defaultProps for in CookiesPanel I'd just use getSelectedRequest from selector and move the rest back to cookies panel, untill we find other place use the same code

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99256 > I think you forgot to deal with long string. > > We don't fetch full text for each request at the beginning. Sometimes cookies value can be very long, invoking gNetwork.getString(cookie.value) to update cookies values for each request is necessary, so we can make sure that cookies panel renders long value correctly. It's intended. Since all tests passed on try run and I haven't find a case that have long cookies, I think it might worth to add another bug to deal with longCookies, ex: That bug need create a redux-thunk action to update long cookies and add related tests.

I think its time to ask for first round review to find out corner cases, * all try green * Accordion component comes from debugger.html https://github.com/devtools-html/debugger.html/blob/master/src/components/Accordion.js * as mentioned on comment 24, scrolling only parts of the page seems still not work yet * the long cookie value issue will be provided as another patch on this bug

(In reply to Fred Lin [:gasolin] from comment #31) > * Accordion component comes from debugger.html > https://github.com/devtools-html/debugger.html/blob/master/src/components/ > Accordion.js Gabe introduced the same Accordion component (although an outdated version) for the layout view at [0]. It'd be nice not to have the same component at 3 different places. Maybe the one with the m-c specific tweaks could live in devtools/client/shared/components/, so we only end up with 2 different copies. What do you think? [0]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/layout/components

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99562 ::: devtools/client/netmonitor/components/search-box.js:21 (Diff revision 7) > keyShortcut: L10N.getStr("netmonitor.toolbar.filterFreetext.key"), > placeholder: L10N.getStr("netmonitor.toolbar.filterFreetext.label"), > type: "filter", > }), > (dispatch) => ({ > onChange: (url) => { You might want to rename the `url` variable as well. ::: devtools/client/netmonitor/details-view.js:122 (Diff revision 7) > - this._cookies = new VariablesView($("#all-cookies"), > - Heritage.extend(GENERIC_VARIABLES_VIEW_SETTINGS, { > + this._cookies = new VariablesView($("#react-cookies-tabpanel-hook"), > + Heritage.extend(GENERIC_VARIABLES_VIEW_SETTINGS, {})); > - emptyText: L10N.getStr("cookiesEmptyText"), > - searchPlaceholder: L10N.getStr("cookiesFilterText") > - })); Why is this still using the VariablesView constructor? The cookies-panel component is supposed to replace the VView altogether. Moreover, this._cookies shouldn't be needed anymore. ::: devtools/client/netmonitor/shared/components/accordion.css:18 (Diff revision 7) > + width: 100%; > + -moz-user-select: none; > +} > + > +.accordion ._header:hover { > + background-color: var(--theme-search-overlays-semitransparent); This var doesn't exist in m-c (seems debugger-html specific :( ). ::: devtools/client/netmonitor/shared/components/cookies-panel.js:59 (Diff revision 7) > + }, > + opened: true, > + }; > + }); > + > + return div({className: "cookiesTabBox tabContainer"}, We like to use cookies-tab-box instead of camelCase for CSS. ::: devtools/client/netmonitor/shared/components/info-params.js:13 (Diff revision 7) > + > +const {div, span} = React.DOM; > +const PropTypes = React.PropTypes; > + > +/** > + * This template renders list of parameters within a group. My main concern with this component + Accordion, is that it's essentially the same component as TreeTable, but with the top level item styled differently. So we're creating a new component for something that's already covered by TreeTable. I think it's safe here (and much simpler) to just use the TreeTable component directly here without any styling changes. This way, we end up with something consistent with the Security panel, and in the future across panels. ::: devtools/client/themes/netmonitor.css:1100 (Diff revision 7) > display: -moz-box; > -moz-box-orient: vertical; > -moz-box-flex: 1; > } > + > +.cookiesTabBox.tabContainer { nit: use the foo-bar style instead of fooBar here and below ::: devtools/client/webconsole/net/components/cookies-tab.js (Diff revision 7) > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > "use strict"; > > const React = require("devtools/client/shared/vendor/react"); > const NetInfoGroupList = React.createFactory(require("./net-info-group-list")); > -const Spinner = React.createFactory(require("./spinner")); Nit: Unrelated change

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99570 ::: devtools/client/netmonitor/components/search-box.js:16 (Diff revision 7) > keyShortcut: L10N.getStr("netmonitor.toolbar.filterFreetext.key"), > placeholder: L10N.getStr("netmonitor.toolbar.filterFreetext.label"), > type: "filter", Now that this search box is shared, it doesn't really make sense to leave these props here. Maybe it would be better to put them here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/components/toolbar.js#31

So, when it comes to the accordion that is taken in place from debugger.html, I would add a note to both the accordion.js and accordion.css to acknowledge this fact and where you should go to file bugs or tweak the changes (basically where is the source of truth). See the accordion files in https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/layout/components for the message I used. It is assumed that if we ever figure out how to share components from Github, we would remove these accordions in m-c.

Thanks ntim to point out the dup of Accordion.js! I made a patch that based on Gabriel's Accordion version, it works fine as well. If we end up using accordion, I'll move it to devtools/client/shared/components I'll also try to make a version with TreeView component, though my concern is we don't have UX review at this moment, not sure if the change UI workflow in devtools is fine when all engineers agree that.

(In reply to Fred Lin [:gasolin] from comment #37) > I'll also try to make a version with TreeView component, though my concern > is we don't have UX review at this moment, not sure if the change UI > workflow in devtools is fine when all engineers agree that. If you're concerned of changing the UX, you can style the top-level item differently, and you would end up with exactly the same UX as previously with a TreeTable.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99840 ::: devtools/client/netmonitor/selectors/requests.js:112 (Diff revision 9) > } > > return getRequestById(state, state.requests.selectedId); > } > > +function getSelectedCookies(state) { This selector function is not used anywhere, but should be in the CookiesPanel's mapStateToProps. ::: devtools/client/netmonitor/selectors/requests.js:115 (Diff revision 9) > + let response = selectedRequest && selectedRequest.responseCookies ? > + selectedRequest.responseCookies : {cookies: []}; > + let request = selectedRequest && selectedRequest.requestCookies ? > + selectedRequest.requestCookies : {cookies: []}; If no request is selected, this selector function should return null. Use createSelector to memoize the return value if "selectedRequest" is unchanged. Will avoid a lot of unnecessary rerenders. Instead of testing for presence of "requestCookies" and "responseCookies", define "{cookies: []}" as default value in the definition of the "Request" record. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:20 (Diff revision 9) > +const Actions = require("../../actions/index"); > +const { getSelectedRequest } = require("../../selectors/index"); > + > +const { div } = DOM; > +const SearchBox = createFactory(require("devtools/client/shared/components/search-box")); > +const Accordion = createFactory(require("devtools/client/inspector/layout/components/Accordion")); Move the Accordion to shared components. The best way is to do it as "Part 1" patch in this bug, with the main part of the implementation in a "Part 2" patch. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:42 (Diff revision 9) > + groups = groups.filter(group => { > + return group.params && group.params.length; > + }); nit: do a shorter and more readable one-liner: filter(group => group.params && groups.params.length) ::: devtools/client/netmonitor/shared/components/cookies-panel.js:83 (Diff revision 9) > + const selectedRequest = getSelectedRequest(state); > + const response = selectedRequest && selectedRequest.responseCookies ? > + selectedRequest.responseCookies : {cookies: []}; > + const request = selectedRequest && selectedRequest.requestCookies ? > + selectedRequest.requestCookies : {cookies: []}; Use a selector function here - the mapStateToProps should be as simple as possible, everything nontrivial delegated to selectors. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:98 (Diff revision 9) > + cookiesFilterHandler: value => { > + dispatch(Actions.setCookiesFilterText(value)); > + } Rename the "cookiesFilterHandler" to "setCookiesFilterText" - make the property name equal to the action name. As a step two, use bindActionCreators: dispatch => bindActionCreators(Actions, dispatch) ::: devtools/client/netmonitor/shared/components/info-params.js:18 (Diff revision 9) > + * This template renders list of parameters within a group. > + * It's essentially a list of name + value pairs. > + */ > +function InfoParams({params = [], filter = "", key}) { > + let filtered = filter === "" ? params : > + params.filter(param => param.name.toLowerCase().indexOf(filter.toLowerCase()) >= 0); Call toLowerCase only once: const lcText = filter.toLowerCase() Use Array.prototype.includes: params.filter(param => param.name.toLowerCase().includes(lcText)) ::: devtools/client/netmonitor/shared/components/info-params.js:20 (Diff revision 9) > + filtered.sort(function (a, b) { > + return a.name > b.name ? 1 : -1; > + }); nit: use one-line arrow function here. Maybe a better design would be to do this filtering and sorting somewhere in a selector outside the React component. Not in the render method here. Also, please put some effort into optimizing the rendering of CookiesPanel and InfoParams - right now, the CookiesPanel gets rerendered on every change of the Redux state, even a completely unrelated one. ::: devtools/client/netmonitor/shared/components/info-params.js:25 (Diff revision 9) > + filtered.sort(function (a, b) { > + return a.name > b.name ? 1 : -1; > + }); > + > + let rows = []; > + filtered.forEach((param, index) => { Nit: use filtered.map ::: devtools/client/netmonitor/shared/components/info-params.js:28 (Diff revision 9) > + > + let rows = []; > + filtered.forEach((param, index) => { > + rows.push( > + div({ > + className: "variables-view-variable variable-or-property", Please don't use the variables-view styles here. ::: devtools/client/netmonitor/shared/components/info-params.js:29 (Diff revision 9) > + let rows = []; > + filtered.forEach((param, index) => { > + rows.push( > + div({ > + className: "variables-view-variable variable-or-property", > + key: param.name + index, It's not useful to use "param.name" here. Just "index" does the job equally well. ::: devtools/client/netmonitor/shared/components/info-params.js:32 (Diff revision 9) > + span({className: "plain name"}, param.name), > + span({className: "plain separator"}, ": "), > + span({className: "plain value token-string"}, `"${param.value}"`) Don't use "plain" CSS class here. It's intended only for XUL labels, not useful in HTML markup. ::: devtools/client/netmonitor/shared/components/info-params.js:41 (Diff revision 9) > + ) > + ); > + }); > + > + return div({ > + key, Don't pass the key to the child element. If you pass the key as a property to InfoParams, React will read and use it. You don't need to handle it in any way inside the component. In fact, it won't be ever passed as a parameter to InfoParams. Try it yourself in debugger - you'll never get "key" and "ref" - they are reserved properties. ::: devtools/client/netmonitor/shared/components/info-params.js:49 (Diff revision 9) > + > +InfoParams.displayName = "InfoParams"; > + > +InfoParams.propTypes = { > + filter: PropTypes.string.isRequired, > + key: PropTypes.string.isRequired, No need to declare "key" here - it's a reserved React property. ::: devtools/client/themes/netmonitor.css:1159 (Diff revision 9) > +.devtools-toolbar.devtools-searchbox { > + width: 100%; > +} What does this style do? I don't see it used anywhere.

Several UI issues I found during review. Illustrated in the attached screenshot. Compared to the "Headers" tab, the "Cookies" tab has the following issues: - the filter box is focused each time I select the Cookies tab. That's unwanted - it's quite rare to use the filtering feature, so it's a distraction to get it focused every time. - when the Cookies tab is selected, both the "Request" and "Response" groups are collapsed. Not very friendly - when selecting the tab, I expect to see the cookies info, not a collapsed UI with nothing. - the cookies list has a very small line-height - looks very compressed. Should be consistent with Headers. - the "Request cookies" label and the cookie names should be aligned under each other on a single vertical line - wrapping long values behaves significantly worse than in Headers. Headers use "crop=center", which is a XUL-only feature. In HTML version, text-overflow: ellipsis at the end should be sufficient.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99860 ::: devtools/client/netmonitor/shared/components/info-params.js:18 (Diff revision 9) > + * This template renders list of parameters within a group. > + * It's essentially a list of name + value pairs. > + */ > +function InfoParams({params = [], filter = "", key}) { > + let filtered = filter === "" ? params : > + params.filter(param => param.name.toLowerCase().indexOf(filter.toLowerCase()) >= 0); The original XUL version searches also values, not only names. A very useful feature that shouldn't be removed.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99968 ::: devtools/client/netmonitor/requests-menu-view.js:222 (Diff revision 9) > + responseCookies.cookies.forEach(function(cookie, idx) { > + cookies[idx].value = yield gNetwork.getString(cookie.value); > + }); You can't use yield inside a non-generator function. You'll need a for-let-of loop, or a Promise.all call. And what's the "cookies" variable?

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99968 > You can't use yield inside a non-generator function. You'll need a for-let-of loop, or a Promise.all call. > > And what's the "cookies" variable? `updateRequest` is a generator function, so its safe to use yield here. `cookies` supposed to be `responseCookies.cookies`, my bad

(In reply to Fred Lin [:gasolin] from comment #44) > `updateRequest` is a generator function, so its safe to use yield here. I'm talking about the for-each callback. You probably expect that the code will call getString in a loop and will wait for the async completion each time, but that's not what will happen.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review100096 Fred, Params panel patch (bug 1317650) has been uploaded and asked for the first round review already. TreeView component works perfectly after customizing CSS rules, please apply my patch to see what changed accordingly. There are no changes for usability although some visuals are slightly different, but we can continue tweaking styling until everyone agrees. Thus, I think for the rest of the panels (Header, Cookies, Params, Response) should be migrated to TreeView component as possible to keep the same styling. I'd cancel review flag and please make sure migrate the current implementation to TreeView instead.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; Ouch thanks Jarda for catching that! This PR * address comments * update for..of syntax * use reselect Cookie I've checked Ricky's patch and I think UI with TreeView looks good as well, I'd change Accrodion to TreeView.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review100202 ::: devtools/client/netmonitor/requests-menu-view.js:215 (Diff revisions 9 - 11) > const action = Actions.updateRequest(id, data, true); > yield this.store.dispatch(action); > > let { responseContent, requestPostData } = action.data; > > if (responseContent && responseContent.content) { There's no reason why cookies string fetching should be performed only if the incoming action has a "responseContent" field. The logic should be: 1. If the action.data has a responseContent field and the MIME type is an image, then fetch the responseContent body and create data URI from it. 2. If the action.data has a requestPostData field, fetch the POST body and extract headers from it. 3. If the action.data has a request/responseCookies field, fetch the full cookie values. These three actions are fairly independent from each other, and should become separate action creators soon. ::: devtools/client/netmonitor/requests-menu-view.js:237 (Diff revisions 9 - 11) > - cookies[idx].value = yield gNetwork.getString(cookie.value); > - }); > + let reqCookie = Object.assign({}, cookie); > + reqCookie.value = yield gNetwork.getString(cookie.value); > + reqCookies.push(reqCookie); > + }; > + } > + if (rspCookies || reqCookies) { This condition always evaluates to true - rsp/reqCookies is an array, sometimes empty, sometimes not. Maybe you wanted to check the array length? ::: devtools/client/netmonitor/selectors/requests.js:97 (Diff revisions 9 - 11) > }; > } > ); > > +// reselect version of getSelectedRequest > +const getReSelectedRequest = createSelector( Do we really need two implementations of getSelectedRequest, one memoized and one not? I think one should be sufficient. The one with memoization. It will save the find() call (O(n) complexity) in case neither request nor selection has changed. ::: devtools/client/netmonitor/selectors/requests.js:111 (Diff revisions 9 - 11) > + let response = selectedRequest && selectedRequest.responseCookies ? > + selectedRequest.responseCookies : null; If you return {cookies:[]} here instead of null, you won't need to do a "data.response ? data.response.cookies : []" check later in the CookiesPanel component. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:88 (Diff revisions 9 - 11) > - cookiesFilterHandler: value => { > + setCookiesFilterText: value => { > dispatch(Actions.setCookiesFilterText(value)); > } nit: one-line lambda without braces: value => dispatch(...) ::: devtools/client/netmonitor/shared/components/info-params.js:20 (Diff revisions 9 - 11) > */ > -function InfoParams({params = [], filter = "", key}) { > +function InfoParams({params = [], filter = ""}) { > + let lcText = filter.toLowerCase() > let filtered = filter === "" ? params : > - params.filter(param => param.name.toLowerCase().indexOf(filter.toLowerCase()) >= 0); > + params.filter(param => param.name.toLowerCase().includes(lcText) || > + param.value.toLowerCase().includes(lcText)); Can it ever happen that the "param.value" is not a string, but a longStringGrip object? The string value is fetched asynchronously, so there is a time period where it's still a grip, isn't it? It's very short, but I can imagine that a test can sometimes fail here.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review100576 I don't really have anything to add. Jarda will likely have more useful comments. ::: devtools/client/netmonitor/actions/filters.js:55 (Diff revision 12) > +/** > + * Set filter text in cookies tabpanel. > + * > + * @param {string} text - A filter text is going to be set > + */ > +function setCookiesFilterText(text) { > + return { > + type: SET_COOKIES_FILTER_TEXT, > + text, > + }; > +} > + It would be nice if the actions/reducers were inside a shared directory as well, since we'll likely want the web console to benefit from those files. Jarda, what do you think? ::: devtools/client/netmonitor/components/search-box.js:16 (Diff revision 12) > keyShortcut: L10N.getStr("netmonitor.toolbar.filterFreetext.key"), > placeholder: L10N.getStr("netmonitor.toolbar.filterFreetext.label"), Since this component is now more generic, these 2 props don't belong here. ::: devtools/client/netmonitor/selectors/requests.js:108 (Diff revision 12) > + > + return requests.requests.find(r => r.id === requests.selectedId); > + } > +); > + > +const getSelectedCookies = createSelector( Same about selectors, it'd be nice if they were in shared. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:40 (Diff revision 12) > +}) { > + let object = {}; > + if (data.response.length) { > + object[responseCookiesStr] = arrayToMap(data.response); > + } > + console.log(data.request); nit: console.log ::: devtools/client/themes/netmonitor.css:1156 (Diff revision 12) > > +.detailsTreeView .treeTable { > + display: block; > + position: absolute; > + overflow-y: auto; > + top: 48px; Please add a comment to get this hack removed later on.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review100202 > There's no reason why cookies string fetching should be performed only if the incoming action has a "responseContent" field. The logic should be: > > 1. If the action.data has a responseContent field and the MIME type is an image, then fetch the responseContent body and create data URI from it. > > 2. If the action.data has a requestPostData field, fetch the POST body and extract headers from it. > > 3. If the action.data has a request/responseCookies field, fetch the full cookie values. > > These three actions are fairly independent from each other, and should become separate action creators soon. Thanks for explaination, I've move cookies long value retrive as a separate generator function

The new PR come with TreeView, address comments and put requestCookiesLongValue as a separate generator function

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review100604 ::: devtools/client/netmonitor/components/search-box.js:16 (Diff revision 14) > keyShortcut: L10N.getStr("netmonitor.toolbar.filterFreetext.key"), > placeholder: L10N.getStr("netmonitor.toolbar.filterFreetext.label"), This component is now generic, so it shouldn't have toolbar strings being set here.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #56) > > +function setCookiesFilterText(text) { > > It would be nice if the actions/reducers were inside a shared directory as > well, since we'll likely want the web console to benefit from those files. > Jarda, what do you think? The Cookies/Params components would be simpler and more reusable if they didn't use Redux for the filterText at all. And if they stored it in component state instead. The actions and reducers are trivial - they just set a simple value, with no additional logic. Redux doesn't add any value here, just complexity and boilerplate code. Dan Abramov has a nice article about the idea that not everything needs to be in Redux: https://medium.com/@dan_abramov/you-might-not-need-redux-be46360cf367#.tnszcj93j With local state for the filter, the components will be much easier to reuse at other places. > > +const getSelectedCookies = createSelector( > > Same about selectors, it'd be nice if they were in shared. Selectors are very likely to be different for other places where the component is used. They depend on the exact shape of the Redux store. The same is true for the connect() call. There will be different implementations, depending on what the component is being connected to. Reusability would be helped if the presentation component and the container component were in different files. See how zeit/hyper organize their code: https://github.com/zeit/hyper/blob/master/lib/containers/header.js Presentation component is in lib/components/header, the container is in lib/containers/header.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review99840 > What does this style do? I don't see it used anywhere. It should be `devtools-toolbar .devtools-searchbox {}` which expand search box's width to 100%

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review100576 > It would be nice if the actions/reducers were inside a shared directory as well, since we'll likely want the web console to benefit from those files. > Jarda, what do you think? I think it worth a exploration for possible solutions. Fire bug 1325316 for this. I'll take some time to research it. Before we have some conclution, it might too early to shrare redux actions/reducers accross projects. > Since this component is now more generic, these 2 props don't belong here. components/search-box.js is only used for toolbar and not relevant to this patch

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review101500 I haven't applied the patch and verified user interaction yet, but still saw some issues so you can address them first. Please keep being aware of any updates in ParamsPanel. I'm modifying TreeView implementation for supporting better experience to make it looks more like VariablesView.jsm. I think our current target should focus on stabilizing TreeView component first and find out a way to mimic string crop for TreeCell as well as displaying long string in input field which are WIP in ParamsPanel. Once it's stable, then you can copy it from ParamsPanel without rebasing repeatedly. ::: devtools/client/netmonitor/details-view.js:167 (Diff revision 14) > destroy: function () { > dumpn("Destroying the DetailsView"); > ReactDOM.unmountComponentAtNode(this._previewPanelNode); > ReactDOM.unmountComponentAtNode(this._securityPanelNode); > ReactDOM.unmountComponentAtNode(this._timingsPanelNode); > + ReactDOM.unmountComponentAtNode(this._cookiesPanelNode); nit: Alphabetical order. so move it to top of _previewPanelNode ::: devtools/client/netmonitor/requests-menu-view.js:210 (Diff revision 14) > > this.store.dispatch(action).then(() => window.emit(EVENTS.REQUEST_ADDED, action.id)); > }, > > + // Fetch (request or response) cookies long value > + fetchRequestCookiesLongValue: function* (request) { This API is similar to getFormDataSections() so it can move to request-utils.js. Please explain more details on comments and write down JSDoc format like getFormDataSections() did. ::: devtools/client/netmonitor/requests-menu-view.js:210 (Diff revision 14) > > this.store.dispatch(action).then(() => window.emit(EVENTS.REQUEST_ADDED, action.id)); > }, > > + // Fetch (request or response) cookies long value > + fetchRequestCookiesLongValue: function* (request) { Separate this into two method for purpose. 1. getRequestCookies() 2. getResponseCookies() ::: devtools/client/netmonitor/requests-menu-view.js:212 (Diff revision 14) > }, > > + // Fetch (request or response) cookies long value > + fetchRequestCookiesLongValue: function* (request) { > + let { responseCookies, requestCookies } = request; > + let rspCookies = []; nit: resCookies instead of rspCookies ::: devtools/client/netmonitor/requests-menu-view.js:218 (Diff revision 14) > + let reqCookies = []; > + if (responseCookies) { > + for (let cookie of responseCookies.cookies) { > + let rsp = yield gNetwork.getString(cookie.value); > + rspCookies.push(Object.assign({}, cookie, { > + value: rsp, nit: assign yield string directly and remove rsp ``` value: yield gNetwork.getString(cookie.value) ``` ::: devtools/client/netmonitor/requests-menu-view.js:228 (Diff revision 14) > + // request store cookies in requestCookies or requestCookies.cookies > + let cookies = requestCookies.cookies ? requestCookies.cookies : requestCookies; > + for (let cookie of cookies) { > + let req = yield gNetwork.getString(cookie.value); > + reqCookies.push(Object.assign({}, cookie, { > + value: req, nit: assign yield string directly and remove req ``` value: yield gNetwork.getString(cookie.value) ``` ::: devtools/client/netmonitor/requests-menu-view.js:236 (Diff revision 14) > + } > + let updateObj = {}; > + let isDirty = false; > + if (rspCookies.length) { > + updateObj.responseCookies = rspCookies; > + isDirty = true; What's the purpose for setting isDirty? It's more than enough to return new object instance with updated cookies payload. Remember to use Object.assign(...) for creating new object in this case. Redux is good at dealing with this and it is able to detect the new object reference and then tell react component update itself. ::: devtools/client/netmonitor/requests-menu-view.js:306 (Diff revision 14) > }, true)); > } > + > + > + // Fetch cookies long value when any cookie is exist > + if (responseCookies || requestCookies) { likewise. Call two methods (getRequestCookies() and getResponseCookies()) separately to fetch request cookies and response cookies parallelly. And remove isDirty check if it's unnecessary. ::: devtools/client/netmonitor/selectors/requests.js:107 (Diff revision 14) > -function getDisplayedRequestById(state, id) { > + return requests.requests.find(r => r.id === requests.selectedId); > - return getDisplayedRequests(state).find(r => r.id === id); > -} > + } > +); > > -function getSelectedRequest(state) { > +const getSelectedCookies = createSelector( I think we can split it into two selectors for specific purpose. 1. getSelectedRequestCookies 2. getSelectedResponseCookies ::: devtools/client/netmonitor/selectors/requests.js:121 (Diff revision 14) > + response = selectedRequest.responseCookies.cookies; > + } > + // request store cookies in requestCookies or requestCookies.cookies > + if (selectedRequest.requestCookies && > + selectedRequest.requestCookies) { > + request = selectedRequest.requestCookies.cookies ? nit: remove useless white space ::: devtools/client/netmonitor/selectors/requests.js:148 (Diff revision 14) > getDisplayedRequests, > getDisplayedRequestsSummary, > getRequestById, > getDisplayedRequestById, > getSelectedRequest, > + getSelectedCookies, nit: the order to export the method is right. Moreover, you should keep the order of above function definition like this as well to improve code consistency. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:10 (Diff revision 14) > +"use strict"; > + > +const { > + createFactory, > + DOM, > + PropTypes nit: add tailing , ::: devtools/client/netmonitor/shared/components/cookies-panel.js:34 (Diff revision 14) > + * sent and received cookies. > + */ > +function CookiesPanel({ > + data, > + cookiesFilterText, > + setCookiesFilterText nit: add tailing , ::: devtools/client/netmonitor/shared/components/cookies-panel.js:43 (Diff revision 14) > + object[responseCookiesStr] = arrayToMap(data.response); > + } > + if (data.request.length) { > + object[requestCookiesStr] = arrayToMap(data.request); > + } > + if (data.response.length === 0 && data.request.length === 0) { This condition can be moved to top of this function to speed up rendering if there is no data to show. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:140 (Diff revision 14) > + cookiesFilterText: state.filters.cookiesFilterText, > + }; > + }, > + dispatch => ({ > + setCookiesFilterText: value => > + dispatch(Actions.setCookiesFilterText(value)), nit: modify to one line setCookiesFilterText: (text) => dispatch(Actions.setCookiesFilterText(text)),

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review101500 > This API is similar to getFormDataSections() so it can move to request-utils.js. Please explain more details on comments and write down JSDoc format like getFormDataSections() did. File in `request-utils` because its used in multiple places (`getFormDataSections` is used in request-list-context-menu and details-view). And the fetchRequestCookiesLongValue will call gNetwork and store.dispatch which are not occurred in `request-utils` yet. Two separate generator functions(fetchResponseCookiesLongValue, fetchRequestCookiesLongValue) in request-menu-view might be clear enough in this phase? > nit: resCookies instead of rspCookies though some node express example use `res` to denote `response` message, `rsp` is still a more common abbr than `res` in networking domain (at least TCP protocol use rsp as abbr of `response header`)

new PR * address issues * use separate function to query request/response long cookie values in requests-menu-view * use separate function to query selected request/response cookies in cookies panel * Since cookies panel only display string values, display cookies as `input` in security panel instead of `Rep` in params panel.

(In reply to Fred Lin [:gasolin] from comment #66) > Comment on attachment 8815570 [details] > Bug 1317649 - Implement Cookies Panel; > > https://reviewboard.mozilla.org/r/96444/#review101500 > > > This API is similar to getFormDataSections() so it can move to request-utils.js. Please explain more details on comments and write down JSDoc format like getFormDataSections() did. > > File in `request-utils` because its used in multiple places > (`getFormDataSections` is used in request-list-context-menu and > details-view). > And the fetchRequestCookiesLongValue will call gNetwork and store.dispatch > which are not occurred in `request-utils` yet. > > Two separate generator functions(fetchResponseCookiesLongValue, > fetchRequestCookiesLongValue) in request-menu-view might be clear enough in > this phase? I haven't seen you dispatch any actions in fetchRequestCookiesLongValue() in previous patch, even if there was, action dispatching like that shouldn't be in this method because the method name tells you that it's aiming for fetching purpose, any action dispatching should be done after that. IMO, it's worth to move it to request-utils.js because the purpose of method is really similar to those getXXX() methods in request-utils.js. These useful APIs define in request-utils.js can benefit extensions ecosystem for those developers who want to bring more features to netmonitor. So that's why I think we can separate it into two methods for various use cases :)

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review101600 ::: devtools/client/netmonitor/components/search-box.js:16 (Diff revision 17) > keyShortcut: L10N.getStr("netmonitor.toolbar.filterFreetext.key"), > placeholder: L10N.getStr("netmonitor.toolbar.filterFreetext.label"), These 2 props need to be set here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/components/toolbar.js#31 ::: devtools/client/netmonitor/selectors/requests.js:112 (Diff revision 17) > + } else { > + // response store cookies in responseCookies.cookies > + if (selectedRequest.responseCookies && > + selectedRequest.responseCookies.cookies) { > + return selectedRequest.responseCookies.cookies; > + } > - } > + } I wonder why this isn't triggering ESLint's no-else-after-return rule. ::: devtools/client/netmonitor/selectors/requests.js:127 (Diff revision 17) > + } else { > + // request store cookies in requestCookies or requestCookies.cookies > + if (selectedRequest.requestCookies && > + selectedRequest.requestCookies) { > + return selectedRequest.requestCookies.cookies ? > + selectedRequest.requestCookies.cookies : selectedRequest.requestCookies; > + } > + } same here ::: devtools/client/netmonitor/shared/components/cookies-panel.js:60 (Diff revision 17) > + delay: FILTER_SEARCH_DELAY, > + onChange: setCookiesFilterText, > + placeholder: cookiesFilterTextStr, > + type: "filter" Let's override only the necessary props ::: devtools/client/netmonitor/shared/components/cookies-panel.js:62 (Diff revision 17) > + > + return div({ className: "detailsTreeView" }, > + SearchBox({ > + delay: FILTER_SEARCH_DELAY, > + onChange: setCookiesFilterText, > + placeholder: cookiesFilterTextStr, I wonder why we need a variable for the strings. L10N.getStr(..) would perfectly fit under 90 ch.

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #72) > I wonder why we need a variable for the strings. L10N.getStr(..) would > perfectly fit under 90 ch. react might render component repeatedly if there have props changed. Define L10N.getStr(..) as a constant out of component function can bring slightly performant improvement.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review101600 > These 2 props need to be set here: > https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/components/toolbar.js#31 netmonitor/components/search-box is not reused in cookies panel at all. we can create a shared/components/search-box for all kinds of filter search box, but it could be a separate bug

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review101992 ::: devtools/client/netmonitor/requests-menu-view.js:208 (Diff revisions 19 - 20) > // Fetch response cookies long value. > // Actor does not provide full sized cookie value when the value is too long > // To display values correctly, we need fetch them in each request. > fetchResponseCookiesLongValue: function* (request) { > let { responseCookies } = request; > - if (typeof responseCookies === "undefined") { > + if (typeof responseCookies === "undefined" || !responseCookies.cookies) { nit: !responseCookies || !responseCookies.cookies

Fred, could you apply the patch in bug 1328828 and test it Cookie panel?

I am attaching a screenshot showing couple of UI issues -> differences between the old (XUL) implementation and the new (React) version: 1) The panel displays just Cookie name and no other Cookie properties (as e.g. cookie expire date, cookie path, etc. 2) I am not seeing Response Cookies. Only Request cookies are displayed. Can you reproduce this on your machine? Honza

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; since properties-view is going to land, I'll rebase on that component and send review again

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review104094 Thanks for working on this Fred! R+ assuming try is green Honza ::: devtools/client/netmonitor/requests-menu-view.js:212 (Diff revision 23) > + let { responseCookies } = request; > + if (!responseCookies || !responseCookies.cookies) { > + return null; > + } > + > + let rspCookies = []; Please rename to 'resCookies' (already requested in a review)

Fred, The params panel (bug 1317650) is in landing process even though it hasn't arrived to m-c yet. There are some useful changes such as PropertiesView and style improvements and a new waitForDOM() helper to assist in writing react mochitest, so you can rebase you patch once params panel is landed and you don't have to touch css file in your patch. Moreover, PropertiesView has its own filterText state to take over the responsibility for handling filter searching. So it is unnecessary to add these extra action or reducer for filtering cookies search box.

The updated PR shows object rep when there are other cookies, it's a bit different from the current one. Honza, do you think we need to patch reps to show the default value for these kind of cookies (show the value instead of `Object` when the object contains the `value` property)?

Fred, could you please set me as another reviewer since I saw there are lots of issues need to be addressed. I can start to assist in reviewing this patch once you rebase on top of params panel (bug 1317650).

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review104108 ::: devtools/client/netmonitor/components/search-box.js:21 (Diff revision 24) > keyShortcut: L10N.getStr("netmonitor.toolbar.filterFreetext.key"), > placeholder: L10N.getStr("netmonitor.toolbar.filterFreetext.label"), > type: "filter", > }), > (dispatch) => ({ > - onChange: (url) => { > + onChange: text => dispatch(Actions.setRequestFilterText(text)), nit: always wrap () for parameters onChange: (text) => xxx ::: devtools/client/netmonitor/requests-menu-view.js:212 (Diff revision 24) > + let { responseCookies } = request; > + if (!responseCookies || !responseCookies.cookies) { > + return null; > + } > + > + let rspCookies = []; nit: as Honza mentioned, I agree with him to rename to resCookies ::: devtools/client/netmonitor/requests-menu-view.js:289 (Diff revision 24) > payload.requestHeadersFromUploadStream = { headers, headersSize }; > > yield this.store.dispatch(Actions.updateRequest(action.id, payload, true)); > } > + > + if (request) { I think there are some issues in here. We should fetch full cookies data from const { requestCookies, responseCookies } = action.data but not in request object itself. So you don't need to check requestCookies in your fetch methods. It can also bring some performance improvement with this way. ::: devtools/client/netmonitor/requests-menu-view.js:291 (Diff revision 24) > yield this.store.dispatch(Actions.updateRequest(action.id, payload, true)); > } > + > + if (request) { > + // Fetch responseCookies long value when any response cookie is exist > + let responseObj = yield this.fetchResponseCookiesLongValue(request); nit: In alphabetical order and rename responseObj to payload which has been used in former piece of code. ::: devtools/client/netmonitor/requests-menu-view.js:297 (Diff revision 24) > + if (responseObj) { > + yield this.store.dispatch(Actions.updateRequest( > + action.id, responseObj, true)); > + } > + // Fetch requestCookies long value when any request cookie is exist > + let requestObj = yield this.fetchRequestCookiesLongValue(request); nit: In alphabetical order and rename requestObj to payload which has been used in former piece of code. ::: devtools/client/netmonitor/selectors/requests.js:107 (Diff revision 24) > > return requests.requests.find(r => r.id === requests.selectedId); > } > ); > > +const getSelectedResponseCookies = createSelector( nit: method name should be ordered by alphabet. const getSelectedRequestCookies = ... const getSelectedResponseCookies = ... ::: devtools/client/netmonitor/selectors/requests.js:115 (Diff revision 24) > + if (!selectedRequest) { > + return []; > + } > + > + // response store cookies in responseCookies or responseCookies.cookies > + if (selectedRequest.responseCookies) { nit: it's unnecessary to check !selectedRequest in this addtional if statement. ``` if (selectedRequest && selectedRequest.responseCookies) { ... } return []; ``` ::: devtools/client/netmonitor/selectors/requests.js:131 (Diff revision 24) > + selectedRequest => { > + if (!selectedRequest) { > + return []; > + } > + > + // request store cookies in requestCookies or requestCookies.cookies nit: likewise above ::: devtools/client/netmonitor/selectors/requests.js:155 (Diff revision 24) > getDisplayedRequestById, > getDisplayedRequests, > getDisplayedRequestsSummary, > getRequestById, > getSelectedRequest, > + getSelectedResponseCookies, nit: alphabetical order ::: devtools/client/netmonitor/shared/components/cookies-panel.js:14 (Diff revision 24) > + DOM, > + PropTypes, > +} = require("devtools/client/shared/vendor/react"); > +const { connect } = require("devtools/client/shared/vendor/react-redux"); > +const { L10N } = require("../../l10n"); > +const { getSelectedResponseCookies, getSelectedRequestCookies } = require("../../selectors/index"); nit: ```js const { getSelectedRequestCookies, getSelectedResponseCookies, } = require("../../selectors/index"); ``` ::: devtools/client/netmonitor/shared/components/cookies-panel.js:16 (Diff revision 24) > +} = require("devtools/client/shared/vendor/react"); > +const { connect } = require("devtools/client/shared/vendor/react-redux"); > +const { L10N } = require("../../l10n"); > +const { getSelectedResponseCookies, getSelectedRequestCookies } = require("../../selectors/index"); > + > +const { div } = DOM; nit: code style consistency. See the patch of params panel. ```js // Component const PropertiesView = createFactory(require("./properties-view"); const { div } = DOM; const CONSTANT = xxx; ``` ::: devtools/client/netmonitor/shared/components/cookies-panel.js:21 (Diff revision 24) > +const { div } = DOM; > +const PropertiesView = createFactory(require("./properties-view")); > + > +const COOKIES_EMPTY_TEXT = L10N.getStr("cookiesEmptyText"); > +const COOKIES_FILTER_TEXT = L10N.getStr("cookiesFilterText"); > +const COOKIES_RESPONSE_COOKIES_TEXT = L10N.getStr("responseCookies"); nit: RESPONSE_COOKIES and please rearrange them in alphabetical order ::: devtools/client/netmonitor/shared/components/cookies-panel.js:22 (Diff revision 24) > +const PropertiesView = createFactory(require("./properties-view")); > + > +const COOKIES_EMPTY_TEXT = L10N.getStr("cookiesEmptyText"); > +const COOKIES_FILTER_TEXT = L10N.getStr("cookiesFilterText"); > +const COOKIES_RESPONSE_COOKIES_TEXT = L10N.getStr("responseCookies"); > +const COOKIES_REQUEST_COOKIES_TEXT = L10N.getStr("requestCookies"); nit: REQUEST_COOKIES and please rearrange them in alphabetical order ::: devtools/client/netmonitor/shared/components/cookies-panel.js:28 (Diff revision 24) > +const SECTION_NAMES = [ > + COOKIES_RESPONSE_COOKIES_TEXT, > + COOKIES_REQUEST_COOKIES_TEXT, > +]; > + > +/** nit: Every panel I copy panel introduction from MDN https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor#Cookies IMO, we can keep using this convention for all panels. Let's rewrite it to: ``` /* * Cookies panel component * This tab lists full details of any cookies sent with the request or response */ ``` ::: devtools/client/netmonitor/shared/components/cookies-panel.js:37 (Diff revision 24) > + */ > +function CookiesPanel({ > + request, > + response, > +}) { > + if (response.length === 0 && request.length === 0) { nit: ```js if (!response.length && !request.length) { ... } ``` ::: devtools/client/netmonitor/shared/components/cookies-panel.js:90 (Diff revision 24) > + }, {}); > +} > + > +module.exports = connect( > + state => ({ > + response: getSelectedResponseCookies(state), nit: alphabetical order ::: devtools/client/themes/netmonitor.css (Diff revision 24) > > .properties-view .searchbox-section { > flex: 0 1 auto; > } > > -.properties-view .devtools-searchbox { Is there any reason why we need to remove this?

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review104122 ::: devtools/client/netmonitor/requests-menu-view.js:206 (Diff revision 25) > }, > > + // Fetch response cookies long value. > + // Actor does not provide full sized cookie value when the value is too long > + // To display values correctly, we need fetch them in each request. > + fetchResponseCookiesLongValue: function* (request) { I'm still thinking that fetch... methods can be moved into request-utils.js since it's pretty much like getFormDataSections() which could be reused somewhere. Furthermore, everything in request-menu-view.js will be removed soon, so it's worth to move some reusable method to request-utils.js. ::: devtools/client/netmonitor/requests-menu-view.js:206 (Diff revision 25) > }, > > + // Fetch response cookies long value. > + // Actor does not provide full sized cookie value when the value is too long > + // To display values correctly, we need fetch them in each request. > + fetchResponseCookiesLongValue: function* (request) { nit: rename to fetchResponseCookies is fine. ::: devtools/client/netmonitor/requests-menu-view.js:226 (Diff revision 25) > + }, > + > + // Fetch request cookies long value. > + // Actor does not provide full sized cookie value when the value is too long > + // To display values correctly, we need fetch them in each request. > + fetchRequestCookiesLongValue: function* (request) { nit: rename to fetchRequestCookies. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:75 (Diff revision 25) > + * > + * @param {Object[]} arr - key-value pair array like cookies or params > + * @returns {Object} > + */ > +function arrayToDict(arr) { > + return arr.reduce(function (map, obj) { nit: using arrow function. ::: devtools/client/netmonitor/shared/components/cookies-panel.js:76 (Diff revision 25) > + * @param {Object[]} arr - key-value pair array like cookies or params > + * @returns {Object} > + */ > +function arrayToDict(arr) { > + return arr.reduce(function (map, obj) { > + // Display any other information other than the cookie name and value nit: Improve this comment to explain more implemenation details in length > 2 part and you can also rename arrayToDict() to getProperties() or something better.

(In reply to Fred Lin [:gasolin] from comment #88) > The updated PR shows object rep when there are other cookies, it's a bit > different from the current one. > > Honza, do you think we need to patch reps to show the default value for > these kind of cookies (show the value instead of `Object` when the object > contains the `value` property)? I am not sure what do you mean by "when there are other cookies" I can see the difference between the current and new design and I agree that we should display cookie value instead of `Object` keyword. You should be able to customize the rendering using `renderValue` pro (callback) in TreeView component. Honza

(In reply to Fred Lin [:gasolin] from comment #88) > Honza, do you think we need to patch reps to show the default value for > these kind of cookies (show the value instead of `Object` when the object > contains the `value` property)? I'm wondering what exactly is the purpose of the Reps classes. Is it to display various types of Javascript values? Or to display any data whatsoever? HTTP headers (including "Cookie") have generic string values that have nothing to do with any Javascript. Do Reps have the ambition to display this kind of data, too? In that case, maybe a special Rep for a "HTTPHeader" would be useful? It's certainly rather confusing if HTTP headers are displayed as some JS pseudo-objects or pseudo-arrays.

Honza, The full sentence is "when there are other cookies properties besides 'name', 'value'". It make sense to customize the tree view to display cookie value instead of `Object` keyword Jarda, as above comment we don't need change Reps to display extend cookies properties(besides the name and value), we can custom it on treeview.

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review104432 Nice job! I think it's ready to ship. Please address a few nits I commented in review board and rebase patch again, and finally make sure try is green before landing :) ::: devtools/client/netmonitor/requests-menu-view.js:319 (Diff revision 27) > + let payload = reqCookies.length ? { requestCookies: reqCookies } : null; > + if (payload) { > + yield this.store.dispatch(Actions.updateRequest( > + action.id, payload, true)); > + } nit: we can simply this part ```js if (reqCookies.length) { yield this.store.dispatch(Actions.updateRequest( action.id, { requestCookies: reqCookies }, true )); } ``` ::: devtools/client/netmonitor/requests-menu-view.js:336 (Diff revision 27) > + let payload = resCookies.length ? { responseCookies: resCookies } : null; > + if (payload) { > + yield this.store.dispatch(Actions.updateRequest( > + action.id, payload, true)); > + } nit: likewise above ::: devtools/client/netmonitor/shared/components/cookies-panel.js:15 (Diff revision 27) > + PropTypes, > +} = require("devtools/client/shared/vendor/react"); > +const { connect } = require("devtools/client/shared/vendor/react-redux"); > +const { L10N } = require("../../l10n"); > +const { > + getSelectedResponseCookies, nit: alphabetical order

Try commits looks weird to me https://treeherder.mozilla.org/#/jobs?repo=try&revision=a149fe4bc349&selectedJob=67860487 I think you can cherry pick your patch on top of master branch, and please ensure try is green since there are some netmonitor relevant test failures on try. thanks!

(In reply to Jarda Snajdr [:jsnajdr] from comment #94) > (In reply to Fred Lin [:gasolin] from comment #88) > > Honza, do you think we need to patch reps to show the default value for > > these kind of cookies (show the value instead of `Object` when the object > > contains the `value` property)? > > I'm wondering what exactly is the purpose of the Reps classes. Is it to > display various types of Javascript values? Or to display any data > whatsoever? HTTP headers (including "Cookie") have generic string values > that have nothing to do with any Javascript. Do Reps have the ambition to > display this kind of data, too? In that case, maybe a special Rep for a > "HTTPHeader" would be useful? > > It's certainly rather confusing if HTTP headers are displayed as some JS > pseudo-objects or pseudo-arrays. Reps are intended to render data such as: built in JS types, data structures coming from 3rd party libraries as well as data Devtools introduces. So, yes there might be a ep (template/component) for a cookie header or e.g. XHR log displayed in the Console panel. We just need to define JS data structure for it so, the rep can be registered for it. This would be especially useful if the same data structure is reused and rendered at different place (UI) in DevTools. But, I believe that this patch is good and ready to land. Honza

Typo: there might be a ep -> there might be a rep

Comment on attachment 8815570 [details] Bug 1317649 - Implement Cookies Panel; https://reviewboard.mozilla.org/r/96444/#review104506 OK, let's land another panel!

I'd fix the test and file another bug to customize the tree view to display cookie value instead of `Object` keyword

(In reply to Fred Lin [:gasolin] from comment #103) > I'd fix the test and file another bug to customize the tree view to display > cookie value instead of `Object` keyword OK for me. Honza

Note that Bug 1317648 (Headers Panel) is in landing procedure. You have to rebase again once it's landed in m-c. You might need to winding up as final step since there are some unused variable should be removed such as VariablesView, GENERIC_VARIABLES_VIEW_SETTINGS and this._dataSrc.

test passed, thanks! I tried to remove `_onTabSelect` in detailsView since all sidebar panels are removed, but it will cause plenty of tests fail. I think this bug should focus on landing the cookie panel related change, we could handle changes in detailsView at once in bug 1328197 (turning details view into react component).

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/088802f27318 Implement Cookies Panel;r=Honza,jsnajdr,rickychien

I have reproduced this bug with Nightly 53.0a1 (2016-11-15) (32-bit) on Windows 8.1 (64-bit). This bug's fix is verified on Latest Nightly 53.0a1. Build ID : 20170113030227 User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 [bugday-20170111]

This is also verified on 53 beta 7 (20170327081421) using Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.11.6. The Cookies Panel in netmonitor works as expected.