Bug 69401 – Web Inspector: add support for %c (style) in console API

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Pavel Feldman]

console.log("%cHello World", "color:red") should result in "Hello world" colored in red.
Downstream bug: http://code.google.com/p/chromium/issues/detail?id=91346

[Pavel Feldman]

*** Bug 98407 has been marked as a duplicate of this bug. ***

[Mike West]

Mind if I take a look at this, Pavel?

[Pavel Feldman]

Not at all, go ahead. I was starting it once, but then something stopped me. I don't quite remember what exactly - maybe I wanted to sandbox this style so that it does not lead to the total layout destruction. Not sure if that is necessary though.

[Mike West]

(In reply to comment #3)
> Not at all, go ahead. I was starting it once, but then something stopped me. I don't quite remember what exactly - maybe I wanted to sandbox this style so that it does not lead to the total layout destruction. Not sure if that is necessary though.

Cool, thanks.

I think restricting the allowed styles would be a good idea, but I'll worry about that once I have a first pass for you to review.

[Mike West]

Created attachment 167125 [details]
Patch

[WebKit Review Bot]

Attachment 167125 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
LayoutTests/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mike West]

Created attachment 167128 [details]
Patch

[Mike West]

Now with fewer tabs.

I have the feeling that we should do some sort of whitelisting for the style attributes, but it doesn't look like Firebug does. *shrug* https://github.com/firebug/firebug/blob/master/extension/content/firebug/console/consolePanel.js#L576 WDYT?

[Alexander Pavlov (apavlov)]

Comment on attachment 167128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167128&action=review

Technically, the change looks good, but I'm slightly concerned by a small inconsistency in the formatter behavior (which also holds for Firebug). http://getfirebug.com/wiki/index.php/Console_API does not specify the correct behavior in the case when %c and corresponding substitute arguments encounter more than once in a console.log() invocation. Currently, the style will be set to that found in the last respective argument, which is counter-intuitive (I would expect each %c to affect the entire following text up to the next %c). Does anyone have thoughts about this or am I being a paranoid?

> Source/WebCore/inspector/front-end/ConsoleMessage.js:477
> +                a.setAttribute('style', b.cssText);

We use double quotes rather than apostrophes in the Web Inspector code

> LayoutTests/http/tests/inspector/console-test.js:33
> +}

Please add a separating blank line after this one

> LayoutTests/inspector/console/console-format-style.html:7
> +// Global Values

What do you need these for? I can't see "globals" used anywhere in the code

> LayoutTests/inspector/console/console-format-style.html:26
> +<p id="p">Tests that console logging dumps properly styled messages.</p>

The "id" attribute is not used either.

[Mike West]

Thanks!

(In reply to comment #9)
> (From update of attachment 167128 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167128&action=review
> 
> Technically, the change looks good, but I'm slightly concerned by a small inconsistency in the formatter behavior (which also holds for Firebug). http://getfirebug.com/wiki/index.php/Console_API does not specify the correct behavior in the case when %c and corresponding substitute arguments encounter more than once in a console.log() invocation. Currently, the style will be set to that found in the last respective argument, which is counter-intuitive (I would expect each %c to affect the entire following text up to the next %c). Does anyone have thoughts about this or am I being a paranoid?

I think matching Firebug's behavior is probably enough. While I think it might be nice to be able to rainbow-style a console message, I'm not sure it's worth the extra complexity. I'm open to argument, though. :)

I'll address the remaining comments in a new patch.

[Mike West]

Created attachment 167290 [details]
Patch

[Pavel Feldman]

Comment on attachment 167290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167290&action=review

Wow, that is impressive! Overall looks good, but I think we should explicitly style the entire message (as FB). See my version of the code inline.

> Source/WebCore/inspector/front-end/ConsoleMessage.js:450
> +        function ConsoleMessageStyle(value)

my version had 

        function styleFormatter(obj)
        {
            formattedResult.setAttribute("style", obj.description);
        }

> Source/WebCore/inspector/front-end/ConsoleMessage.js:-463
> -            if (!(b instanceof Node))

and
        function append(a, b)
        {
            if (b instanceof Node)
                a.appendChild(b);
            else if (b)
                a.appendChild(WebInspector.linkifyStringAsFragment(b.toString()));
            return a;
        }

[Mike West]

(In reply to comment #12)
> (From update of attachment 167290 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167290&action=review
> 
> Wow, that is impressive! Overall looks good, but I think we should explicitly style the entire message (as FB). See my version of the code inline.

I'm not sure I see a difference between your code and mine. Can you clarify?

From my perspective, it seemed like a good idea to keep side-effects out of the formatters, which is why I did the hop through the extra class. That let me keep the styling in append() along with the rest of the changes. *shrug* I'm happy to do it your way if you like it better, of course. :)

[Pavel Feldman]

Comment on attachment 167290 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=167290&action=review

Your code is fine, just a bunch of nits from my perfectionist's side. See comments on why I thought it was not perfect. All you need to do is to apply the style in the %c's parameter to the top level element. Your code seemed to introduce too much complexity.

>> Source/WebCore/inspector/front-end/ConsoleMessage.js:450
>> +        function ConsoleMessageStyle(value)
> 
> my version had 
> 
>         function styleFormatter(obj)
>         {
>             formattedResult.setAttribute("style", obj.description);
>         }

You should annotate this function as @constructor.

> Source/WebCore/inspector/front-end/ConsoleMessage.js:476
> +            if (b instanceof ConsoleMessageStyle)

Annotating with classes and using instanceof looks like a hack to me, especially since you don't really need it.

> Source/WebCore/inspector/front-end/ConsoleMessage.js:477
> +                a.setAttribute("style", b.cssText);

I don't think this API gives you a good control over the element you set style to. In my case it was always the root element.

[Mike West]

(In reply to comment #14)
> (From update of attachment 167290 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167290&action=review
> 
> Your code is fine, just a bunch of nits from my perfectionist's side. See comments on why I thought it was not perfect. All you need to do is to apply the style in the %c's parameter to the top level element. Your code seemed to introduce too much complexity.

The explanation makes sense. Thanks, Pavel!

I'll spin up a new patch for you this afternoon.

[Mike West]

Created attachment 168033 [details]
Patch

[Mike West]

Alright, the latest patch should look pretty much exactly like your patch did, Pavel. WDYT? :)

[Pavel Feldman]

Comment on attachment 168033 [details]
Patch

Thank s

[Mike West]

Comment on attachment 168033 [details]
Patch

Talked with Pavel about whitelisting: short story is that we want it, but we'll do it in a separate patch. Throwing this into the queue.

[WebKit Review Bot]

Comment on attachment 168033 [details]
Patch

Clearing flags on attachment: 168033

Committed r130941: <http://trac.webkit.org/changeset/130941>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.