Bug 110483

Summary:

Web Inspector: CSS Debugging Information

Product:

WebKit

Reporter:

Sergey Ryazanov <serya>

Component:

Web Inspector (Deprecated)

Assignee:

Sergey Ryazanov <serya>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, buildbot, burg, dglazkov, esprehn+autocc, gtk-ews, joepeck, keishi, loislo, macpherson, menard, mkwst+watchlist, ojan.autocc, pfeldman, philn, pmuellr, rego+ews, rniwa, syoichi, timothy, vsevik, web-inspector-bugs, webkit.review.bot, xan.lopez, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

http://crbug.com/140608

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Screenshot	none
Patch	none
Patch	none

Sergey Ryazanov

Reported 2013-02-21 08:59:19 PST

Show CSS warnings in inspector console.

Attachments
Patch (29.25 KB, patch) 2013-02-21 09:06 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (106.66 KB, patch) 2013-02-26 02:21 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (112.61 KB, patch) 2013-02-26 05:35 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (53.96 KB, patch) 2013-02-26 08:41 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (53.91 KB, patch) 2013-02-26 23:20 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (46.14 KB, patch) 2013-02-27 06:21 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (36.45 KB, patch) 2013-02-27 08:11 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (40.75 KB, patch) 2013-02-27 08:46 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (43.30 KB, patch) 2013-02-28 02:17 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (43.29 KB, patch) 2013-02-28 02:32 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Screenshot (125.57 KB, image/png) 2013-02-28 04:40 PST, Sergey Ryazanov	no flags	Details
Patch (40.39 KB, patch) 2013-03-01 05:58 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Patch (40.14 KB, patch) 2013-03-01 08:07 PST, Sergey Ryazanov	no flags	Details Formatted Diff Diff
Show Obsolete (11) View All Add attachment proposed patch, testcase, etc.

Sergey Ryazanov

Comment 1 2013-02-21 09:06:05 PST

Created attachment 189542 [details] Patch

Build Bot

Comment 2 2013-02-21 21:49:36 PST

Comment on attachment 189542 [details] Patch Attachment 189542 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16694527

Mike West

Comment 3 2013-02-21 22:50:36 PST

Comment on attachment 189542 [details] Patch I like the direction, thanks for this! I'm concerned, however, that this is going to flood the console with warnings. I believe we should block this work on a solution to https://bugs.webkit.org/show_bug.cgi?id=98492 that will enable developers to somehow filter down the messages to only those that they care about. Also: no tests? If I could give an r- for that, I would. :)

WebKit Review Bot

Comment 4 2013-02-21 23:10:02 PST

Comment on attachment 189542 [details] Patch Attachment 189542 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16693606 New failing tests: css2.1/t040105-atkeyw-02-b.html animations/big-rotation.html css2.1/t040105-atrule-00-b.html css3/calc/transitions.html css2.1/t040105-atrule-01-b.html animations/suspend-resume-animation-events.html css2.1/t0402-syntax-03-f.html css2.1/t0402-syntax-01-f.html css2.1/t0402-syntax-02-f.html accessibility/table-detection.html css3/parsing-css3-nthchild.html css2.1/t0402-c71-fwd-parsing-00-f.html css2.1/t040103-escapes-02-d.html css3/calc/zoom-with-em.html css1/conformance/forward_compatible_parsing.html css3/filters/filter-animation-multi-hw.html css3/filters/filter-animation-multi.html css2.1/t0402-c71-fwd-parsing-03-f.html css3/filters/filter-animation-from-none-multi.html css3/filters/filter-animation-from-none.html css3/filters/filter-animation-from-none-multi-hw.html css2.1/t0402-syntax-04-f.html css3/filters/filter-animation.html css2.1/t040103-escapes-08-b.html css3/filters/filter-animation-from-none-hw.html css2.1/t0402-syntax-06-f.html css3/font-feature-settings-parsing.html css3/filters/filter-animation-hw.html css2.1/t0402-syntax-05-f.html css2.1/t0402-c71-fwd-parsing-02-f.html

Sergey Ryazanov

