1096289 - Reduce the amount of screen estate occupied by strings in the webconsole

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Patrick Brosset <:pbro>]

This comes from this uservoice idea:
https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/6263603-concise-object-representations-in-the-console

See this screenshot in particular:
http://i.imgur.com/7VGTzh5.png

In the screenshot, one of the console message is an array that contains the string for a function declaration. The function code is displayed fully, which leads to a lot of screen real estate being occupied, probably for not a lot of value.

In Firebug however, as soon as a string is part of an array (or an object I guess), all carriage returns are escaped and the string is cut off in the middle, with an ellipsis. This helps reading the array (or object) content better.

To test this:
- open the webconsole on the current page
- type in the following: [document.body.textContent]
- hit enter

So the idea would be to:
- escape all \n
- define a constant for the maximum string length (or a pref?)
- cut strings that are longer than that in the middle and put an ellipsis in the middle.

Comment 1

[Mrinal Dhar]

Hi, I'd like to work on this bug. Could you please help me get started?

Comment 2

[Patrick Brosset <:pbro>]

Panos, you think you could help Mrinal get started with this bug? I think the changes will have to be done in browser/devtools/webconsole/console-output.js, but I don't have the time just now to dig deeper and give more information.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Hi Mrinal, have you compiled Firefox from source? If not, please see https://developer.mozilla.org/Simple_Firefox_build and direct further questions to #introduction on http://irc.mozilla.org. When you have a build environment prepared, please feel free to start work on this bug immediately, and I will try to answer any specific questions you may have about this task.

Patrick described in a nutshell what needs to be done here in comment 0. console-output.js contains a number of object renderers that are responsible for displaying the contents of JS objects. From the screenshot in comment 0, my guess about the most problematic case would be the "ObjectWithText" kind of Widgets.ObjectRenderers. I bet that |preview.text| in that case should have a smaller limit.

Try to come up with a test case that reproduces the problem first and then see if making this change fixes it. Ping me on irc if you need any help.

Comment 5

[Mrinal Dhar]

Hi, Panos. I'm testing this with the same script as is seen in the screenshot. 

I tried to modify the ObjectWithText ObjectRenderer, as you directed me to it. However, it seems that no matter what I do to this part of the code and re-build the project, the web-console is unaffected by it. What could I be doing wrong?

Comment 6

[Mrinal Dhar]

Updates: 
1. The code that handles the string output on the console, is located in VariablesView.jsm at line 3404.
2. I've modified it to escape all \n, and it now replaces every newline character with a blank space. 
3. What should be the value of the constant for the maximum string length?

Comment 7

[Patrick Brosset <:pbro>]

