1304804 - Don't set a default cropLimit for reps

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Brian Grinstead [:bgrins]]

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 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1304804 - Don't set a default cropLimit for reps;

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

Comment 2

[Brian Grinstead [:bgrins]]

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.

Comment 3

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

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

Comment 4

[Brian Grinstead [:bgrins]]

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

Comment 5

[Brian Grinstead [:bgrins]]

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

Comment 6

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

(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

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8793884 [details]
Bug 1304804 - Don't set a default cropLimit for reps;

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c5d6e4be64b0
Don't set a default cropLimit for reps;r=Honza

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c5d6e4be64b0