WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
[PATCH] Proposed Fix
(22.80 KB, patch)
2015-03-11 12:41 PDT
,
Joseph Pecoraro
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2013-04-09 15:58:10 PDT
<
rdar://problem/13614327
>
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
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
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
https://trac.webkit.org/r181426
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.
Top of Page
Format For Printing
XML
Clone This Bug