1317645 - Implement NetworkDetailsPanel

Status: VERIFIED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

We're going to keep sidebar UI as is, so sidebar view a.k.a network details view will be converted into react.

Comment 1

[[e-mail]]

should refer inspector/toolsidebar.js to replace ToolSidebar implemented in devtools/client/framework/sidebar.js module

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Changed bug title to focus on implementing SidebarView react component.

Architecture diagram of sidebar view

SidebarView
- DetailsView
- CustomRequestView

Smaller tasks should be done:

- Create new netmonitor/shared/components/sidebar-view.js react component.
- Remove netmonitor/sidebar-view.js.
- Remove all static XUL <deck id="details-pane"> tags in netmonitor.xul
- SidebarView component will be imported directly in netmonitor-view.js

Comment 3

[[e-mail]]

I make a quick try to replace <deck> to <html:div> with bug 1308449(custom request view) patch, it will show custom request view instead of detail view by default. We can address this issue in this bug as well.

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Rename to NetworkDetailsPanel since the name "network-details" has been used in Prefs which represents to entire sidebar component, so it makes sense to follow this convention. We can also rename current DetailsPanel component here in case of confusion.

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Attachment: Custom request panel

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

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1317645 - Implement NetworkDetailsPanel

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

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Patch is ready and time to ask first round review. I'll ask second round review after bug 1308449 is landed.

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

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

Comment 9

[[e-mail]]

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

https://reviewboard.mozilla.org/r/109466/#review111428

