Bug 174746

Summary: Web Inspector: Inline multiple console log values if they are simple
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, hi, inspector-bugzilla-changes, joepeck, mattbaker, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[IMAGE] Before
none
[IMAGE] After
none
[PATCH] Proposed Fix
mattbaker: review-
[PATCH] Proposed Fix
none
Archive of layout-test-results from ews126 for ios-simulator-wk2 none

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-
DetailsFormatted DiffDiff
[PATCH] Proposed Fix (11.64 KB, patch)
2017-07-24 11:15 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
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.