Bug 101495 - Web Inspector: styled logs should work with multiple log() arguments
Summary: Web Inspector: styled logs should work with multiple log() arguments
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-07 11:33 PST by Paul Irish
Modified: 2012-11-12 00:30 PST (History)
12 users (show)

See Also:


Attachments
After. (14.39 KB, image/png)
2012-11-10 07:26 PST, Mike West
no flags Details
Patch (8.74 KB, patch)
2012-11-10 08:14 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (8.65 KB, patch)
2012-11-10 08:40 PST, Mike West
no flags Details | Formatted Diff | Diff
Rebased. (8.64 KB, patch)
2012-11-10 08:46 PST, Mike West
no flags Details | Formatted Diff | Diff
Hrm. (8.64 KB, patch)
2012-11-10 11:19 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (8.70 KB, patch)
2012-11-10 15:40 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Irish 2012-11-07 11:33:56 PST
Execute:

console.log('%c styled', 'unstyled', 'color:red');

Only the text " styled color:red"  is logged.



I would expect these to work:

console.log('%c red text', 'unstyled', 'color:red');
console.log(window, 'unstyled', '%c red text', 'color:red');
console.log('unstyled', document.body, '%c red text', 'color:red');

I suppose there isn't a spec's behavior here to follow, but this seems an intuitive direction.
Comment 1 Pavel Feldman 2012-11-07 12:21:01 PST
> I would expect these to work:
> 
> console.log('%c red text', 'unstyled', 'color:red');
> console.log(window, 'unstyled', '%c red text', 'color:red');
> console.log('unstyled', document.body, '%c red text', 'color:red');

