Bug 88646 – Web Inspector: CSSParser::parseSheet() should provide ready-to-use source data

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

[Alexander Pavlov (apavlov)]

Currently, parseSheet() returns an intermediate form (StyleRuleRangeMap), which needs to be post-processed by the client.
The entire processing chain should be encapsulated into CSSParser instead.

This change starts a series of changes, which are intended to improve the CSS source code model exposed through Web Inspector (meta bug 88408).

[Alexander Pavlov (apavlov)]

Created attachment 146540 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 146540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146540&action=review

> Source/WebCore/css/CSSParser.cpp:1131
> +        data->styleSourceData = *styleSourceData;

styleSourceData should be PassRefPtr<CSSStyleSourceData>.

> Source/WebCore/css/CSSParser.cpp:1150
> +        RefPtr<CSSRuleSourceData>& data = m_currentRuleDataStack->last();

Why do you need & here?
Consider asserting the stack is not empty.

> Source/WebCore/css/CSSParser.cpp:1153
> +        for (Vector<CSSPropertySourceData>::iterator it = data->styleSourceData->propertyData.begin(), endIt = data->styleSourceData->propertyData.end(); it != endIt; ++it) {

Style guide prefers using indexes over iterators for Vectors.

> Source/WebCore/css/CSSParser.cpp:9161
> +            ASSERT(!m_currentRuleDataStack->isEmpty());

You have the same assert in popRuleData() which is called on next line, seems redundant.

> Source/WebCore/css/CSSParser.cpp:9381
> +    if (!ruleData->styleSourceData)

This check seems redundant since this method should be called for style rules only.

[Alexander Pavlov (apavlov)]

Created attachment 146570 [details]
Patch

[Vsevolod Vlasov]

Comment on attachment 146570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146570&action=review

> Source/WebCore/css/CSSParser.h:314
> +    size_t m_parsedTextLength;

This field is not used outside of setupParser(), why is it needed?

[Vsevolod Vlasov]

Other than that inspector-related part looks good to me, somebody who understands CSSParser better might want to have a look.

[Alexander Pavlov (apavlov)]

Created attachment 146693 [details]
Patch

[Antti Koivisto]

Comment on attachment 146693 [details]
Patch

Looks ok, bit sad about the extra bloat in CSSParser. r=me

[Alexander Pavlov (apavlov)]

Committed r120469: <http://trac.webkit.org/changeset/120469>