174746 – Web Inspector: Inline multiple console log values if they are simple

RESOLVED FIXED 174746

Web Inspector: Inline multiple console log values if they are simple

https://bugs.webkit.org/show_bug.cgi?id=174746

Summary Web Inspector: Inline multiple console log values if they are simple

Joseph Pecoraro

Reported 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 {...}

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Joseph Pecoraro

Comment 1 2017-07-21 21:56:42 PDT

Created attachment 316169 [details] [IMAGE] Before

Joseph Pecoraro

Comment 2 2017-07-21 21:56:52 PDT

Created attachment 316170 [details] [IMAGE] After

Radar WebKit Bug Importer

Comment 3 2017-07-21 21:57:21 PDT

<rdar://problem/33469376>

Joseph Pecoraro

Comment 4 2017-07-21 22:41:54 PDT

Created attachment 316174 [details] [PATCH] Proposed Fix

Devin Rousso

Comment 5 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.

Matt Baker

Comment 6 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":

Joseph Pecoraro

Comment 7 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".

Joseph Pecoraro

Comment 8 2017-07-24 11:15:54 PDT

Created attachment 316303 [details] [PATCH] Proposed Fix

Build Bot

Comment 9 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

Build Bot

Comment 10 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

Joseph Pecoraro

Comment 11 2017-07-24 13:26:34 PDT

Comment on attachment 316303 [details] [PATCH] Proposed Fix Test failure is unrelated to this change.

Matt Baker

Comment 12 2017-07-25 16:03:45 PDT

Comment on attachment 316303 [details] [PATCH] Proposed Fix r=me

WebKit Commit Bot

Comment 13 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>

WebKit Commit Bot

Comment 14 2017-07-25 16:31:24 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Joseph Pecoraro

Reported

2017-07-21 21:56 PDT

Modified

2017-07-25 16:31 PDT History

CC List

7 users Show

URL

Keywords InRadar

Depends on

Blocks