Comment 5 2013-02-22 00:14:26 PST

This patch is uploaded not to commit but for discussion. Tests to be added later. (In reply to comment #3) > (From update of attachment 189542 [details]) > I like the direction, thanks for this! > > I'm concerned, however, that this is going to flood the console with warnings. I believe we should block this work on a solution to https://bugs.webkit.org/show_bug.cgi?id=98492 that will enable developers to somehow filter down the messages to only those that they care about. > > Also: no tests? If I could give an r- for that, I would. :)

Build Bot

Comment 6 2013-02-22 18:39:31 PST

Comment on attachment 189542 [details] Patch Attachment 189542 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16711380

Build Bot

Comment 7 2013-02-22 20:31:20 PST

Comment on attachment 189542 [details] Patch Attachment 189542 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16719401

Build Bot

Comment 8 2013-02-23 00:44:34 PST

Comment on attachment 189542 [details] Patch Attachment 189542 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16718581

Sergey Ryazanov

Comment 9 2013-02-26 02:21:12 PST

Created attachment 190249 [details] Patch

Sergey Ryazanov

Comment 10 2013-02-26 05:35:24 PST

Created attachment 190267 [details] Patch

Alexander Pavlov (apavlov)

Comment 11 2013-02-26 05:51:23 PST

Comment on attachment 190267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190267&action=review > Source/WebCore/css/CSSGrammar.y.in:1660 > + // ??????????? It's a good idea to find the person who added this comment and find out what it means. > Source/WebCore/css/CSSParser.cpp:11128 > + String message = String("CSS unexpected token: ") + String(location.token); Please use StringBuilder and ASCIILiteral when building the message. > LayoutTests/fast/table/frame-and-rules.html:-1 > -<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" I understand that you have been fixing test document errors as an exercise :) Why is this line removed? > LayoutTests/fast/text/international/bidi-AN-after-empty-run.html:2 > + div { width: 80px; border: 1px solid blue; padding: 4px; margin: 4p; } JFYI: this should become 4px, not 4p > LayoutTests/inspector/console/console-css-warnings-expected.txt:2 > +FAIL: Timed out waiting for notifyDone to be called This is not the right expectation :) > LayoutTests/inspector/console/console-css-warnings.html:124 > +<body onload="onload()"> This seemingly results in an infinite loop? You should runTest() directly (take a look at other tests).

Sergey Ryazanov

Comment 12 2013-02-26 08:41:39 PST

Created attachment 190295 [details] Patch

Sergey Ryazanov

Comment 13 2013-02-26 08:55:56 PST

Comment on attachment 190267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190267&action=review >> Source/WebCore/css/CSSParser.cpp:11128 >> + String message = String("CSS unexpected token: ") + String(location.token); > > Please use StringBuilder and ASCIILiteral when building the message. Done >> LayoutTests/fast/table/frame-and-rules.html:-1 >> -<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" > > I understand that you have been fixing test document errors as an exercise :) Why is this line removed? It actually haven't. This line actually is huge and contains all the CSS code. Anyway, I reverted this change since it isn't important for this change and hard to track. >> LayoutTests/fast/text/international/bidi-AN-after-empty-run.html:2 >> + div { width: 80px; border: 1px solid blue; padding: 4px; margin: 4p; } > > JFYI: this should become 4px, not 4p Fixed. >> LayoutTests/inspector/console/console-css-warnings-expected.txt:2 >> +FAIL: Timed out waiting for notifyDone to be called > > This is not the right expectation :) Right. run_webkit_tests stored platform-specific baseline and I didn't spot that. >> LayoutTests/inspector/console/console-css-warnings.html:124 >> +<body onload="onload()"> > > This seemingly results in an infinite loop? You should runTest() directly (take a look at other tests). I actually borrowed that code from another test and it works. But your proposal is shorter. Thanks.

Build Bot

Comment 14 2013-02-26 10:22:14 PST

Comment on attachment 190295 [details] Patch Attachment 190295 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16768429

Build Bot

Comment 15 2013-02-26 11:23:19 PST

Comment on attachment 190295 [details] Patch Attachment 190295 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16771336

WebKit Review Bot

Comment 16 2013-02-26 16:35:26 PST

Comment on attachment 190295 [details] Patch Attachment 190295 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16799006 New failing tests: fast/text/international/bidi-AN-after-empty-run.html

WebKit Review Bot

Comment 17 2013-02-26 17:40:17 PST

Comment on attachment 190295 [details] Patch Attachment 190295 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16792002 New failing tests: fast/text/international/bidi-AN-after-empty-run.html

Sergey Ryazanov

Comment 18 2013-02-26 23:20:19 PST

Created attachment 190450 [details] Patch Fixed bidi-AN-after-empty-run

Build Bot

Comment 19 2013-02-27 00:00:05 PST

Comment on attachment 190450 [details] Patch Attachment 190450 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16841030

Build Bot

Comment 20 2013-02-27 00:35:35 PST

Comment on attachment 190450 [details] Patch Attachment 190450 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16819002

kov's GTK+ EWS bot

Comment 21 2013-02-27 00:48:06 PST

Comment on attachment 190450 [details] Patch Attachment 190450 [details] did not pass gtk-ews (gtk): Output: http://webkit-commit-queue.appspot.com/results/16758103

Build Bot

Comment 22 2013-02-27 01:38:47 PST

Comment on attachment 190450 [details] Patch Attachment 190450 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16831065

Sergey Ryazanov

Comment 23 2013-02-27 06:21:46 PST

Created attachment 190507 [details] Patch

Build Bot

Comment 24 2013-02-27 07:00:44 PST

Comment on attachment 190507 [details] Patch Attachment 190507 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16842147

Alexander Pavlov (apavlov)

Comment 25 2013-02-27 07:03:08 PST

Comment on attachment 190507 [details] Patch 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 > +@media screen { What about unknown media types and syntax errors in the media queries? > LayoutTests/inspector/console/console-css-warnings.html:44 > + 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 > +@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 > + 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.

Sergey Ryazanov

Comment 26 2013-02-27 08:11:14 PST

Created attachment 190524 [details] Patch

Sergey Ryazanov

Comment 27 2013-02-27 08:46:29 PST

Created attachment 190531 [details] Patch

Build Bot

Comment 28 2013-02-27 09:27:44 PST

Comment on attachment 190531 [details] Patch Attachment 190531 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16833028

Build Bot

Comment 29 2013-02-27 10:29:53 PST

Comment on attachment 190531 [details] Patch Attachment 190531 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16794104

Build Bot

Comment 30 2013-02-27 11:38:26 PST

Comment on attachment 190531 [details] Patch Attachment 190531 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/16766079

Sergey Ryazanov

Comment 31 2013-02-28 02:17:21 PST

Created attachment 190683 [details] Patch

Early Warning System Bot

Comment 32 2013-02-28 02:25:58 PST

Comment on attachment 190683 [details] Patch Attachment 190683 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16848150

Early Warning System Bot

Comment 33 2013-02-28 02:28:16 PST

Comment on attachment 190683 [details] Patch Attachment 190683 [details] did not pass qt-wk2-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/16837180

EFL EWS Bot

Comment 34 2013-02-28 02:29:32 PST

Comment on attachment 190683 [details] Patch Attachment 190683 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/16768764

Sergey Ryazanov

Comment 35 2013-02-28 02:32:02 PST

Created attachment 190686 [details] Patch

Sergey Ryazanov

Comment 36 2013-02-28 02:34:05 PST

Comment on attachment 190507 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190507&action=review >> Source/WebCore/css/CSSParser.cpp:11131 >> + builder.append(String(ASCIILiteral("CSS unexpected token: "))); > > Please check if you can use builder.appendLiteral("CSS...") directly. done >> Source/WebCore/page/Console.cpp:171 >> + if (source == CSSMessageSource) > > What does this do? The warnings still seem to be logged, according to the test? Warning are logged in the Inspector previous with lines (InspectorInstrumentation::addMessageToConsole). But don't go to page->chrome()->client(). So DumpRenderTree doesn't see them. It let ~200 tests with CSS errors (many of them intentionally test CSS errors) to pass. >> LayoutTests/inspector/console/console-css-warnings.html:36 >> +@media screen { > > What about unknown media types and syntax errors in the media queries? unknown media types is not that type of errors I'm trying to catch at the moment. Regarding syntax errors in the media queries. Our grammar handles some types of error but not in the way http://www.w3.org/TR/css3-mediaqueries/ states. For instance this is example from the spec: @media (example, all,), speech { x:* /* ignored in our grammar while supposed to work on speech devices*/ } So we need to fix it first. >> LayoutTests/inspector/console/console-css-warnings.html:44 >> + > > 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.) The grammar has 2 rules to cover all that cases: PAGE_SYM errors invalid_block PAGE_SYM errors ';' The examples you provided anyway would be reduced to one of this rule. I don't see that we would test with that. >> LayoutTests/inspector/console/console-css-warnings.html:46 >> +@page :pseudo-class { > > This should be an error/warning, since only :left, :right, and :first pseudo-pages are supported. done >> LayoutTests/inspector/console/console-css-warnings.html:64 >> + color: * > > What about the invalid property name inside @font-face case? This patch doesn't handle invalid property names inside @font-face or in any other declaration_list. It is to be done later. >> LayoutTests/inspector/console/console-css-warnings.html:117 >> +runTest(); > > According to the convention, this should be executed from the <body onload> handler. done

Sergey Ryazanov

Comment 37 2013-02-28 04:40:48 PST

Created attachment 190703 [details] Screenshot

Pavel Feldman

Comment 38 2013-02-28 06:44:12 PST

Comment on attachment 190686 [details] Patch 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)

Sergey Ryazanov

Comment 39 2013-03-01 05:58:52 PST

Created attachment 190951 [details] Patch

Sergey Ryazanov

Comment 40 2013-03-01 06:13:57 PST

Comment on attachment 190686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190686&action=review >> Source/WebCore/css/CSSGrammar.y.in:745 >> + | invalid_block { > > Why did this change? It's redundant. I expected previous rule doesn't cover just invalid block but id does. >> Source/WebCore/css/CSSGrammar.y.in:1556 >> + $$ = true; > > why? Otherwise "div {}" is an invalid declaration. It used to be OK but while we didn't report it. >> Source/WebCore/css/CSSGrammar.y.in:1708 >> +location_mark: { > > error_location done >> Source/WebCore/css/CSSGrammar.y.in:2041 >> + | ATKEYWORD invalid_block { > > Why was this added? As above. Removed as redundant. >> Source/WebCore/css/CSSGrammar.y.in:2070 >> + '{' location_mark invalid_block_contents closing_brace { > > This seems to be non-equivalent substitution. I think it is equivalent. Anyway previous code worked fine so I returned it. >> 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. done >> Source/WebCore/css/CSSParser.cpp:456 >> +void CSSParser::parseSheet(StyleSheetContents* sheet, const String& string, int startLineNumber, RuleSourceDataList* ruleSourceDataResult, bool logErrors) > > Lets always log error. I don't think we want to report errors of reparsing in InspectorStyleSheet (it just duplicates errors) and script-generated CSS (because we can't properly associate it with source code). >> Source/WebCore/css/CSSParser.cpp:11131 >> + builder.appendLiteral("CSS unexpected token: "); > > Unexpected CSS token done >> 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. We doesn't do it now. So JS errors are escaped on the backend. Lets do so with CSS errors for consistency. >> Source/WebCore/css/CSSParser.cpp:11150 >> +void CSSParser::suspendErrors() > > suspendErrorReportingInProperty done >> Source/WebCore/css/CSSParser.cpp:11152 >> + m_errorsSuspended = true; > > m_propertyErrorReportingSuspended done >> 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) done

Sergey Ryazanov

Comment 41 2013-03-01 08:07:51 PST

Created attachment 190970 [details] Patch

Pavel Feldman

Comment 42 2013-03-06 07:21:51 PST

Comment on attachment 190970 [details] Patch Clearing r? as this patch is being split into parts.

Note You need to log in before you can comment on or make changes to this bug.