Bug 143623 - Web Inspector: Improve Console Message Formatting
Summary: Web Inspector: Improve Console Message Formatting
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-10 16:58 PDT by Joseph Pecoraro
Modified: 2015-04-10 17:52 PDT (History)
8 users (show)

See Also:


Attachments
[TEST] Test for different console log cases (1.34 KB, text/html)
2015-04-10 16:59 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (44.94 KB, patch)
2015-04-10 17:09 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2015-04-10 16:58:31 PDT
* SUMMARY
Improve Console Message Formatting. Mostly focusing on handling of extra objects.

One of the goals of the new console design was to eliminate expanding multiple objects on the same line. In that way, a console.log/error/etc with multiple objects should be presented with each object on its own line. This eliminates the confusing and poor experience that comes with expanding/collapsing objects sharing the same line which causes everything to move when expanding.

The different cases for logging are and proposed UI are:

    1. console.log("message")
      => message

    2. console.log("format string message: %s", "foo")
      => format string message: foo

    3. console.log(valueOrLosslessObject)
      => {a: 1}

    4. console.log(lossyObject)
      => ▶︎ Object {a: 1, b: 2, c: 3, d: 4, e: 5, ...}

    6. console.log("message", valueOrLosslessObject)
      => message – {a: 1}

    7. console.log("message", lossyObject)
      => ▶︎ message – Object {a: 1, b: 2, c: 3, ...}

    b. (it is collapsed by default, so after expanding)
      => ▼ message
         • ▼ Object
            a: 1
            b: 2
            ...

    8. console.log("message", anyValue1, anyValue2)
      => ▼ message (2)
         • {a: 1}
         • ▶︎ Object {a: 1, b: 2, c: 3, d: 4, e: 5, ...}

    b. (it is expanded by default, so after collapsing)
      => ▶︎ message (2)

    9. console.log(anyValue1, anyValue2)
      => ▼ No message (2)
         • {a: 1}
         • ▶︎ Object {a: 1, b: 2, c: 3, d: 4, e: 5, ...}


* NOTES
- There is an initial version of what this would look like at: http://timothy.hatcher.name/console/
- Currently for console.error we show the backtrace / call stack after the objects. Those can still show up underneath these "extra objects" for now.
Comment 1 Radar WebKit Bug Importer 2015-04-10 16:58:47 PDT
<rdar://problem/20507112>
Comment 2 Joseph Pecoraro 2015-04-10 16:59:42 PDT
Created attachment 250542 [details]
[TEST] Test for different console log cases
Comment 3 Joseph Pecoraro 2015-04-10 17:09:59 PDT
Created attachment 250543 [details]
[PATCH] Proposed Fix
Comment 4 Joseph Pecoraro 2015-04-10 17:11:37 PDT
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 5 Timothy Hatcher 2015-04-10 17:19:04 PDT
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.
Comment 6 Joseph Pecoraro 2015-04-10 17:34:51 PDT
(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!
Comment 7 Joseph Pecoraro 2015-04-10 17:48:11 PDT
http://trac.webkit.org/changeset/182644
Comment 8 Joseph Pecoraro 2015-04-10 17:52:19 PDT
And I made an error in my post-review changes that got fixed with:
http://trac.webkit.org/changeset/182646