DetailsView component is part of SidebarView (it contains DetailsView and CustomRequestView), so here is a breakdown task of SidebarView for migrating details-view.js into DetailsView react component. - Reuse devtools/client/shared/components/tabs.js component to replace XUL tabbox. - Create new netmonitor/shared/components/details-view.js react component. - Remove netmonitor/details-view.js. - Remove all static XUL <tabbox id="event-details-pane"> tags in netmonitor.xul - DetailsView component will be imported directly in netmonitor-view.js

Some comments related to this bug (a follow up for our standup discussion today and IRC comments). 1) There is a ToolSidebar object ("devtools/client/inspector/toolsidebar") introduced to replace old and XUL based ToolSidebar object ("devtools/client/framework/sidebar"). 2) Net panel's DetailsView is using the old XUL one and we should replace it by the new one introduced in the Inspector panel. The Inspector panel shows how to use it. Note that the ToolSidebar (both the new and old one) uses Telemetry to collect user data about side panel usage. This is something we still want to support. Are there any difficulties to use this new ToolSidebar object? It's definitely better so share one implementation of a ToolSidebar among panels. Could we improve it (e.g. be more React style) so, even the Inspector panel can profit from it? 3) The new ToolSidebar object uses also Tabbar() component ("devtools/client/shared/components/tabs/tabbar"). This component allows to dynamically append panels. E.g. the preview side-panel is available only for plain HTML requests and so, don't have to be appended/instantiated otherwise. This sounds better than instantiating it, appending and later setting 'hidden' attribute on it. 4) Another reason why we should use the Tabbar() component is that it implements support for 'all tabs menu' and we need this feature. Honza