(In reply to Mrinal Dhar from comment #6)
> 2. I've modified it to escape all \n, and it now replaces every newline
> character with a blank space. 
Just wanted to make sure about one thing: we don't want to do this for all strings, just strings that are output to the console as a property of a owner object or array.

Comment 8

[Mrinal Dhar]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> (In reply to Mrinal Dhar from comment #6)
> > 2. I've modified it to escape all \n, and it now replaces every newline
> > character with a blank space. 
> Just wanted to make sure about one thing: we don't want to do this for all
> strings, just strings that are output to the console as a property of a
> owner object or array.

Oh. Currently all strings are being shortened. I'll make the changes now.

Comment 9

[Mrinal Dhar]

Attachment: Screenshot of Webconsole after modifications

I changed the code to shorten only the strings that are either object properties or elements of an array. Does the console in the screenshot represent what was needed here?

Comment 10

[Patrick Brosset <:pbro>]

(In reply to Mrinal Dhar from comment #9)
> Created attachment 8548688 [details]
> Screenshot of Webconsole after modifications
> 
> I changed the code to shorten only the strings that are either object
> properties or elements of an array. Does the console in the screenshot
> represent what was needed here?
fwiw, I like this a lot!

Comment 11

[Mrinal Dhar]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #10)
> (In reply to Mrinal Dhar from comment #9)
> > Created attachment 8548688 [details]
> > Screenshot of Webconsole after modifications
> > 
> > I changed the code to shorten only the strings that are either object
> > properties or elements of an array. Does the console in the screenshot
> > represent what was needed here?
> fwiw, I like this a lot!

Great :D

Comment 12

[Mrinal Dhar]

Attachment: Patch that shortens strings which are part of object properties or array elements

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8548873 [details] [diff] [review]
Patch that shortens strings which are part of object properties or array elements

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

Nice work! I have a few comments about improving the code a bit, and then we should be good to land this.

::: browser/devtools/webconsole/console-output.js
@@ +2356,5 @@
>      if (valueIsText) {
>        this._text(value);
>      } else {
> +      if (typeof(value)=="string") {
> +        var shortVal = value.replace(/(\r\n|\n|\r)/gm," ");

Use |let| instead of |var| everywhere.

@@ +2358,5 @@
>      } else {
> +      if (typeof(value)=="string") {
> +        var shortVal = value.replace(/(\r\n|\n|\r)/gm," ");
> +        if (shortVal.length>24) {
> +         shortVal = shortVal.substring(0,35)+"...";

Instead of 3 dots we should be using a single ellipsis unicode character instead.

@@ +2361,5 @@
> +        if (shortVal.length>24) {
> +         shortVal = shortVal.substring(0,35)+"...";
> +          }
> +      }
> +      else {

There are various departures from the code style in this change. E.g., "else" goes in the same line with both braces, wrongly indented lines, missing spaces between operators, etc. Please make sure you are using soft-tabs (spaces instead of tabs) and fix these minor issues.

@@ +2362,5 @@
> +         shortVal = shortVal.substring(0,35)+"...";
> +          }
> +      }
> +      else {
> +        var shortVal = value;

Instead of declaring shortVal in both branches, better declare it before the typeof check and assign it to |value|. That way the |else| branch can be omitted.

@@ +2693,5 @@
> +      }
> +      else {
> +        var shortVal = item;
> +      }
> +        let elem = this.message._renderValueGrip(shortVal, { concise: true });

Use a helper function instead of duplicating this code.

::: browser/devtools/webconsole/test/browser_webconsole_output_06.js
@@ +100,5 @@
> +  },
> +
> +  //12 - array with long strings as elements
> +  {
> +    input: '["SHOW\\nALL\\nOF\\nTHIS\\nON\\nA\\nSINGLE\\nLINE ONLY. ESCAPE ALL NEWLINE","SHOW\\nALL\\nOF\\nTHIS\\nON\\nA\\nSINGLE\\nLINE ONLY. ESCAPE ALL NEWLINE","SHOW\\nALL\\nOF\\nTHIS\\nON\\nA\\nSINGLE\\nLINE ONLY. ESCAPE ALL NEWLINE"]',

Lots of unnecessary duplication here and below. Better to declare a const string above and reuse it as many times as you want here.

Comment 14

[Mrinal Dhar]

Attachment: New patch with changes made according to review

Comment 15

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8553523 [details] [diff] [review]
New patch with changes made according to review

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

Thanks for the interdiff, but I need to see the actual final patch as well. It would be preferable when you ask for review in the future to set the review flag on the updated patch and provide an additional interdiff patch to ease the review (it does lead to a faster review!).

You didn't follow one piece of my advice to use a helper function in console-output.js to avoid duplication. Also, can you push the patch to try to see if tests still pass? If not, I will do it for you.

Comment 16

[Mrinal Dhar]

Attachment: Third patch, with changes according to review. Helper function now added.

Hi, Panos. 

I'll keep your suggestions regarding the review process in mind, now onwards.

This one is the complete patch, with all code changes. I have added the helper function, like you asked. 

Since I do not have commit access to the try server, please push the patch to see if the tests pass.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8553689 [details] [diff] [review]
Third patch, with changes according to review. Helper function now added.

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

This is almost ready, I just have a few more minor comments to address. I pushed it to try, so we will be able to see if all test runs are green.

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

::: browser/devtools/webconsole/console-output.js
@@ +1126,5 @@
>      return result;
>    },
>  
> +
> +  shortenValueGrip: function(grip)

Add a method comment to explain that this function shortens string grips, but leaves other kinds of grips unmodified.

@@ +1132,5 @@
> +    let shortVal = grip;
> +    if (typeof(grip)=="string") {
> +      shortVal = grip.replace(/(\r\n|\n|\r)/gm," ");
> +      if (shortVal.length > 24) {
> +        shortVal = shortVal.substring(0,35) + "\u2026";

I don't see the point in treating strings with more than 24 characters as long, but then displaying a string with 35+1 characters.You should compare to a MAX_STRING_GRIP_LENGTH of 36 and then take shortVal.substring(0, MAX_STRING_GRIP_LENGTH - 1). The decrement by one is to account for the added ellipsis.

As I implied above, you should also move the magic number 35 (or whatever you feel is right) to the top of the file as a constant.

Also, there is a better form for the ellipsis character which is more amenable to l10n issues:

https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/breadcrumbs.js#16

breadcrumbs.js also has an example of a constant declaration at the top.

Comment 18

[Mrinal Dhar]

Attachment: Modifications have been made according to comments.

Comment 19

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8553710 [details] [diff] [review]
Modifications have been made according to comments.

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

Okay! Fix the last couple of nits and I'll land this. Remember to add r=past to the commit message, per:

https://developer.mozilla.org/docs/Mercurial_FAQ#Commit_Message_Conventions

::: browser/devtools/webconsole/console-output.js
@@ +1129,5 @@
>      return result;
>    },
>  
>    /**
> +   * Shorten grips of the type string, leaves other grips unmodified. 

Slightly better wording: "Shorten string type grips longer than MAX_STRING_GRIP_LENGTH. Leave other grips unmodified."

Also, remove the trailing whitespace.

::: browser/devtools/webconsole/test/browser_webconsole_output_06.js
@@ +7,5 @@
>  
>  const TEST_URI = "data:text/html;charset=utf8,test for console output - 06";
> +const ELLIPSIS = Services.prefs.getComplexValue("intl.ellipsis", Ci.nsIPrefLocalizedString).data;
> +const test_str_in = "SHOW\\nALL\\nOF\\nTHIS\\nON\\nA\\nSINGLE\\nLINE ONLY. ESCAPE ALL NEWLINE";
> +const test_str_out = "SHOW ALL OF THIS ON A SINGLE LINE O"+ELLIPSIS;

Nit: add spaces around +

Comment 20

[Mrinal Dhar]

Attachment: Fixed final nits

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8553806 [details] [diff] [review]
Fixed final nits

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

No need for extra review request when you get an r+. Just uploading the updated patch will suffice. There is still a trailing whitespace, but I'll take it out myself before landing.

Thanks for your work!

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/853b1e47bc92

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/853b1e47bc92