[webkit-reviews] review denied: [Bug 98945] Web Inspector: Whitelist safe styles for 'console.log('%c...', ...)'. : [Attachment 168070] Patch

Thu Oct 11 04:33:35 PDT 2012

Pavel Feldman <pfeldman at chromium.org> has denied Mike West
<mkwst at chromium.org>'s request for review:
Bug 98945: Web Inspector: Whitelist safe styles for 'console.log('%c...',
...)'.
https://bugs.webkit.org/show_bug.cgi?id=98945

Attachment 168070: Patch
https://bugs.webkit.org/attachment.cgi?id=168070&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168070&action=review

> Source/WebCore/inspector/front-end/ConsoleMessage.js:455
> +		   if (buffer.style.hasOwnProperty(x) && isWhitelistedStyle(x))

You will get hundreds of them, it is better to iterate the whitelist instead.

> Source/WebCore/inspector/front-end/ConsoleMessage.js:460
> +	   function isWhitelistedStyle(s) {

{ on the next line

> Source/WebCore/inspector/front-end/ConsoleMessage.js:464
> +	     var prefixes = ["background", "border", "font", "margin",
"padding", "text", "webkitBackground", "webkitBorder", "webkitFont",
"webkitMargin", "webkitPadding", "webkitText"];

You don't need to check the prefixes - getting the shorthand "border" should
generate proper value for you.

> LayoutTests/inspector/console/console-format-style-whitelist.html:2
> +    <head>

We never indent in the inspector tests.