| Summary: | Web Inspector: styled logs should work with multiple log() arguments | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Paul Irish <paulirish> | ||||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Mike West <mkwst> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | apavlov, dglazkov, keishi, loislo, mkwst, odvarko, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
> 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.
(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 > console.log('%c red-text %c green-text', 'color:red', 'color:green');
This matches the formatting logic well, so sure, why not.
(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 I'll take this at some point in the relatively near future. Created attachment 173452 [details]
After.
(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 :( ). Created attachment 173453 [details]
Patch
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! Created attachment 173454 [details]
Patch
(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! Created attachment 173455 [details]
Rebased.
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 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 Created attachment 173460 [details]
Hrm.
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;')
(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! Created attachment 173480 [details]
Patch for landing
Comment on attachment 173480 [details] Patch for landing Clearing flags on attachment: 173480 Committed r134166: <http://trac.webkit.org/changeset/134166> All reviewed patches have been landed. Closing bug. 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)" (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 |
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.