Don't set a default cropLimit for reps

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

unspecified
Firefox 52
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

3 years ago
Having the default value of 50 makes it difficult to consistently show uncropped strings in the console frontend.  I think it'd be better if we showed uncropped by default and then let consumers choose to crop if they want.
Comment hidden (mozreview-request)
Assignee

Comment 2

3 years ago
The idea here is that cropping can be handled by longString but there shouldn't be a frontend only limit set.  There's an alternative where we could prevent cropping in the console with the cropLimit option, but it might be hard to propagate that all the way through nested objects.
Blocks: 1304178
Comment on attachment 8793884 [details]
Bug 1304804 - Don't set a default cropLimit for reps;

https://reviewboard.mozilla.org/r/80482/#review79704

OK for me, r+. But, please make sure the existing
UI is not broken (DOM panel, JSONv, HTTPi)

Honza
Attachment #8793884 - Flags: review?(odvarko) → review+
Assignee

Updated

3 years ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Assignee

Comment 5

3 years ago
AFAICT the json viewer doesn't crop strings with or without the patch (testing on https://bgrins.github.io/devtools-demos/misc/file.json).

For DOM panel, you can run this in the console: `window.foo = new Array(500).join("a a")` then search for foo in the DOM panel.  This is different with and without the patch (all of the string is visible with it applied as opposed to 50 chars).  Same with network response / post in console.  Not sure if it's 'broken', but I can readd a cropLimit in these cases [1][2][3] if you think it should be there.  Question: will this be handled automatically be long string support in https://bugzilla.mozilla.org/show_bug.cgi?id=1302982?  I'm not sure if we'll need two different ways for showing short strings once that's added.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/dom/content/components/dom-tree.js#63
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/net/components/post-tab.js#80
[3]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/net/components/response-tab.js#108
Flags: needinfo?(odvarko)
(In reply to Brian Grinstead [:bgrins] from comment #5)
> AFAICT the json viewer doesn't crop strings with or without the patch
> (testing on https://bgrins.github.io/devtools-demos/misc/file.json).
When I open the file I see a`longString` field (at the bottom)
expanded by default (string fully visible). Collapsing that string
should crop it, but it doesn't.

This should be fixed as follows. The return statement here:
https://dxr.mozilla.org/mozilla-central/rev/29beaebdfaccbdaeb4c1ee5a43a9795ab015ef49/devtools/client/jsonview/components/json-panel.js#96

should be something like:

return Rep(Object.assign({}, props, {
  cropLimit: 50,
}));


> 
> For DOM panel, you can run this in the console: `window.foo = new
> Array(500).join("a a")` then search for foo in the DOM panel.  This is
> different with and without the patch (all of the string is visible with it
> applied as opposed to 50 chars).
Yes, [1] should be fixed (cropLimit prop added into the Rep ctor).


> Same with network response / post in
> console.  Not sure if it's 'broken', but I can readd a cropLimit in these
> cases [1][2][3] if you think it should be there.
Yes, [2] and [3] should also be fixed (cropLimit prop added into the Rep ctor).
It fixes JSON preview of HTTP post and response bodies.

> Question: will this be
> handled automatically be long string support in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1302982?  I'm not sure if we'll
> need two different ways for showing short strings once that's added.
Having just one way to crop strings would definitely be better.
I am not sure yet how bug 1302982 will be resolved, but I have at least two
concerns:

1) The backend should cooperate more with the front end i.e. provide properly
cropped strings (in the middle and/or at the and).
See also my comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=1302982#c3

2) Long string has hard-coded limit for 1024 chars. I am worried if this isn't too
long in some cases. For example Watch panel in the Debugger doesn't have much
space and it might require different limit for string values (e.g. 50, empirically
calculated in Firebug).

I think that front end should be able to somehow specify/customize the form of
the returned string, both: length and cropping style. Where length could be
a specified using a mode e.g. tiny, short, long (instead of various adhoc numbers).

I would just fix this bug and discuss more in bug 1302982.

Honza



> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/dom/content/
> components/dom-tree.js#63
> [2]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/
> net/components/post-tab.js#80
> [3]:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/
> net/components/response-tab.js#108
Flags: needinfo?(odvarko)
Comment hidden (mozreview-request)

Comment 8

3 years ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c5d6e4be64b0
Don't set a default cropLimit for reps;r=Honza

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c5d6e4be64b0
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

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