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

Wed Feb 27 07:00:43 PST 2013

Alexander Pavlov (apavlov) <apavlov at chromium.org> has canceled 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 190507: Patch
https://bugs.webkit.org/attachment.cgi?id=190507&action=review

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190507&action=review

> Source/WebCore/css/CSSGrammar.y.in:1674
> +    IMPORTANT_SYM { $<location>$ = parser->currentLocation(); } maybe_space
{

Can you migrate this to "location_mark" as well?

> Source/WebCore/css/CSSGrammar.y.in:2070
> +    '{' { $<location>$ = parser->currentLocation(); } invalid_block_contents
closing_brace {

Ditto for "location_mark".

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

Please check if you can use builder.appendLiteral("CSS...") directly.

> Source/WebCore/css/StyleSheetContents.h:145
> +    StyleSheetContents(StyleRuleImport* ownerRule, const String&
originalURL, const CSSParserContext&, const String& sourceURL);

I don't think you need a new argument. Take a look at the |String
InspectorStyleSheet::styleSheetURL(CSSStyleSheet* pageStyleSheet)|
implementation. The URL you need is embedded into the CSSParserContext.

> Source/WebCore/dom/ProcessingInstruction.cpp:220
> +    RefPtr<StyleSheetContents> newSheet = StyleSheetContents::create(href,
parserContext, href);

This change will go away.

> Source/WebCore/dom/StyleElement.cpp:175
> +	       String sourceURL = m_createdByParser ? document->url().string()
: String();

Ditto.

> Source/WebCore/html/HTMLLinkElement.cpp:324
> +    RefPtr<StyleSheetContents> styleSheet = StyleSheetContents::create(href,
parserContext, href);

Ditto.

> Source/WebCore/page/Console.cpp:171
> +    if (source == CSSMessageSource)

What does this do? The warnings still seem to be logged, according to the test?

> LayoutTests/inspector/console/console-css-warnings-expected.txt:3
> +CSS unexpected token: * console-css-warnings.html:8

Opera seems to provide more versatile and useful messages. Can we do anything
similar to that?

> LayoutTests/inspector/console/console-css-warnings-expected.txt:28
> +CSS unexpected token: 

This should be converted into an escaped representation ("\n" or something)

> LayoutTests/inspector/console/console-css-warnings.html:36
> + at media screen {

What about unknown media types and syntax errors in the media queries?

> LayoutTests/inspector/console/console-css-warnings.html:44
> +<!-- @page -->

What about garbage in the @page selectors? Please refer to
http://www.w3.org/TR/css3-page ("*" is one example, but there are others, like
invalid IDENT etc.)

> LayoutTests/inspector/console/console-css-warnings.html:46
> + at page :pseudo-class {

This should be an error/warning, since only :left, :right, and :first
pseudo-pages are supported.

> LayoutTests/inspector/console/console-css-warnings.html:64
> +    color: *

What about the invalid property name inside @font-face case?

> LayoutTests/inspector/console/console-css-warnings.html:99
> +<!-- Warnings suppressed as common IE7 compatibility tricks -->

Ultimately, we should be able to fine-tune the suppressions from the Web
Inspector.

> LayoutTests/inspector/console/console-css-warnings.html:117
> +runTest();

According to the convention, this should be executed from the <body onload>
handler.