1302982 - Reps does not handle long strings

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe]]

STR:
1. Create a global variable in the console `longString = Array(10000).fill("a").join("");`
2. Go to the DOM Panel and search for the `longString` variable

ER:
longString value is displayed as a truncated string `"aaaaaaaaaaaaaaaaaaaaaaaaa…aaaaaaaaaaaaaaaaaaaaaaaa""aaaaaaaaaaaaaaaaaaaaaaaaa…aaaaaaaaaaaaaaaaaaaaaaaa"`

AR:
longString value is `Object`, without properties

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

What is happening is that when we create a large string, we get a grip which type is `longString`.
The longString grip is an object with the following form : 

  {
    type: "longString",
    initial: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...",
    length: 10000,
    actor: "server1.conn1.child1/longString41"
  }

The initial property seems to contain the first thousand character of the complete string.
I wonder if we could simply use this initial property or if we should retrieve the complete string (like it's done here for example http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/client.js#623 ).

It seems a bit of an overkill for me to do another server round-trip (even if we could do so only if initial.length < length ) because we will then truncate the string in the middle. 
But in the other hand, if we only work with the initial property, we will only check the first thousand characters, and we will display an inaccurate truncation ( if longString is for example 5000 "aa" and then 5000 "bb" , we would display "aa...aa" , which is not correct).

What do you think of this Honza ?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Something I just saw, in the DOM panel, if you click on the truncated string, we show the complete string. If we only work with the `initial` property, we won't get the real full string.

Comment 3

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

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)
> What is happening is that when we create a large string, we get a grip which
> type is `longString`.
> The longString grip is an object with the following form : 
> 
>   {
>     type: "longString",
>     initial: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa...",
>     length: 10000,
>     actor: "server1.conn1.child1/longString41"
>   }
> 
> The initial property seems to contain the first thousand character of the
> complete string.
> I wonder if we could simply use this initial property or if we should
> retrieve the complete string (like it's done here for example
> http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/
> client.js#623 ).

I think we should just use the initial property and display the '...' at the end - since that's how the string is truncated and received from the backend in the first place. We might also change/customize the back-end to truncate the string in the middle (to correspond to what we want to show in the UI).

> 
> It seems a bit of an overkill for me to do another server round-trip (even
Totally agree, that's why I think the back-end should cooperate more with the front-end and truncate long strings the same way (as needed).


(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)
> Something I just saw, in the DOM panel, if you click on the truncated
> string, we show the complete string. If we only work with the `initial`
> property, we won't get the real full string.
Yes, the DOM panel UI/UX is built on top of how the backend works. The user sees only the `initial` string initially a and if it isn't enough an extra user action (click to expand) is required.

The HTTPi UI/UX in the Console panel works similarly for responses (HTTP response is also long string). If the user expands HTTP request in the Console panel and switches into the Response tab, only `initial` part of the response is visible + there is a 'more...' link (at the end) that, if clicked, resolves the long string and shows the rest of the response.

I think we should always try (if possible) to come up with UI/UX that requires and extra action from the user to avoid automatic string-resolving initiating new round-trip. This auto-resolving kind of breaks the idea of having the long-string in the first place.

Honza

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1302982 - Add Rep for LongString;

Display a longString with what is available on the grip.
Straight from the server, a longString grip has a "initial"
property, which contains the first 10000 chars of the string.
The grip can also contains a "fullText" property that should
be set when the user want to display the full text of the string.
For example, in the DOM Panel, it would be when the user click on
the expand arrow.
The fullText needs to be computed with the result of the
LongStringClient.substring function, but it's not the responsability
of the Reps to do such thing.

In rep-utils, we extract the logic that replace non-printable
characters in its own function and export it, so we can use it outside
the cropString function.

Add some test to make sure LongString are displayed as expected in the
several modes.

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

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Honza the patch for review is the following of https://reviewboard.mozilla.org/r/88384/ ,  it's better to have it here, the other bug is more console specific since we should add a way to expand a longString in it.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

TRY looks fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=d82aafceed07

Comment 7

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

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> Honza the patch for review is the following of
> https://reviewboard.mozilla.org/r/88384/ ,

Hm..., the patch seems to be discarded. Is it correctly pushed to mozreview?

Honza

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

This one is fine isn't it https://reviewboard.mozilla.org/r/90458/ ?
The one I mentioned in comment 5 was on another bug that wasn't best suited for it.

Comment 9

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

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> Honza the patch for review is the following of
> https://reviewboard.mozilla.org/r/88384/ ,  it's better to have it here, the
> other bug is more console specific since we should add a way to expand a
> longString in it.

I made the review, but of course the result went to bug 1310630

So, the expandability will be part of bug 1310630 ?

Honza

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Thanks for the review Honza

> I made the review, but of course the result went to bug 1310630 

wow, that's so weird, my bad for messing this up in the first place

> So, the expandability will be part of bug 1310630 ?

For the console, yes, and there is Bug 1314923 for the DOM Panel too.

Comment 11

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

Comment on attachment 8807231 [details]
Bug 1302982 - Add Rep for LongString;

https://reviewboard.mozilla.org/r/90458/#review91580

R+ this patch, again assuming try is green.

Honza

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8807231 [details]
Bug 1302982 - Add Rep for LongString;

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

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

TRY looks fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bdc33520e60dc6fac9100aca7973366983962ef

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ffa818f1cb3
Add Rep for LongString; r=Honza

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5ffa818f1cb3