[webkit-reviews] review canceled: [Bug 110483] Web Inspector: CSS Debugging Information : [Attachment 190507] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
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.
More information about the webkit-reviews
mailing list