Here is the message formatting logic: when there are multiple arguments, if the first argument is a string, it is considered a "message format". message format string absorbes parameter per %X occurrence. So you should not expect the onces above to work.
Comment 2 Jan Honza Odvarko 2012-11-08 07:52:58 PST
(In reply to comment #1)
> > I would expect these to work:
> > 
> > console.log('%c red text', 'unstyled', 'color:red');
> > console.log(window, 'unstyled', '%c red text', 'color:red');
> > console.log('unstyled', document.body, '%c red text', 'color:red');
> 

> Here is the message formatting logic: when there are multiple arguments, if
> the first argument is a string, it is considered a "message format". message
> format string absorbes parameter per %X occurrence.
Agree

FWG had a similar discussion about styling parts of the log output, e.g. make only part of the log red or green.

What about using the following concept:

console.log('%c red-text %c green-text', 'color:red', 'color:green');

Honza
Comment 3 Pavel Feldman 2012-11-09 04:21:15 PST
> console.log('%c red-text %c green-text', 'color:red', 'color:green');

This matches the formatting logic well, so sure, why not.
Comment 4 Jan Honza Odvarko 2012-11-09 05:17:41 PST
(In reply to comment #3)
> > console.log('%c red-text %c green-text', 'color:red', 'color:green');
> 
> This matches the formatting logic well, so sure, why not.
Excellent, patch for Firebug committed at:
https://github.com/firebug/firebug/commit/7d6f7e951f9222d8c8f5252c148d84e072f4bb18

Related issue report:
http://code.google.com/p/fbug/issues/detail?id=6064

Honza
Comment 5 Mike West 2012-11-09 05:22:26 PST
I'll take this at some point in the relatively near future.
Comment 6 Mike West 2012-11-10 07:26:35 PST
Created attachment 173452 [details]
After.
Comment 7 Mike West 2012-11-10 07:27:42 PST
(In reply to comment #6)
> Created an attachment (id=173452) [details]
> After.

The work is trivial; I just need to change the tests. Should have a patch up in an hour or so (compiling === slow at home :( ).
Comment 8 Mike West 2012-11-10 08:14:22 PST
Created attachment 173453 [details]
Patch
Comment 9 Pavel Feldman 2012-11-10 08:26:17 PST
Comment on attachment 173453 [details]
Patch

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

> Source/WebCore/inspector/front-end/ConsoleMessage.js:477
> +        var currentStyle = {};

You probably mean var currentStyle = null; here.

> Source/WebCore/inspector/front-end/ConsoleMessage.js:523
> +                        if (currentStyle.hasOwnProperty(key))

You don't need this check: enumerable properties on the object created with {} is what you need.

> LayoutTests/inspector/console/console-format-style.html:9
> +                console.log('%cRed! %cRed!', 'color: blue;', 'color: red;');

%cBlue! %cRed!
Comment 10 Mike West 2012-11-10 08:40:22 PST
Created attachment 173454 [details]
Patch
Comment 11 Mike West 2012-11-10 08:41:35 PST
(In reply to comment #9)
> (From update of attachment 173453 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173453&action=review
> 
> > Source/WebCore/inspector/front-end/ConsoleMessage.js:477
> > +        var currentStyle = {};
> 
> You probably mean var currentStyle = null; here.

Sure. That's slightly faster.

> > Source/WebCore/inspector/front-end/ConsoleMessage.js:523
> > +                        if (currentStyle.hasOwnProperty(key))
> 
> You don't need this check: enumerable properties on the object created with {} is what you need.

Habit. But you're right; I'll drop it.

> > LayoutTests/inspector/console/console-format-style.html:9
> > +                console.log('%cRed! %cRed!', 'color: blue;', 'color: red;');
> 
> %cBlue! %cRed!

%cCopy! %cPaste! %cError! :)

Thanks!
Comment 12 Mike West 2012-11-10 08:46:05 PST
Created attachment 173455 [details]
Rebased.
Comment 13 Pavel Feldman 2012-11-10 10:06:03 PST
Comment on attachment 173455 [details]
Rebased.

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

Thanks!

> Source/WebCore/inspector/front-end/ConsoleMessage.js:480
> +        var currentStyle = null;

It is not that it is now faster. It is that otherwise your code was wrong and was wrapping everything.
Comment 14 WebKit Review Bot 2012-11-10 10:41:10 PST
Comment on attachment 173455 [details]
Rebased.

Attachment 173455 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14796347

New failing tests:
inspector/console/console-format-style-whitelist.html
Comment 15 Mike West 2012-11-10 11:19:28 PST
Created attachment 173460 [details]
Hrm.
Comment 16 Mike West 2012-11-10 11:20:52 PST
Comment on attachment 173460 [details]
Hrm.

Carrying over r+. Not sure why the bots didn't like the patch. Reuploading it after changing a line of the expectations that I can't explain ('text-decoration: none;' -> 'text-decoration: initial;')
Comment 17 Mike West 2012-11-10 11:21:23 PST
(In reply to comment #13)
> (From update of attachment 173455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173455&action=review
> 
> Thanks!
> 
> > Source/WebCore/inspector/front-end/ConsoleMessage.js:480
> > +        var currentStyle = null;
> 
> It is not that it is now faster. It is that otherwise your code was wrong and was wrapping everything.

You're right. I'd convinced myself that an empty object was falsey. :(

Thanks!
Comment 18 Mike West 2012-11-10 15:40:06 PST
Created attachment 173480 [details]
Patch for landing
Comment 19 WebKit Review Bot 2012-11-10 16:21:01 PST
Comment on attachment 173480 [details]
Patch for landing

Clearing flags on attachment: 173480

Committed r134166: <http://trac.webkit.org/changeset/134166>
Comment 20 WebKit Review Bot 2012-11-10 16:21:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 eustas.bug 2012-11-12 00:04:55 PST
Comment on attachment 173480 [details]
Patch for landing

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

> Source/WebCore/inspector/front-end/ConsoleMessage.js:525
> +                    for (key in currentStyle)

Should be "for (var key in currentStyle)"
Comment 22 Mike West 2012-11-12 00:30:33 PST
(In reply to comment #21)
> (From update of attachment 173480 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173480&action=review
> 
> > Source/WebCore/inspector/front-end/ConsoleMessage.js:525
> > +                    for (key in currentStyle)
> 
> Should be "for (var key in currentStyle)"

Oops. You are entirely correct: https://bugs.webkit.org/show_bug.cgi?id=101908