Implement NetworkDetailsPanel

VERIFIED FIXED in Firefox 54

Status

P1
normal
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: rickychien, Assigned: rickychien)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Firefox 54
Dependency tree / graph

Firefox Tracking Flags

(firefox54 verified)

Details

(Whiteboard: [netmonitor])

Attachments

(1 attachment, 1 obsolete attachment)

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

Updated

2 years ago
Depends on: 1317648
(Assignee)

Updated

2 years ago
Depends on: 1317649
(Assignee)

Updated

2 years ago
Depends on: 1317650
(Assignee)

Updated

2 years ago
Depends on: 1317651
(Assignee)

Updated

2 years ago
Depends on: 1309187
(Assignee)

Updated

2 years ago
Depends on: 1309188
(Assignee)

Updated

2 years ago
Depends on: 1317659

Updated

2 years ago
Flags: qe-verify?

Updated

2 years ago
Flags: qe-verify? → qe-verify+
Priority: -- → P2
QA Contact: ciprian.georgiu
Whiteboard: [netmonitor][triage] → [netmonitor]
Blocks: 1251899

Comment 1

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

Updated

2 years ago
Priority: P2 → P1
(Assignee)

Updated

2 years ago
Depends on: 1321162

Updated

2 years ago
Depends on: 1308449

Updated

2 years ago
Priority: P1 → P2

Updated

2 years ago
Depends on: 1325914
(Assignee)

Updated

2 years ago
Depends on: 1328197
(Assignee)

Comment 2

2 years ago
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)

Updated

2 years ago
Assignee: nobody → rchien

Updated

2 years ago
Status: NEW → ASSIGNED
Iteration: --- → 53.5 - Jan 23
Priority: P2 → P1

Updated

2 years ago
Iteration: 53.5 - Jan 23 → 54.1 - Feb 6

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Blocks: 1309183
(Assignee)

Comment 7

2 years ago
Patch is ready and time to ask first round review. I'll ask second round review after bug 1308449 is landed.
Comment hidden (mozreview-request)

Updated

2 years ago
Iteration: 54.1 - Feb 6 → 54.2 - Feb 20

Comment 9

2 years ago
mozreview-review
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
(Assignee)

Comment 10

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8833230 - Attachment is obsolete: true
Rebased patch on top of latest CustomRequestPanel.

Comment 13

2 years ago
mozreview-review
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-

Comment 14

2 years ago
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.
Comment hidden (mozreview-request)
(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 hidden (mozreview-request)

Comment 21

2 years ago
mozreview-review
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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

2 years ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/085735f8d2ff
Implement NetworkDetailsPanel r=Honza

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/085735f8d2ff
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1337006
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1315610
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1308450
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+
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1251899

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.