Summary: | Web Inspector: Inline multiple console log values if they are simple | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||||
Component: | Web Inspector | Assignee: | 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
Joseph Pecoraro
2017-07-21 21:56:25 PDT
Created attachment 316169 [details]
[IMAGE] Before
Created attachment 316170 [details]
[IMAGE] After
Created attachment 316174 [details]
[PATCH] Proposed Fix
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 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": (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". Created attachment 316303 [details]
[PATCH] Proposed Fix
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 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 on attachment 316303 [details]
[PATCH] Proposed Fix
Test failure is unrelated to this change.
Comment on attachment 316303 [details]
[PATCH] Proposed Fix
r=me
Comment on attachment 316303 [details] [PATCH] Proposed Fix Clearing flags on attachment: 316303 Committed r219893: <http://trac.webkit.org/changeset/219893> All reviewed patches have been landed. Closing bug. |