[webkit-reviews] review denied: [Bug 174746] Web Inspector: Inline multiple console log values if they are simple : [Attachment 316174] [PATCH] Proposed Fix

Sat Jul 22 11:44:53 PDT 2017

Matt Baker <mattbaker at apple.com> has denied Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 174746: Web Inspector: Inline multiple console log values if they are
simple
https://bugs.webkit.org/show_bug.cgi?id=174746

Attachment 316174: [PATCH] Proposed Fix

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

--- Comment #6 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 316174
  --> https://bugs.webkit.org/attachment.cgi?id=316174
[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":