(In reply to Jan Honza Odvarko [:Honza] from comment #1) > Some comments related to this bug (a follow up for our standup discussion > today and IRC comments). > > 1) There is a ToolSidebar object ("devtools/client/inspector/toolsidebar") > introduced to replace old and XUL based ToolSidebar object > ("devtools/client/framework/sidebar"). I'm still thinking that the way we use ToolSidebar component which is built for inspector. It doesn't look like a traditional react style. ToolSidebar provides a set of public APIs such as addTab(), addExistingTab() and removeTab() which are used like http://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#538-565 to setup all tabs. I noticed that inspector.js isn't a react component and we are unable to benefit from react Virtual DOM optimization if we setup tab with addTab or removeTab() like this way. So I'd like to know that is there any good reason for it? or maybe Tabs component is a more suitable approach to build these tab panels and after then we can improve the telemetry feature as a followup. > 4) Another reason why we should use the Tabbar() component is that it > implements support for 'all tabs menu' and we need this feature. all tabs menu is an easy to copy function which is at http://searchfox.org/mozilla-central/source/devtools/client/shared/components/tabs/tabbar.js#143-168

Further comments (based on our discussion at today's standup) - It's ok if we don't use Inspector's ToolSidebar and do the implementation directly in DetailsView. But, we should make sure Telemetry is properly supported. - I think we should use Tabbar() component and not duplicate the code (especially the 'all tabs menu'). - The Tabbar() component can be extended to support 'children' components so, we can do the following: Tabbar({onAfterChange: this.onUpdateTelemetry}, HeadersPanel(), CookiesPanel(), ParamsPanel(), ResponsePanel(), SecurityPanel(), PreviewPanel(), TimingsPanel() ) In order to hide some panels if necessary, we could do the following: let tabs = [HeadersPanel(), ...]; if (showCookies()) { tabs.push(CookiesPanel()); } Tabbar({onAfterChange: this.onUpdateTelemetry}, tabs ) - I believe that DetailsView() will turn into a component at some point. Honza

After meeting with Honza yesterday, I've agreed with moving onAllTabMenuClick callback into tabs.js http://searchfox.org/mozilla-central/source/devtools/client/shared/components/tabs/tabbar.js#143-168

WIP patch has uploaded. Note that I haven't fixed layout issues but please take a look for other parts.

Comment on attachment 8828740 [details] Bug 1328197 - Implement details view https://reviewboard.mozilla.org/r/106036/#review107394 Thanks for the patch Ricky! * I like the 'connect' methods clean up and the way how 'request' object is passed into side panels. * I am seeing a little UI discrepancy. There is a little 1-2 px gap underneath of selected tab, it wasn't there before (see the attached screenshot). Also, height of the tab bar should be the same as height of the header for the request-list. * Selected tab isn't remembered. Open the sidebar, selecte e.g. the Response tab. Close and open the sidebar, the Response tab isn't selected anymore -> BUG Honza ::: devtools/client/netmonitor/actions/ui.js:53 (Diff revision 6) > /** > + * Change the selected tab index for details panel. > + * > + * @param {number} index - tab index to be selected > + */ > +function setDetailsPanelTabIndex(index) { This could be renamed to 'selectSidePanel' (to be consistent with the action-constant) ::: devtools/client/netmonitor/constants.js:27 (Diff revision 6) > OPEN_SIDEBAR: "OPEN_SIDEBAR", > OPEN_STATISTICS: "OPEN_STATISTICS", > PRESELECT_REQUEST: "PRESELECT_REQUEST", > REMOVE_SELECTED_CUSTOM_REQUEST: "REMOVE_SELECTED_CUSTOM_REQUEST", > SELECT_REQUEST: "SELECT_REQUEST", > + SET_DETAILS_PANEL_TAB_INDEX: "SET_DETAILS_PANEL_TAB_INDEX", Could we rename to something like: SELECT_SIDE_PANEL? ::: devtools/client/netmonitor/netmonitor-view.js:25 (Diff revision 6) > +const { createFactory } = require("devtools/client/shared/vendor/react"); > +const Actions = require("./actions/index"); > +const ReactDOM = require("devtools/client/shared/vendor/react-dom"); > +const Provider = createFactory(require("devtools/client/shared/vendor/react-redux").Provider); > +const DetailsPanel = createFactory(require("./shared/components/details-panel")); > I saw in some other patches/reviews that we are trying to preserver alphabeticall order of imports. Do we have a rule for that? Honza ::: devtools/client/netmonitor/reducers/ui.js:16 (Diff revision 6) > + SET_DETAILS_PANEL_TAB_INDEX, > WATERFALL_RESIZE, > } = require("../constants"); > > const UI = I.Record({ > + detailsPanelTabIndex: 0, detailsPanelTabIndex => selectedSidePanel

Comment on attachment 8828740 [details] Bug 1328197 - Implement details view https://reviewboard.mozilla.org/r/106036/#review107394 > This could be renamed to 'selectSidePanel' (to be consistent with the action-constant) Renamed to selectDetailsPanelTab instead of selectSidePanel since I don't think the tab belongs to sidebar component definitely. Sidebar contains two panels details panel and custom request panel. > I saw in some other patches/reviews that we are trying to preserver alphabeticall order of imports. Do we have a rule for that? > > Honza Good question! I haven't come out with good solution so far and I'd prefer to loosen the check in these files which are going to be deprecated soon. Moreover, we can deal with this code styling improvement more strictly with eslint in one sweep after moving to Github.

(In reply to Jan Honza Odvarko [:Honza] from comment #12) > * I am seeing a little UI discrepancy. There is a little 1-2 px gap > underneath of selected tab, it wasn't there before (see the attached > screenshot). Also, height of the tab bar should be the same as height of the > header for the request-list. inspector doesn't has this issue due to box-sizing: border-box in inspector.css * { box-sizing: border-box; } Do you think it's fine to set all elements to border-box for netmonitor? > * Selected tab isn't remembered. Open the sidebar, selecte e.g. the Response > tab. Close and open the sidebar, the Response tab isn't selected anymore -> > BUG Fixed!

Comment on attachment 8828740 [details] Bug 1328197 - Implement details view https://reviewboard.mozilla.org/r/106036/#review107486 A few more comments but his is coming along nicely! Honza ::: devtools/client/themes/netmonitor.css:9 (Diff revisions 6 - 7) > +* { > + box-sizing: border-box; > +} > + I am not seeing any UI issues and it looks like that even Paul Irish would R+ this [1] ;-) [1] https://www.paulirish.com/2012/box-sizing-border-box-ftw/ ::: devtools/client/netmonitor/shared/components/headers-panel.js:47 (Diff revision 7) > const HeadersPanel = createClass({ > displayName: "HeadersPanel", > > propTypes: { > cloneSelectedRequest: PropTypes.func.isRequired, > request: PropTypes.object, 'request' prop is required. ::: devtools/client/netmonitor/test/browser_net_resend.js:50 (Diff revision 7) > > // edit the custom request > yield editCustomForm(); > // FIXME: reread the customItem, it's been replaced by a new object (immutable!) > customItem = RequestsMenu.selectedItem; > + yield once(document, "sdoijf"); "sdoijf"? Should we use a bit more meaningfull text :-) ::: devtools/client/netmonitor/test/browser_net_security-details.js:25 (Diff revision 7) > yield ContentTask.spawn(tab.linkedBrowser, REQUESTS_URL, function* (url) { > content.wrappedJSObject.performRequests(1, url); > }); > yield wait; > > - wait = waitForDOM(document, "#security-tabpanel"); > + wait = waitForDOM(document, "#panel-5"); Auto generated ID like: "#panel-5" can be unstable in case we change order of panels. What if we modified the Tabs component and introduced a new property that allows to specify panel/tab ID? It'd be fine to do it as a follow up but, it looks it could already help with tests now.

Comment on attachment 8828740 [details] Bug 1328197 - Implement details view https://reviewboard.mozilla.org/r/106036/#review107486 > Auto generated ID like: "#panel-5" can be unstable in case we change order of panels. What if we modified the Tabs component and introduced a new property that allows to specify panel/tab ID? > > It'd be fine to do it as a follow up but, it looks it could already help with tests now. I'd prefer to leave it as a follow up since it's still necessary to use #tab-0 ~ #tab-6 to select a tab. Tab component needs to support customize id props to deal with this requirement.

Comment on attachment 8828740 [details] Bug 1328197 - Implement details view https://reviewboard.mozilla.org/r/106036/#review107552 Looks good now R+ assuming try is green Honza

Comment on attachment 8828740 [details] Bug 1328197 - Implement details view https://reviewboard.mozilla.org/r/106036/#review107764 ::: devtools/client/netmonitor/actions/ui.js:10 (Diff revision 10) > "use strict"; > > const { > OPEN_SIDEBAR, > OPEN_STATISTICS, > + SELECT_DETAILS_PANEL_TAB, `SELECT_PANEL_TAB` might be clear enough ::: devtools/client/netmonitor/actions/ui.js:53 (Diff revision 10) > /** > + * Change the selected tab for details panel. > + * > + * @param {number} index - tab index to be selected > + */ > +function selectDetailsPanelTab(index) { `SelectPanelTab` seems clear enough ::: devtools/client/netmonitor/netmonitor-view.js:24 (Diff revision 10) > const { Prefs } = require("./prefs"); > +const { createFactory } = require("devtools/client/shared/vendor/react"); > +const Actions = require("./actions/index"); > +const ReactDOM = require("devtools/client/shared/vendor/react-dom"); > +const Provider = createFactory(require("devtools/client/shared/vendor/react-redux").Provider); > +const DetailsPanel = createFactory(require("./shared/components/details-panel")); I think the details panel might not the thing we want to share with webconsole, therefore we can put it into general `components` folder instead of `shared` folder

Comment on attachment 8828740 [details] Bug 1328197 - Implement details view https://reviewboard.mozilla.org/r/106036/#review107768 ::: devtools/client/netmonitor/shared/components/details-panel.js:51 (Diff revision 10) > + showAllTabsMenu: true, > + tabActive: tabIndex, > + toolbox, > + }, > + TabPanel({ > + title: L10N.getStr("netmonitor.tab.headers")}, would you like declare string as CONST outside of the react component for code consistency? ::: devtools/client/netmonitor/shared/components/params-panel.js:40 (Diff revision 10) > - formDataSections, > + request, > - mimeType, > - postData, > - query, > }) { > + let { formDataSections, mimeType, requestPostData, url } = request; declare variables in separate line for code consistency

Comment on attachment 8828740 [details] Bug 1328197 - Implement details view https://reviewboard.mozilla.org/r/106036/#review107764 > `SELECT_PANEL_TAB` might be clear enough I still prefer to use DetailsPanel explicitly. Sidebar could contain two panels (details panel and custom request panel), so it would be clear to distinct them. > I think the details panel might not the thing we want to share with webconsole, therefore we can put it into general `components` folder instead of `shared` folder No. what you said is exactly the opposite. DetailsPanel is exactly indentical to HTTP inspector. So the reason we moved all sub-panels and details panel to here is in order to share with webconsole.

Pushed by rchien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/12b1474b8065 Implement details view r=Honza

This issue is verified fixed using latest Nightly 54.0a1 (2017-01-26). Details view is working correctly under Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.11.6.

Got several warnings when `ac_add_options --enable-debug-js-modules` is set in mozconfig Warning: Failed prop type: Required prop `setTabIndex` was not specified in `DetailsPanel`. in DetailsPanel (created by Connect(DetailsPanel)) in Connect(DetailsPanel) in Provider Warning: Failed prop type: Required prop `selectedTab` was not specified in `DetailsPanel`. in DetailsPanel (created by Connect(DetailsPanel)) in Connect(DetailsPanel) in Provider Warning: Failed prop type: Invalid prop `children` of type `array` supplied to `Tabbar`, expected `object`. in Tabbar (created by DetailsPanel) in DetailsPanel (created by Connect(DetailsPanel)) in Connect(DetailsPanel) in Provider