Summary: | Web Inspector: Improve Console Message Formatting | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | burg, graouts, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Joseph Pecoraro
2015-04-10 16:58:31 PDT
Created attachment 250542 [details]
[TEST] Test for different console log cases
Created attachment 250543 [details]
[PATCH] Proposed Fix
Comment on attachment 250543 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=250543&action=review > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css:106 > + margin-left: 24px; This should have been 20px. Comment on attachment 250543 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=250543&action=review > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css:204 > + color: hsl(0, 0%, 67%); Might want to use the same color as .console-message-enclosed. > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:149 > + this._boundExpandClickHandler = function() { this.toggle(); }.bind(this); Though this is safer, it also makes an extra closure that isn't needed. Maybe JSC ignores that since there is not closure vars used? > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:338 > + this._stackTraceElement = this._element.appendChild(document.createElement("ul")); > + this._stackTraceElement.classList.add("console-message-stack-trace-container"); > + this._stackTraceElement.classList.add("console-message-text"); > + > + for (var callFrame of this._message.stackTrace) { > + var callFrameElement = this._stackTraceElement.appendChild(document.createElement("li")); > + callFrameElement.classList.add("console-message-stack-trace-call-frame"); > + callFrameElement.textContent = callFrame.functionName || WebInspector.UIString("(anonymous function)"); > + if (callFrame.url && !this._shouldHideURL(callFrame.url)) > + callFrameElement.appendChild(this._linkifyCallFrame(callFrame)); > } We might want to clean this up with a custom UI with function icons and locations like the stack frame sidebar section. To unify how we show stack traces. (In reply to comment #5) > Comment on attachment 250543 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=250543&action=review > > > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css:204 > > + color: hsl(0, 0%, 67%); > > Might want to use the same color as .console-message-enclosed. Great catch! I actually just fixed all of these to be: color: rgba(0, 0, 0, 0.33); > > > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:149 > > + this._boundExpandClickHandler = function() { this.toggle(); }.bind(this); > > Though this is safer, it also makes an extra closure that isn't needed. > Maybe JSC ignores that since there is not closure vars used? Another great catch! I used to have more code in the handler but later moved it to expand. We should totally be able to make this "this.toggle". > > > Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:338 > > + this._stackTraceElement = this._element.appendChild(document.createElement("ul")); > > + this._stackTraceElement.classList.add("console-message-stack-trace-container"); > > + this._stackTraceElement.classList.add("console-message-text"); > > + > > + for (var callFrame of this._message.stackTrace) { > > + var callFrameElement = this._stackTraceElement.appendChild(document.createElement("li")); > > + callFrameElement.classList.add("console-message-stack-trace-call-frame"); > > + callFrameElement.textContent = callFrame.functionName || WebInspector.UIString("(anonymous function)"); > > + if (callFrame.url && !this._shouldHideURL(callFrame.url)) > > + callFrameElement.appendChild(this._linkifyCallFrame(callFrame)); > > } > > We might want to clean this up with a custom UI with function icons and > locations like the stack frame sidebar section. To unify how we show stack > traces. Ooo, that is a great idea! And I made an error in my post-review changes that got fixed with: http://trac.webkit.org/changeset/182646 |