Bug 174746 - Web Inspector: Inline multiple console log values if they are simple
Summary: Web Inspector: Inline multiple console log values if they are simple
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-21 21:56 PDT by Joseph Pecoraro
Modified: 2017-07-25 16:31 PDT (History)
7 users (show)

See Also:


Attachments
[IMAGE] Before (168.52 KB, image/png)
2017-07-21 21:56 PDT, Joseph Pecoraro
no flags Details
[IMAGE] After (156.63 KB, image/png)
2017-07-21 21:56 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (10.00 KB, patch)
2017-07-21 22:41 PDT, Joseph Pecoraro
mattbaker: review-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (11.64 KB, patch)
2017-07-24 11:15 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (965.93 KB, application/zip)
2017-07-24 12:18 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-07-21 21:56:25 PDT
Inline multiple console log values if they are simple.

Common cases like:

    console.log("message", 100, 200);
    console.log("event", event.type, event.pageX, event.pageY, event);

Result in huge amounts of wasted space:

    ▼ message (2)
        • 100
        • 200

    ▼ event (3)
        • "click"
        • 337
        • 158
        • ▶︎ MouseEvent {...}

We should condense this to one line where possible (leading primitive values):

    ▶︎ message – 100 – 200
    ▶︎ event – "click" – 337 – 158 – MouseEvent {...}
