Bug 88420 – Web Inspector: Provide source data for all known rule types in CSSParser, except "keyframe" and "region"

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Alexander Pavlov (apavlov)]

This patch does not intend to refactor the protocol, so some new code will be in an intermediate state to mask the actual rule tree structure (along with non-style rules) when pushing it over the WIP.

[Alexander Pavlov (apavlov)]

Created attachment 146060 [details]
Patch

[Alexander Pavlov (apavlov)]

Need to mention that Tools/Scripts/run-perf-tests Parser/css-parser-yui.html didn't show any impact of this change at all...

[Vsevolod Vlasov]

Comment on attachment 146060 [details]
Patch

Please split this patch.

[Alexander Pavlov (apavlov)]

Created attachment 146272 [details]
Patch

[Vsevolod Vlasov]

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?

[Alexander Pavlov (apavlov)]

Created attachment 146286 [details]
Patch

[Alexander Pavlov (apavlov)]

Created attachment 146312 [details]
Patch

[Alexander Pavlov (apavlov)]

Created attachment 149281 [details]
Patch

[Vsevolod Vlasov]

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.

[Alexander Pavlov (apavlov)]

Created attachment 149301 [details]
Patch

[Vsevolod Vlasov]

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.

[Alexander Pavlov (apavlov)]

Created attachment 150133 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 150133 [details]
Patch

This looks good to me now inspector-wise.

[Antti Koivisto]

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)

[Antti Koivisto]

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.

[Alexander Pavlov (apavlov)]

Committed r121551: <http://trac.webkit.org/changeset/121551>