Bug 113175 – Web Inspector: Remove console warnings for *_ prefixed CSS styles

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

[Sergey Ryazanov]

This hack commonly used for IE-only declarations. It floods the console and should be ignored.

[Sergey Ryazanov]

Created attachment 194801 [details]
Patch

[Sergey Ryazanov]

Created attachment 194816 [details]
Patch

[Pavel Feldman]

Comment on attachment 194816 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:11288
> +    // Ignore syntax error. This trick is used for IE7:

There should be no "IE7" in CSS parser. You should filter this error later.

[Sergey Ryazanov]

Created attachment 194834 [details]
Patch

[Pavel Feldman]

Comment on attachment 194834 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:11273
> +static void AppendToken(StringBuilder* builder, const CSSParserString& token)

Methods start with lower case in WebKit.

> Source/WebCore/css/CSSParser.cpp:11294
> +    builder.appendLiteral("Invalid CSS property declaration at: ");

I'd pass enum into single syntaxError and would choose title based on the enum value.

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:191
> +    if (source == CSSMessageSource && message == "Invalid CSS property declaration at: *")

I'd simply look for "*", please also add a comment explaining why this happens.

[Sergey Ryazanov]

Created attachment 194841 [details]
Patch

[Pavel Feldman]

Comment on attachment 194841 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:11289
> +        appendToken(&builder, location.token);

You don't need appendToken any longer, do you?

> Source/WebCore/css/CSSParser.h:80
> +        InvalidPropertyDeclaration,

PropertyDeclarationError

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:192
> +    if (source == CSSMessageSource && message == "Invalid CSS property declaration at: *")

You should only look for "*" as I suggested.

[Sergey Ryazanov]

Created attachment 194851 [details]
Patch

[Pavel Feldman]

Comment on attachment 194851 [details]
Patch

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

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:193
> +        int index = message.find(": ");

size_t

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:194
> +        if (index >= 0 && (int)message.length() > index + 1 && message[index + 2] == '*')

static_cast<int>(...)

[Sergey Ryazanov]

Comment on attachment 194851 [details]
Patch

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

>> Source/WebCore/inspector/InspectorConsoleAgent.cpp:193
>> +        int index = message.find(": ");
> 
> size_t

WTF::STring::find has int return value and may return -1.

[Sergey Ryazanov]

Created attachment 194858 [details]
Patch

[Pavel Feldman]

Comment on attachment 194858 [details]
Patch

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

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:194
> +        if (index >= 0 && static_cast<int>(message.length()) > index + 1 && message[index + 2] == '*')

Even when message.length() > index + 1, it does not mean message[index + 2] is within range. Isn't it all just find(": *")?

[Sergey Ryazanov]

Created attachment 194866 [details]
Patch

[WebKit Review Bot]

Comment on attachment 194866 [details]
Patch

Clearing flags on attachment: 194866

Committed r146781: <http://trac.webkit.org/changeset/146781>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[PhistucK]

How about adding an option to enable all logging?
We should encourage removing hacks, too.

[Pavel Feldman]

(In reply to comment #16)
> How about adding an option to enable all logging?
> We should encourage removing hacks, too.

Sure. This is a short term fix. The real one is to make it logged with verbose severity + filter it out on backend by default. Stay tuned.

[PhistucK]

(In reply to comment #17)
> (In reply to comment #16)
> > How about adding an option to enable all logging?
> > We should encourage removing hacks, too.
> 
> Sure. This is a short term fix. The real one is to make it logged with verbose severity + filter it out on backend by default. Stay tuned.

I say "Yay!". :)