Closed Bug 1317645 Opened 8 years ago Closed 7 years ago

Implement NetworkDetailsPanel

Categories

(DevTools :: Netmonitor, defect, P1)

Product:
DevTools
Component:
Netmonitor
Type:
defect

Tracking

(firefox54 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Iteration:
54.2 - Feb 20
Tracking Flags:
Tracking Status
firefox54 --- verified

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file, 1 obsolete file)

Bug 1317645 - Implement NetworkDetailsPanel
59 bytes, text/x-review-board-request
Honza
: review+
Details 
We're going to keep sidebar UI as is, so sidebar view a.k.a network details view will be converted into react.
Depends on: 1317648
Depends on: 1317649
Depends on: 1317650
Depends on: 1317651
Depends on: 1309187
Depends on: 1309188
Depends on: 1317659
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Priority: -- → P2
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor]
should refer inspector/toolsidebar.js to replace ToolSidebar implemented in devtools/client/framework/sidebar.js module
Priority: P2 → P1
Depends on: 1321162
Depends on: 1308449
Priority: P1 → P2
Depends on: 1325914
Depends on: 1328197
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
Summary: Implement sidebar view (network details view) → Implement sidebar view
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Iteration: --- → 53.5 - Jan 23
Priority: P2 → P1
Iteration: 53.5 - Jan 23 → 54.1 - Feb 6
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.
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.
Summary: Implement sidebar view → Implement NetworkDetailsPanel
Blocks: 1309183
Patch is ready and time to ask first round review. I'll ask second round review after bug 1308449 is landed.
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20
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 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);
```
Attachment #8833230 - Attachment is obsolete: true
Rebased patch on top of latest CustomRequestPanel.
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'?
Attachment #8833231 - Flags: review?(odvarko) → review-
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
(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.
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.
(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.
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 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
Attachment #8833231 - Flags: review?(odvarko) → review+
(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.
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/085735f8d2ff
Implement NetworkDetailsPanel r=Honza
https://hg.mozilla.org/mozilla-central/rev/085735f8d2ff
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: qe-verify+
Depends on: 1343079
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: