RESOLVED FIXED 114313
Web Inspector: CSS parser errors in the console should include column numbers
https://bugs.webkit.org/show_bug.cgi?id=114313
Summary Web Inspector: CSS parser errors in the console should include column numbers
Joseph Pecoraro
Reported 2013-04-09 15:57:35 PDT
In order to jump to the correct place in source code that is potentially pretty printed, the error should include a line number and column number. Currently CSSParser::logError only includes the line number and doesn't include a column number.
Attachments
[PATCH] Proposed Fix (22.39 KB, patch)
2015-03-10 21:01 PDT, Joseph Pecoraro
no flags
[PATCH] Proposed Fix (22.80 KB, patch)
2015-03-11 12:41 PDT, Joseph Pecoraro
darin: review+
Radar WebKit Bug Importer
Comment 1 2013-04-09 15:58:10 PDT
Joseph Pecoraro
Comment 2 2015-03-10 14:06:01 PDT
I'm looking at this right now because inline CSS warnings are always useless on minified CSS. Ben pointed me in the right direction and this looks very easy!
Joseph Pecoraro
Comment 3 2015-03-10 21:01:06 PDT
Created attachment 248393 [details] [PATCH] Proposed Fix
Benjamin Poulain
Comment 4 2015-03-10 22:23:20 PDT
Comment on attachment 248393 [details] [PATCH] Proposed Fix That seems correct to me, rs=me
WebKit Commit Bot
Comment 5 2015-03-10 23:11:40 PDT
Comment on attachment 248393 [details] [PATCH] Proposed Fix Clearing flags on attachment: 248393 Committed r181367: <http://trac.webkit.org/changeset/181367>
WebKit Commit Bot
Comment 6 2015-03-10 23:11:44 PDT
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 7 2015-03-11 08:48:37 PDT
Chris Dumez
Comment 8 2015-03-11 08:49:00 PDT
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x000000010d51539a WTFCrash + 42 1 com.apple.WebCore 0x000000010efa34ea WebCore::CSSParser::currentLocation() + 170 (CSSParser.cpp:10495) 2 com.apple.WebCore 0x000000010ef439d6 cssyyparse(WebCore::CSSParser*) + 66182 (.CSSGrammar.y:1856) 3 com.apple.WebCore 0x000000010ef70f5a WebCore::CSSParser::parseSheet(WebCore::StyleSheetContents*, WTF::String const&, int, int, WTF::Vector<WTF::RefPtr<WebCore::CSSRuleSourceData>, 0ul, WTF::CrashOnOverflow>*, bool) + 1178 (CSSParser.cpp:451) 4 com.apple.WebCore 0x000000011090c0eb WebCore::StyleSheetContents::parseStringAtLineAndColumn(WTF::String const&, int, int, bool) + 123 (StyleSheetContents.cpp:325) 5 com.apple.WebCore 0x000000010f7fd477 WebCore::InlineStyleSheetOwner::createSheet(WebCore::Element&, WTF::String const&) + 1255 (InlineStyleSheetOwner.cpp:149) 6 com.apple.WebCore 0x000000010f7fcd04 WebCore::InlineStyleSheetOwner::createSheetFromTextContents(WebCore::Element&) + 68 (InlineStyleSheetOwner.cpp:97) 7 com.apple.WebCore 0x000000010f7fcf7b WebCore::InlineStyleSheetOwner::finishParsingChildren(WebCore::Element&) + 59 (InlineStyleSheetOwner.cpp:92) 8 com.apple.WebCore 0x000000010f68e80b WebCore::HTMLStyleElement::finishParsingChildren() + 43 (HTMLStyleElement.cpp:91)
WebKit Commit Bot
Comment 9 2015-03-11 08:54:28 PDT
Re-opened since this is blocked by bug 142581
Joseph Pecoraro
Comment 10 2015-03-11 12:41:06 PDT
Created attachment 248441 [details] [PATCH] Proposed Fix This addresses the ASSERT seen in css3/filters/filter-animation-from-none-multi-hw.html. We need to have the token's start line/column, at the time currentLocation() is called we may have parsed past a newline and incremented m_columnOffsetForLine. So stash the token's column at the time we stash the token's line.
Darin Adler
Comment 11 2015-03-11 16:04:28 PDT
Comment on attachment 248441 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=248441&action=review > Source/WebCore/css/CSSParser.cpp:337 > + , m_tokenStartColumnNumber(0) > , m_lastSelectorLineNumber(0) > + , m_columnOffsetForLine(0) > + , m_sheetStartLineNumber(0) > + , m_sheetStartColumnNumber(0) It’s better to initialize in the header; avoids a need to list everything twice. Especially appropriate when the initial value is simple and logical (just a 0) and not specific to any particular constructor. > Source/WebCore/css/CSSParser.cpp:11605 > - if (*currentCharacter<SrcCharacterType>() == '\n') > + if (*currentCharacter<SrcCharacterType>() == '\n') { > ++m_lineNumber; > - ++currentCharacter<SrcCharacterType>(); > + ++currentCharacter<SrcCharacterType>(); > + m_columnOffsetForLine = currentCharacterOffset(); > + } else > + ++currentCharacter<SrcCharacterType>(); Could we use currentCharacterOffset() + 1 here and avoid having to repeat the ++currentCharacter? > Source/WebCore/css/CSSParser.cpp:11904 > + break; Why add this break? > Source/WebCore/css/CSSParser.h:108 > + void parseSheet(StyleSheetContents*, const String&, int startLineNumber, int startColumnNumber, RuleSourceDataList*, bool logErrors); Should we use WTF::TextPosition here instead of separate integers? > Source/WebCore/css/CSSParser.h:467 > + inline unsigned currentCharacterOffset(); The inline keyword here has no effect. I suggest omitting it. We should remove it from those other lines too, later. > Source/WebCore/css/CSSParser.h:652 > + void logError(const String& message, int lineNumber, int columnNumber); Should we use WTF::TextPosition here instead of separate integers? > Source/WebCore/css/StyleSheetContents.h:65 > + bool parseStringAtLineAndColumn(const String&, int startLineNumber, int startColumnNumber, bool createdByParser); Should we use WTF::TextPosition here instead of separate integers?
Joseph Pecoraro
Comment 12 2015-03-11 17:34:14 PDT
(In reply to comment #11) > Comment on attachment 248441 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248441&action=review > > > Source/WebCore/css/CSSParser.cpp:337 > > + , m_tokenStartColumnNumber(0) > > , m_lastSelectorLineNumber(0) > > + , m_columnOffsetForLine(0) > > + , m_sheetStartLineNumber(0) > > + , m_sheetStartColumnNumber(0) > > It’s better to initialize in the header; avoids a need to list everything > twice. Especially appropriate when the initial value is simple and logical > (just a 0) and not specific to any particular constructor. Yes, I'm just caught in the normal position of: - modernize the entire class at the same time => ugly diff, clean file - modernize just my piece and leave the rest unmodified => clean diff, ugly file - match the existing style of the file with new changes => clean diff, clean file > > Source/WebCore/css/CSSParser.cpp:11605 > > - if (*currentCharacter<SrcCharacterType>() == '\n') > > + if (*currentCharacter<SrcCharacterType>() == '\n') { > > ++m_lineNumber; > > - ++currentCharacter<SrcCharacterType>(); > > + ++currentCharacter<SrcCharacterType>(); > > + m_columnOffsetForLine = currentCharacterOffset(); > > + } else > > + ++currentCharacter<SrcCharacterType>(); > > Could we use currentCharacterOffset() + 1 here and avoid having to repeat > the ++currentCharacter? I originally had that and ran into issues. However I changed a lot since then so I'll retry and go with that approach if it works. > > Source/WebCore/css/CSSParser.cpp:11904 > > + break; > > Why add this break? Though not a specific WebKit style, the majority of our switches include a break in the default case. It is not required. I agree with the other comments, and I'll address them.
Joseph Pecoraro
Comment 13 2015-03-11 21:49:59 PDT
Joseph Pecoraro
Comment 14 2015-03-11 23:39:54 PDT
Follow-up fix for ASSERT seen in LayoutTests/printing/page-format-data.html. https://trac.webkit.org/r181434
Note You need to log in before you can comment on or make changes to this bug.