::: devtools/client/netmonitor/netmonitor-view.js:79
(Diff revision 2)
>  
>    /**
>     * Gets the current mode for this tool.
>     * @return string (e.g, "network-inspector-view" or "network-statistics-view")
>     */
>    get currentFrontendMode() {

we can safely remove this seems its not used anywhere

::: devtools/client/netmonitor/shared/components/details-panel.js
(Diff revision 2)
>  DetailsPanel.propTypes = {
> +  activeTabId: PropTypes.string,
>    cloneSelectedRequest: PropTypes.func.isRequired,
>    request: PropTypes.object,
> -  setTabIndex: PropTypes.func.isRequired,
> +  selectTab: PropTypes.func.isRequired,
> -  selectedTab: PropTypes.number.isRequired,

its nice to fix props warnings!

::: devtools/client/netmonitor/shared/components/network-details-panel.js:47
(Diff revision 2)
> +      DetailsPanel({
> +        activeTabId,
> +        request,
> +        selectTab,
> +        toolbox,
> +      })

put `:` in same line `}) :`

::: devtools/client/netmonitor/shared/components/network-details-panel.js:71
(Diff revision 2)
> +
> +module.exports = connect(
> +  (state) => ({
> +    activeTabId: state.ui.detailsPanelSelectedTab,
> +    open: state.ui.networkDetailsOpen,
> +    request: getSelectedRequest(state),

`toolbox` is missing in connect but showed in NetworkDetailsPanel props

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

https://reviewboard.mozilla.org/r/109466/#review111428

> `toolbox` is missing in connect but showed in NetworkDetailsPanel props

Yes, and it is correct. There are two ways to define component props, one is in connect() and another way is to pass it as component props from the outside. See netmonitor-view.js:

```js
ReactDOM.render(Provider(
  { store: gStore },
  NetworkDetailsPanel({ toolbox: NetMonitorController._toolbox }),
), this.networkDetailsPanel);
```

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

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

Comment 12

[Ricky Chien [:rickychien] (inactive)]

Rebased patch on top of latest CustomRequestPanel.

Comment 13

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

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

https://reviewboard.mozilla.org/r/109466/#review111652

Couple of UI issues:

1) I don't see the toggle button in the UI. Clicking on the empty spot causes an error: Console Service ERROR [JavaScript Error: "TypeError: Actions.toggleSidebar is not a function" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/netmonitor/components/toolbar.js" line: 154}] [JavaScript Error: "TypeError: Actions.toggleSidebar is not a function" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/netmonitor/components/toolbar.js" line: 154}]

2) When custom request form is opened it has different than the original side panel. It should be the same (caused by removing <deck> I guess). This might be also problem of the CustomRequestPanel component.

Honza

::: devtools/client/netmonitor/netmonitor.xul:39
(Diff revision 3)
>                    class="devtools-side-splitter"/>
>  
> -        <deck id="details-pane"
> +        <box id="splitter-adjustable-box"
> -              hidden="true">
> +             hidden="true">
>            <html:div xmlns="http://www.w3.org/1999/xhtml"
> -                    id="react-custom-request-panel-hook"/>
> +                    id="react-network-details-panel-hook">

Do we really need 'network' work within the ID? Everything we do here is related to 'network' to, the word seems to be usealess.

This is related to other names as well (e.g. OPEN_NETWORK_DETAILS, NetworkDetailsPanel, etc.)

---

Now, I have noticed that we have 'NetworkDetailsPanel' and 'DetailsPanel', which might explain why the 'Network' prefis is used, but it sounds a bit confusing.

What about usig some form of 'Sidebar'?

Comment 14

[[e-mail]]

I suggest use

Network Detail Panel -> InspectorPanel (since all contained panels are from HTTP inspectors)
Detail Panel -> TabsPanel (inspector/ use inspectorTabPanel as the container of all panels)

It's fine for me if we keep NetworkDetailPanel and renamed DetailsPanel

Comment 15

[Ricky Chien [:rickychien] (inactive)]

(In reply to Jan Honza Odvarko [:Honza] from comment #13) 
> Now, I have noticed that we have 'NetworkDetailsPanel' and 'DetailsPanel',
> which might explain why the 'Network' prefis is used, but it sounds a bit
> confusing.
> 
> What about usig some form of 'Sidebar'?

I renamed NetworkDetailsPanel from SidebarView because in case it becomes a inline panel instead of sidebar, so I prefer to drop the sidebar keyword.

After offline discussion with Fred, we like the name - InspectorPanel. Otherwise, we can still use NetworkDetailsPanel but change the current DetailsPanel to TabsPanel / TabboxPanel in order to distinguish between them.

BTW, chromium is using network-details-view as its component name.

Comment 16

[Ricky Chien [:rickychien] (inactive)]

I'm going to keep using NetworkDetailPanel and rename DetailsPanel to TabboxPanel (derived from XUL tabbox which we used before) if there is no objections. And also I don't have to rename all NetworkDetailPanel relevant changes again which I've modified in WIP patch.

Comment 17

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

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

Comment 18

[Ricky Chien [:rickychien] (inactive)]

(In reply to Jan Honza Odvarko [:Honza] from comment #13)
> Couple of UI issues:
> 
> 1) I don't see the toggle button in the UI. Clicking on the empty spot
> causes an error: Console Service ERROR [JavaScript Error: "TypeError:
> Actions.toggleSidebar is not a function" {file:
> "resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/client/netmonitor/components/toolbar.js" line: 154}]
> [JavaScript Error: "TypeError: Actions.toggleSidebar is not a function"
> {file: "resource://gre/modules/commonjs/toolkit/loader.js ->
> resource://devtools/client/netmonitor/components/toolbar.js" line: 154}]

Fixed.

> 2) When custom request form is opened it has different than the original
> side panel. It should be the same (caused by removing <deck> I guess). This
> might be also problem of the CustomRequestPanel component.

Fixed. My previous patch forgot to read / save width and heigh from Prefs. I added a workaround for it and it should be removed in bug 1309183 (SplittBox). Anyway, my suggestion is that width and height is supposed to memorize in SplittBox component IIUC.

BTW, due to renaming changes, I modified a bunch of tests so this patch looks very huge.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

In this patch I simply drop the animation of toggle pane (ViewHelpers) for these reasons:

1. It might cause potential performance issue while running animation. waterfall timeline will reflow constantly due to width changing.
2. I'm seeing there are UI consistency between inspector and debugger. The inspector supports animation but debugger is not. Is there any reason why debugger decided to drop this feature?
3. If we decided to support animation, it should be done in SplitBox (bug 1309183).

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

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

Comment 21

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

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

https://reviewboard.mozilla.org/r/109466/#review111950

> In this patch I simply drop the animation of toggle pane (ViewHelpers) for these reasons:

I see, ok for me (I am not big fan of the animation either).


> I'm going to keep using NetworkDetailPanel and rename DetailsPanel to TabboxPanel

Nice, this is step in the right direction!
I would yet remove the word "Network" since it doesn't have any value in context of the "Network" panel (i.e. every object we have could use "Network" prefix, which is obviously useless). But it's fine, I am not going to block on this. We can discuss later if needed.

R+ assuming try is gren

Thanks!

Honza

Comment 22

[Ricky Chien [:rickychien] (inactive)]

(In reply to Jan Honza Odvarko [:Honza] from comment #21)
> I would yet remove the word "Network" since it doesn't have any value in
> context of the "Network" panel (i.e. every object we have could use
> "Network" prefix, which is obviously useless). But it's fine, I am not going
> to block on this. We can discuss later if needed.

Agree. In fact, I'm in favor of your opinion to drop the "Network" prefix. I tried to rename "panes-network-details-width" and "panes-network-details-height" in prefs.js but it doesn't work and threw an error in devtools/client/shared/prefs.js (I don't know why)

If anyone come out with a better name, we can deal with this naming problem later in a separate bug. 

> R+ assuming try is gren

Thanks! try is green. I'm going to land this as soon as bug 1308449 gets landed.

Comment 23

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

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

Comment 24

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8833231 [details]
Bug 1317645 - Implement NetworkDetailsPanel

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/085735f8d2ff
Implement NetworkDetailsPanel r=Honza

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/085735f8d2ff

Comment 30

[Ciprian Georgiu, Desktop QA]

This issue is verified fixed with latest Nightly 54.0a1 (2017-02-22) across platforms: 
- Windows 10 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS
I can confirm that Network details panel is working as expected.