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.
<rdar://problem/13614327>
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!
Created attachment 248393 [details] [PATCH] Proposed Fix
Comment on attachment 248393 [details] [PATCH] Proposed Fix That seems correct to me, rs=me
Comment on attachment 248393 [details] [PATCH] Proposed Fix Clearing flags on attachment: 248393 Committed r181367: <http://trac.webkit.org/changeset/181367>
All reviewed patches have been landed. Closing bug.
This seems to have cause 7 layout tests to crash: https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r181367%20(2681)/results.html
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)
Re-opened since this is blocked by bug 142581
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.
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?
(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.
https://trac.webkit.org/r181426
Follow-up fix for ASSERT seen in LayoutTests/printing/page-format-data.html. https://trac.webkit.org/r181434