1350234 - Move stack trace tooltip to network details panel (sidebar panel)

Status: VERIFIED FIXED

(DevTools :: Netmonitor, enhancement, P1)

Description

[[e-mail]]

stack trace are now work as tooltips, we'd like to move them as a new sidebar panel

Comment 1

[[e-mail]]

This issue needs some understanding of netmonitor's structure, so its more suitable as the good-second-bug.

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1350234 - Add stack trace panel.

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

Comment 3

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8852482 [details]
Bug 1350234 - Add stack trace panel.

https://reviewboard.mozilla.org/r/124692/#review127212

Cool! we get rid of stack trace tooltip! I like that patch.

There are some nits please address it before landing the patch.

Thanks!

::: devtools/client/netmonitor/shared/components/stack-trace-panel.js:1
(Diff revision 1)
> +const {
> +  createClass,
> +  createFactory,
> +  DOM,
> +  PropTypes,
> +} = require("devtools/client/shared/vendor/react");
> +const { div, span } = DOM;
> +const StackTrace = createFactory(require("devtools/client/shared/components/stack-trace"));

nit: we have a loose convention about module import order here

shared libaries -> project utils -> DOM alias -> commponents

```js
const {
  createClass,
  createFactory,
  DOM,
  PropTypes,
} = require("devtools/client/shared/vendor/react");

const { div, span } = DOM;

// Components
const StackTrace = createFactory(require("devtools/client/shared/components/stack-trace"));
```

::: devtools/client/netmonitor/shared/components/stack-trace-panel.js:10
(Diff revision 1)
> +  PropTypes,
> +} = require("devtools/client/shared/vendor/react");
> +const { div, span } = DOM;
> +const StackTrace = createFactory(require("devtools/client/shared/components/stack-trace"));
> +
> +const StackTracePanel = createClass({

nit: this component looks like a simple presentational logic.

We can get rid of createClass but use funtional component instead.

See https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/components/network-monitor.js

::: devtools/client/netmonitor/shared/components/tabbox-panel.js:25
(Diff revision 1)
> +const StackTracePanel = createFactory(require("./stack-trace-panel"));
>  const SecurityPanel = createFactory(require("./security-panel"));

nit: Alphabetical order.

it should be

SecurityPanel = ...
StackTracePanel = ...

::: devtools/client/netmonitor/shared/components/tabbox-panel.js:34
(Diff revision 1)
>  const HEADERS_TITLE = L10N.getStr("netmonitor.tab.headers");
>  const COOKIES_TITLE = L10N.getStr("netmonitor.tab.cookies");
>  const PARAMS_TITLE = L10N.getStr("netmonitor.tab.params");
>  const RESPONSE_TITLE = L10N.getStr("netmonitor.tab.response");
>  const TIMINGS_TITLE = L10N.getStr("netmonitor.tab.timings");
> +const STACK_TRACE_TITLE = L10N.getStr("netmonitor.tab.stack-trace");

like wise.

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8852482 [details]
Bug 1350234 - Add stack trace panel.

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

Comment 5

[Tim Nguyen :ntim]

Attachment: Bug 1350234 - Clean up network monitor CSS.

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

Comment 6

[Tim Nguyen :ntim]

Comment on attachment 8852482 [details]
Bug 1350234 - Add stack trace panel.

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

Comment 7

[Tim Nguyen :ntim]

Comment on attachment 8852656 [details]
Bug 1350234 - Clean up network monitor CSS.

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

Comment 8

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

@ntim: thanks for working on this!

Can you please rebase?
The dir structure changed a bit in bug 1350215

Honza

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8852482 [details]
Bug 1350234 - Add stack trace panel.

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

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8852656 [details]
Bug 1350234 - Clean up network monitor CSS.

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

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8852656 [details]
Bug 1350234 - Clean up network monitor CSS.

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

Comment 12

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

Attachment: stack-trace.png

Great progress here!

One comment:

* Frame URL in the Stack panel gets trimmed even if there is enough space in the side bar. See the attached screenshot.

Honza

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8852482 [details]
Bug 1350234 - Add stack trace panel.

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

Comment 14

[Tim Nguyen :ntim]

Comment on attachment 8852656 [details]
Bug 1350234 - Clean up network monitor CSS.

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

Comment 15

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

The issue from comment #12 is fixed now, thanks!

Honza

Comment 16

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

Comment on attachment 8852482 [details]
Bug 1350234 - Add stack trace panel.

https://reviewboard.mozilla.org/r/124692/#review128558

::: devtools/client/locales/en-US/netmonitor.properties:458
(Diff revision 5)
>  # in the network details pane identifying the timings tab.
>  netmonitor.tab.timings=Timings
>  
> +# LOCALIZATION NOTE (netmonitor.tab.stack-trace): This is the label displayed
> +# in the network details pane identifying the stack-trace tab.
> +netmonitor.tab.stack-trace=Stack Trace

Please use camelCase so, it's consistent with the existing keys.

netmonitor.tab.stack-trace => netmonitor.tab.stackTrace

::: devtools/client/netmonitor/src/components/tabbox-panel.js:35
(Diff revision 5)
>  const PARAMS_TITLE = L10N.getStr("netmonitor.tab.params");
> +const PREVIEW_TITLE = L10N.getStr("netmonitor.tab.preview");
>  const RESPONSE_TITLE = L10N.getStr("netmonitor.tab.response");
> -const TIMINGS_TITLE = L10N.getStr("netmonitor.tab.timings");
>  const SECURITY_TITLE = L10N.getStr("netmonitor.tab.security");
> -const PREVIEW_TITLE = L10N.getStr("netmonitor.tab.preview");
> +const STACK_TRACE_TITLE = L10N.getStr("netmonitor.tab.stack-trace");

Should be: netmonitor.tab.stackTrace

Comment 17

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

Comment on attachment 8852656 [details]
Bug 1350234 - Clean up network monitor CSS.

https://reviewboard.mozilla.org/r/124838/#review128564

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css:723
(Diff revision 5)
> +  width: 100%;
> +  overflow: auto;
> +  flex: 1;
> +}
> +
> +.tree-container .treeTable, 

nit: remove whitespace

Comment 18

[Tim Nguyen :ntim]

Comment on attachment 8852482 [details]
Bug 1350234 - Add stack trace panel.

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

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8852656 [details]
Bug 1350234 - Clean up network monitor CSS.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c08f896d669
Add stack trace panel. r=Honza
https://hg.mozilla.org/integration/autoland/rev/1379bfa0f921
Clean up network monitor CSS. r=Honza

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2c08f896d669
https://hg.mozilla.org/mozilla-central/rev/1379bfa0f921

Comment 22

[Ciprian Georgiu, Desktop QA]

I verified this issue using latest Nightly 55.0a1 (2017-04-10) as fixed on Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.11.6.

Stack Trace tab is properly displayed in the Details view panel.