Summary: | Web Inspector: Provide source data for all known rule types in CSSParser, except "keyframe" and "region" | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, cmarcelo, eric.carlson, feature-media-reviews, hyatt, joepeck, keishi, koivisto, loislo, macpherson, menard, mitz, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | 88646 | ||||||||||||||||||
Bug Blocks: | 88408 | ||||||||||||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2012-06-06 08:32:59 PDT
Created attachment 146060 [details]
Patch
Need to mention that Tools/Scripts/run-perf-tests Parser/css-parser-yui.html didn't show any impact of this change at all... Comment on attachment 146060 [details]
Patch
Please split this patch.
Created attachment 146272 [details]
Patch
Comment on attachment 146272 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146272&action=review > Source/WebCore/css/CSSParser.cpp:305 > +void CSSParser::parseSheet(StyleSheetContents* sheet, const String& string, int startLineNumber, Vector<RefPtr<CSSRuleSourceData> >* ruleSourceDataResult) You could use RuleSourceDataList here. > Source/WebCore/css/CSSParser.h:317 > + Vector<SourceRange> m_ruleBodyRangeStack; Why do you need two stacks? Created attachment 146286 [details]
Patch
Created attachment 146312 [details]
Patch
Created attachment 149281 [details]
Patch
Comment on attachment 149281 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149281&action=review > Source/WebCore/css/CSSGrammar.y:492 > + | before_import_rule IMPORT_SYM error ';' { I don't think you need to gather any data for errors. This change should be reverted. > Source/WebCore/css/CSSGrammar.y:496 > + | before_import_rule IMPORT_SYM error invalid_block { Ditto, should be reverted. > Source/WebCore/css/CSSGrammar.y:684 > + CSSParser* p = static_cast<CSSParser*>(parser); This should probably be in the end of keyframes action. > Source/WebCore/css/CSSGrammar.y:685 > + if (p->isExtractingSourceData()) I would move this check inside the method. > Source/WebCore/css/CSSGrammar.y:744 > + | before_page_rule PAGE_SYM error invalid_block { Ditto, should be reverted. > Source/WebCore/css/CSSGrammar.y:748 > + | before_page_rule PAGE_SYM error ';' { Ditto, should be reverted. > Source/WebCore/css/CSSGrammar.y:856 > + | before_font_face_rule FONT_FACE_SYM error invalid_block { Ditto, should be reverted. > Source/WebCore/css/CSSGrammar.y:860 > + | before_font_face_rule FONT_FACE_SYM error ';' { Ditto, should be reverted. > Source/WebCore/css/CSSParser.cpp:1156 > + RefPtr<CSSRuleSourceData> ruleSourceDataRef = ruleSourceData; prpRuleSourceData / ruleSourceData? > Source/WebCore/css/CSSParser.cpp:1175 > + if (ruleSourceDataRef) { I would add an assertion that stack size is correct here. > Source/WebCore/css/CSSParser.cpp:9375 > + ASSERT(!m_currentRuleDataStack->isEmpty()); We can probably extract these 4-5 lines to one method. > Source/WebCore/css/CSSParser.cpp:9681 > +void CSSParser::markAtRuleHeaderStart(CSSRuleSourceData::Type ruleType) Looks like markStyleRuleHeaderStart and markAtRuleHeaderStart are identical. Created attachment 149301 [details]
Patch
Comment on attachment 149301 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149301&action=review > Source/WebCore/css/CSSParser.cpp:9339 > +void CSSParser::updateTopRuleBodyEnd() Could this be replaced by markRuleBodyEnd? > Source/WebCore/css/CSSParser.cpp:9344 > + if (m_currentRuleDataStack->isEmpty()) The stack is never empty at this point. > Source/WebCore/css/CSSParser.cpp:9350 > + if (!data) You are already asserting data is not null on the line above. Created attachment 150133 [details]
Patch
Comment on attachment 150133 [details]
Patch
This looks good to me now inspector-wise.
Comment on attachment 150133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150133&action=review r=me > Source/WebCore/ChangeLog:9 > + No new tests, as there is no client-visible behavior change. Existing Inspector tests will be modified > + to test the new data provided, along with the necessary Inspector plumbing. You could add a word about performance (which I hear progresses) Comment on attachment 150133 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150133&action=review >> Source/WebCore/ChangeLog:9 >> + to test the new data provided, along with the necessary Inspector plumbing. > > You could add a word about performance (which I hear progresses) And some motivation too. Committed r121551: <http://trac.webkit.org/changeset/121551> |