Bug 88420 - Web Inspector: Provide source data for all known rule types in CSSParser, except "keyframe" and "region"
Summary: Web Inspector: Provide source data for all known rule types in CSSParser, exc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on: 88646
Blocks: 88408
  Show dependency treegraph
 
Reported: 2012-06-06 08:32 PDT by Alexander Pavlov (apavlov)
Modified: 2012-06-29 05:06 PDT (History)
19 users (show)

See Also:


Attachments
Patch (88.77 KB, patch)
2012-06-06 10:40 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (50.10 KB, patch)
2012-06-07 06:19 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (49.21 KB, patch)
2012-06-07 07:18 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (49.89 KB, patch)
2012-06-07 09:49 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (41.32 KB, patch)
2012-06-25 06:32 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (40.18 KB, patch)
2012-06-25 09:05 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (39.44 KB, patch)
2012-06-29 04:08 PDT, Alexander Pavlov (apavlov)
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2012-06-06 08:32:59 PDT
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.
Comment 1 Alexander Pavlov (apavlov) 2012-06-06 10:40:38 PDT
Created attachment 146060 [details]
Patch
Comment 2 Alexander Pavlov (apavlov) 2012-06-06 10:45:19 PDT
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 3 Vsevolod Vlasov 2012-06-07 04:53:00 PDT
Comment on attachment 146060 [details]
Patch

Please split this patch.
Comment 4 Alexander Pavlov (apavlov) 2012-06-07 06:19:20 PDT
Created attachment 146272 [details]
Patch
Comment 5 Vsevolod Vlasov 2012-06-07 07:10:54 PDT
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?
Comment 6 Alexander Pavlov (apavlov) 2012-06-07 07:18:57 PDT
Created attachment 146286 [details]
Patch
Comment 7 Alexander Pavlov (apavlov) 2012-06-07 09:49:17 PDT
Created attachment 146312 [details]
Patch
Comment 8 Alexander Pavlov (apavlov) 2012-06-25 06:32:32 PDT
Created attachment 149281 [details]
Patch
Comment 9 Vsevolod Vlasov 2012-06-25 08:28:48 PDT
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.
Comment 10 Alexander Pavlov (apavlov) 2012-06-25 09:05:55 PDT
Created attachment 149301 [details]
Patch
Comment 11 Vsevolod Vlasov 2012-06-25 09:52:04 PDT
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.
Comment 12 Alexander Pavlov (apavlov) 2012-06-29 04:08:56 PDT
Created attachment 150133 [details]
Patch
Comment 13 Vsevolod Vlasov 2012-06-29 04:25:49 PDT
Comment on attachment 150133 [details]
Patch

This looks good to me now inspector-wise.
Comment 14 Antti Koivisto 2012-06-29 04:43:40 PDT
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 15 Antti Koivisto 2012-06-29 04:45:26 PDT
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.
Comment 16 Alexander Pavlov (apavlov) 2012-06-29 05:06:30 PDT
Committed r121551: <http://trac.webkit.org/changeset/121551>