Wrap long request parameter values in the netmonitor sidebar

RESOLVED FIXED in Firefox 57

Status

()

Firefox
Developer Tools: Netmonitor
P2
normal
RESOLVED FIXED
9 months ago
3 months ago

People

(Reporter: pbro, Assigned: Abhinav Koppula, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 57
good-first-bug
Points:
---

Firefox Tracking Flags

(platform-rel +, firefox57 fixed)

Details

(Whiteboard: [platform-rel-Zalando])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 months ago
STR:

- Open the following URL with the netmonitor opened:
https://www.google.fr/search?q=this%20is%20a%20very%20long%20request%20parameter%20that%20will%20need%20to%20wrap%20on%20multiple%20lines

- Click on the first search request in the panel

- Switch to the Params sidebar tab

I see 2 problems in Nightly now:
- even if the panel is big enough to display the full value for the parameter 'q', it has a middle ellipsis
- when the panel is too narrow, instead of using an ellipsis, I think it would be better to wrap the value than partly hide it
platform-rel: --- → +
Whiteboard: [platform-rel-Zalando]
Priority: -- → P2
Mentor: odvarko@gmail.com
Keywords: good-first-bug

Comment 1

8 months ago
I can give it a go if you share some hints.

Comment 2

8 months ago
This is the first time I ever look at the devtools code so I apologize in advance for stupid questions :)

I did a very quick tour: I understand that ParamsPanel in client/netmonitor/src/components/params-panel.js returns the full string. So I understand the "formatting" takes place elsewhere, but I am not sure where.

Comment 3

8 months ago
Hey Vangelis,
I have also looked into it. It seems like the magic happens here:
devtools/client/netmonitor/src/components/properties-view.js on line 211: renderValue: renderValue || this.renderValueWithRep

Because params-panel.js does not specify a renderValue function, it is set by the PropertiesView render method when calling TreeView. 

The renderValueWithRep uses the 'Rep' class to render any kind of objects as it seems to me. (devtools/client/shared/components/reps/reps)

The question for me now is: should the ParamsPanel define its own 'renderValue' function when calling PropertiesView.
(Assignee)

Comment 4

3 months ago
Created attachment 8903960 [details] [diff] [review]
bz-1347833-params-panel.patch

Hi Patrick, Jan
I have taken a stab at this issue.

1. The changes in params-panel.js and properties-view.js are to pass an optional parameter - disableCropLimit - which ensures that the cropLimit of 60 is not applied in Rep component. Since this is being used in the Cookie panel also, I thought it would be better for the panel to pass on the flag to enable/disable crop limit.

2. The change in netmonitor.css is to fix the word-wrapping on panel resize.

Please let me know if I am on the right track.
Attachment #8903960 - Flags: review?(pbrosset)
Attachment #8903960 - Flags: review?(odvarko)
(Reporter)

Updated

3 months ago
Assignee: nobody → abhinav.koppula
Status: NEW → ASSIGNED
(Reporter)

Comment 5

3 months ago
Comment on attachment 8903960 [details] [diff] [review]
bz-1347833-params-panel.patch

I'll let Honza do the review on this one. He knows the code far better than I do, and just 1 review will be enough for this I think.
Thanks for fixing this!
Attachment #8903960 - Flags: review?(pbrosset)
Comment on attachment 8903960 [details] [diff] [review]
bz-1347833-params-panel.patch

Review of attachment 8903960 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this!

Please see my inline comment.

I really like this feature and I think we could use the same approach for other panels: Headers & Cookies. Probably in a follow up bug report.

Honza

::: devtools/client/netmonitor/src/components/properties-view.js
@@ +126,5 @@
> +      cropLimit: 60
> +    };
> +
> +    if (this.props.disableCropLimit) {
> +      delete source.cropLimit;

It should be possible to set `cropLimit` to -1 to disable it. So, we can do it instead of introducing a new property `disableCropLimit`

You can see also, the implementation here:
https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-reps/src/reps/rep-utils.js#L144

Also, we might want to keep that limit, but a lot bigger. What about 1024? Just to avoid extreme cases.


Honza
Attachment #8903960 - Flags: review?(odvarko) → review-
Comment on attachment 8903960 [details] [diff] [review]
bz-1347833-params-panel.patch

Review of attachment 8903960 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/netmonitor/src/components/properties-view.js
@@ +122,2 @@
>        // FIXME: A workaround for the issue in StringRep
>        // Force StringRep to crop the text everytime

nit: typo everytime => every time
(Assignee)

Comment 8

3 months ago
Created attachment 8905281 [details] [diff] [review]
bz-1347833-params-panel-updated.patch

Hi Honza,
Thanks for the review.
I have incorporated the changes which you mentioned. 
I hope my understanding was correct.
Attachment #8905281 - Flags: review?(odvarko)
Attachment #8903960 - Attachment is obsolete: true
Comment on attachment 8905281 [details] [diff] [review]
bz-1347833-params-panel-updated.patch

Review of attachment 8905281 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this!

See my inline comments.

Also:

1) Append `cropLimit: PropTypes.number,` into `propTypes` in PropertiesView
2) Append `cropLimit: 1024,` into `getDefaultProps` in PropertiesView

Honza

::: devtools/client/netmonitor/src/assets/styles/netmonitor.css
@@ +785,5 @@
>    overflow-y: auto;
>  }
>  
> +.panel-container .tree-container .objectBox {
> +  white-space: normal;

Append:
word-wrap: break-word;

... for case where there are no spaces in the value.

::: devtools/client/netmonitor/src/components/params-panel.js
@@ +92,4 @@
>          object,
>          filterPlaceHolder: PARAMS_FILTER_TEXT,
>          sectionNames: SECTION_NAMES,
> +        cropLimit: -1

Remove this. Let's use a default value defined in PropertiesView.

::: devtools/client/netmonitor/src/components/properties-view.js
@@ +124,3 @@
>        member: Object.assign({}, member, { open: false }),
>        mode: MODE.TINY,
> +      cropLimit: ('cropLimit' in this.props) ? this.props.cropLimit : 1024

The right way is to define default value so, it's never undefined.

Just do:
cropLimit: this.props.cropLimit,

@@ +150,4 @@
>        renderRow,
>        renderValue,
>        sectionNames,
> +      cropLimit

Why is this here? You should remove it.
Attachment #8905281 - Flags: review?(odvarko) → review-
Comment hidden (mozreview-request)

Comment 11

3 months ago
mozreview-review
Comment on attachment 8906311 [details]
Bug 1347833 - Add support to wrap long request parameters/cookies in the netmonitor sidebar.

https://reviewboard.mozilla.org/r/178038/#review183294

Looks great now, thanks!

R+ assuming try is green.

Btw. I am not sure how you are submitting the patch for review, but the existing one should be updted in the review board, which didn't happen in your case. You might want to read the docs: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1780b29345ddca06617b2002728b5ed8fc58fc65

Honza
Attachment #8906311 - Flags: review?(odvarko) → review+
Attachment #8905281 - Attachment is obsolete: true

Comment 12

3 months ago
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a71dd9ce8a16
Add support to wrap long request parameters/cookies in the netmonitor sidebar. r=Honza

Comment 13

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a71dd9ce8a16
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.