Bug 69401

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

Mind if I take a look at this, Pavel?

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.

(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.

Created attachment 167125 [details] Patch

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.

Created attachment 167128 [details] Patch

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?

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.

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.

Created attachment 167290 [details] Patch

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; }

(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. :)

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.

(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.

Created attachment 168033 [details] Patch

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

Comment on attachment 168033 [details] Patch Thank s

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.

Comment on attachment 168033 [details] Patch Clearing flags on attachment: 168033 Committed r130941: <http://trac.webkit.org/changeset/130941>

All reviewed patches have been landed. Closing bug.