Comment 1 Joseph Pecoraro 2017-07-21 21:56:42 PDT
Created attachment 316169 [details]
[IMAGE] Before
Comment 2 Joseph Pecoraro 2017-07-21 21:56:52 PDT
Created attachment 316170 [details]
[IMAGE] After
Comment 3 Radar WebKit Bug Importer 2017-07-21 21:57:21 PDT
<rdar://problem/33469376>
Comment 4 Joseph Pecoraro 2017-07-21 22:41:54 PDT
Created attachment 316174 [details]
[PATCH] Proposed Fix
Comment 5 Devin Rousso 2017-07-22 02:45:12 PDT
Comment on attachment 316174 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=316174&action=review

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:467
> +            if (simpleParametersCount) {

This check is unnecessary.  In the case that there are no simple parameters, `splice(0, simpleParametersCount)` will return an empty array, which will not be iterated in the loop below.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:473
> +                    let enclosedElement = document.createElement("span");
> +                    builderElement.append(enclosedElement);

NIT: combine these:

    let enclosedElement = builderElement.appendChild(document.createElement("span"));

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:475
> +                    enclosedElement.textContent = " \u2013 ";

NIT: you can use ` ${enDash} ` to get the dash character.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:476
> +                    enclosedElement.classList.add("inline-lossless");

Merge this with the previous classList.add:

    enclosedElement.classList.add("console-message-preview-divider", "inline-lossless");

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:479
> +                    previewContainer.classList.add("console-message-preview"); // FIXME: Needed? I think we don't want this.

Does this need to be fixed?  If not, please merge the classList.add.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:484
> +

Style: remove extra space.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:486
> +                        preview.setOriginatingObjectInfo(parameter, null);

Style: the `null` is unnecessary.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:489
> +                    let previewElement = isPreviewView ? preview.element : preview;
> +                    previewContainer.appendChild(previewElement);

Style: this could be inlined.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:501
> +                let enclosedElement = document.createElement("span");

You might want to fix some of the same styling issues as above.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:526
> +                let enclosedElement = document.createElement("span");

Ditto.
Comment 6 Matt Baker 2017-07-22 11:44:53 PDT
Comment on attachment 316174 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=316174&action=review

In my brief testing this already feels like a big improvement. A couple of issues:

1. A leading simple parameter becomes bottom aligned after expanding an object immediately following:

> console.log(123, document.body)
[Log] ▶ No message – 123 – ▶ <body>…</body>

After expanding `document.body`:

                         ▼ <body>
                             <div>...</div>
                             <div>...</div>
                             <div>...</div>
[Log] ▶ No message – 123 – </body>


2. When leading parameters are simple, the message's disclosure triangle seems unnecessary:

> console.log(123, document.body)
[Log] ▶ No message – 123 – ▶ <body>…</body>

After expanding:
[Log] ▼ No message – 123
        • ▶ <body>…</body>

Is there a more complicated case where this would be useful? It seems like the Console is enforcing an interpretation of log messages wherein the first parameter is assumed to be a title (or format string), and subsequent parameters are either substitution parameters or children of the message.This seems like a big assumption, and sometimes it creates unexpected results:

> console.log(1, 2, 3)
[Log] No message – 1 – 2 – 3

If `shouldFormatWithStringSubstitution` is false, maybe we should just move on to the remaining parameters. Thoughts?

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:475
> +                    enclosedElement.textContent = " \u2013 ";

Utilities.js:27:

var enDash = "\u2013";

> Source/WebInspectorUI/UserInterface/Views/FormattedValue.js:35
> +    case "string":

Nit: sort the group of fall-through cases:

case "boolean":
case "number":
case "string":
case "symbol":
case "undefined":
Comment 7 Joseph Pecoraro 2017-07-24 11:03:02 PDT
(In reply to Matt Baker from comment #6)
> Comment on attachment 316174 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316174&action=review
> 
> In my brief testing this already feels like a big improvement. A couple of
> issues:
> 
> 1. A leading simple parameter becomes bottom aligned after expanding an
> object immediately following:
> 
> > console.log(123, document.body)
> [Log] ▶ No message – 123 – ▶ <body>…</body>
> 
> After expanding `document.body`:
> 
>                          ▼ <body>
>                              <div>...</div>
>                              <div>...</div>
>                              <div>...</div>
> [Log] ▶ No message – 123 – </body>

This is pre-existing and unrelated to my changes.


> 2. When leading parameters are simple, the message's disclosure triangle
> seems unnecessary:
> 
> > console.log(123, document.body)
> [Log] ▶ No message – 123 – ▶ <body>…</body>
> 
> After expanding:
> [Log] ▼ No message – 123
>         • ▶ <body>…</body>

This is pre-existing as well.

FWIW, these issues only exist when you log an element at the end because elements produce their own DOMTreeOutline instead of ObjectTreeViews like other things. Try this instead:

    console.log(123, window);


> Is there a more complicated case where this would be useful? It seems like
> the Console is enforcing an interpretation of log messages wherein the first
> parameter is assumed to be a title (or format string), and subsequent
> parameters are either substitution parameters or children of the
> message.This seems like a big assumption, and sometimes it creates
> unexpected results:
> 
> > console.log(1, 2, 3)
> [Log] No message – 1 – 2 – 3

This is pre-existing as well.

We can decide to drop the "No message" part, but lets do that in a different bug. For years we have been treating multiple arguments console.log like printf / NSLog. The first parameter is expected to be a format string. This encourages behavior like:

    console.log("before", Date.now());
    ...
    console.log("after", Date.now();

However, we do not have good data on how people use console.log. Its entirely possible that many people don't even know you can provide multiple arguments. To that end we have always supported logging a single value without outputting "No Message".
Comment 8 Joseph Pecoraro 2017-07-24 11:15:54 PDT
Created attachment 316303 [details]
[PATCH] Proposed Fix
Comment 9 Build Bot 2017-07-24 12:18:52 PDT
Comment on attachment 316303 [details]
[PATCH] Proposed Fix

Attachment 316303 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4179779

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html
Comment 10 Build Bot 2017-07-24 12:18:54 PDT
Created attachment 316307 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 11 Joseph Pecoraro 2017-07-24 13:26:34 PDT
Comment on attachment 316303 [details]
[PATCH] Proposed Fix

Test failure is unrelated to this change.
Comment 12 Matt Baker 2017-07-25 16:03:45 PDT
Comment on attachment 316303 [details]
[PATCH] Proposed Fix

r=me
Comment 13 WebKit Commit Bot 2017-07-25 16:31:23 PDT
Comment on attachment 316303 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 316303

Committed r219893: <http://trac.webkit.org/changeset/219893>
Comment 14 WebKit Commit Bot 2017-07-25 16:31:24 PDT
All reviewed patches have been landed.  Closing bug.