Bug 88646 - Web Inspector: CSSParser::parseSheet() should provide ready-to-use source data
Summary: Web Inspector: CSSParser::parseSheet() should provide ready-to-use source data
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:
Blocks: 88420
  Show dependency treegraph
 
Reported: 2012-06-08 04:40 PDT by Alexander Pavlov (apavlov)
Modified: 2012-06-15 09:36 PDT (History)
18 users (show)

See Also:


Attachments
Patch (23.45 KB, patch)
2012-06-08 05:03 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (26.76 KB, patch)
2012-06-08 07:53 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (26.65 KB, patch)
2012-06-09 01:17 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-08 04:40:38 PDT
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).
Comment 1 Alexander Pavlov (apavlov) 2012-06-08 05:03:51 PDT
Created attachment 146540 [details]
Patch
Comment 2 Vsevolod Vlasov 2012-06-08 07:33:01 PDT
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.
Comment 3 Alexander Pavlov (apavlov) 2012-06-08 07:53:37 PDT
Created attachment 146570 [details]
Patch
Comment 4 Vsevolod Vlasov 2012-06-08 09:34:50 PDT
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?
Comment 5 Vsevolod Vlasov 2012-06-08 09:40:56 PDT
Other than that inspector-related part looks good to me, somebody who understands CSSParser better might want to have a look.
Comment 6 Alexander Pavlov (apavlov) 2012-06-09 01:17:02 PDT
Created attachment 146693 [details]
Patch
Comment 7 Antti Koivisto 2012-06-15 09:31:01 PDT
Comment on attachment 146693 [details]
Patch

Looks ok, bit sad about the extra bloat in CSSParser. r=me
Comment 8 Alexander Pavlov (apavlov) 2012-06-15 09:36:32 PDT
Committed r120469: <http://trac.webkit.org/changeset/120469>