[webkit-reviews] review denied: [Bug 110483] Web Inspector: CSS Debugging Information : [Attachment 190686] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Sergey Ryazanov
<serya at chromium.org>'s request for review:
Bug 110483: Web Inspector: CSS Debugging Information
https://bugs.webkit.org/show_bug.cgi?id=110483

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

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


Overall looks good.

You should split this change into:
1) assorted refactorings
2) console plumbing
3) incremental grammar improvements

> Source/WebCore/css/CSSGrammar.y.in:745
> +    | invalid_block {

Why did this change?

> Source/WebCore/css/CSSGrammar.y.in:1556
> +	   $$ = true;

why?

> Source/WebCore/css/CSSGrammar.y.in:1708
> +location_mark: {

error_location

> Source/WebCore/css/CSSGrammar.y.in:-1668
> -    | expr invalid_block_list {

Where did this go?

> Source/WebCore/css/CSSGrammar.y.in:2041
> +  | ATKEYWORD invalid_block {

Why was this added?

> Source/WebCore/css/CSSGrammar.y.in:2070
> +    '{' location_mark invalid_block_contents closing_brace {

This seems to be non-equivalent substitution.

> Source/WebCore/css/CSSGrammar.y.includes:57
> +#define YYPRINT(File,Type,Value) YYFPRINTF(File, "%s", yylvalToString(Type,
&(Value)).utf8().data())

These things should be landed separately.

> Source/WebCore/css/CSSParser.cpp:456
> +void CSSParser::parseSheet(StyleSheetContents* sheet, const String& string,
int startLineNumber, RuleSourceDataList* ruleSourceDataResult, bool logErrors)

Lets always log error.

> Source/WebCore/css/CSSParser.cpp:11131
> +    builder.appendLiteral("CSS unexpected token: ");

Unexpected CSS token

> Source/WebCore/css/CSSParser.cpp:11132
> +    for (unsigned i = 0; i < location.token.length(); i++) {

I think we could handle it on the front-end for now.

> Source/WebCore/css/CSSParser.cpp:11150
> +void CSSParser::suspendErrors()

suspendErrorReportingInProperty

> Source/WebCore/css/CSSParser.cpp:11152
> +    m_errorsSuspended = true;

m_propertyErrorReportingSuspended

> Source/WebCore/css/CSSParser.cpp:11162
> +    m_styleSheet->singleOwnerDocument()->logCSSWarningToConsole(message,
m_styleSheet->baseURL().string(), lineNumber + 1);

You should lookup real Console object (via page)