Bug 165743 – [CSS Parser] Implement deferred parsing of properties, @media, @supports and @keyframes

• Component: New Bugs
• Status: RESOLVED FIXED

[Dave Hyatt]

[CSS Parser] Implement deferred parsing of properties, @media, @supports and @keyframes

[Dave Hyatt]

Created attachment 296875 [details]
Patch

[WebKit Commit Bot]

Attachment 296875 [details] did not pass style-queue:


ERROR: Source/WebCore/css/parser/CSSParserImpl.h:72:  The parameter name "wrapper" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/parser/CSSParserImpl.h:106:  The parameter name "deferredParser" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/parser/CSSParserImpl.h:107:  The parameter name "deferredParser" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/parser/CSSDeferredParser.cpp:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/parser/CSSDeferredParser.cpp:50:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/testing/Internals.cpp:40:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 6 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 296875 [details]
Patch

Attachment 296875 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2700265

New failing tests:
editing/execCommand/5142012-1.html
editing/inserting/insert-at-end-02.html
editing/pasteboard/4989774.html
editing/execCommand/nsresponder-outdent.html

[Build Bot]

Created attachment 296878 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296875 [details]
Patch

Attachment 296875 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2700274

New failing tests:
editing/execCommand/5142012-1.html
editing/inserting/insert-at-end-02.html
editing/pasteboard/4989774.html
editing/execCommand/nsresponder-outdent.html

[Build Bot]

Created attachment 296879 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296875 [details]
Patch

Attachment 296875 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2700269

New failing tests:
editing/execCommand/5142012-1.html
editing/inserting/insert-at-end-02.html
editing/pasteboard/4989774.html
editing/execCommand/nsresponder-outdent.html

[Build Bot]

Created attachment 296880 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296875 [details]
Patch

Attachment 296875 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2700278

New failing tests:
editing/execCommand/5142012-1.html
fast/css/deferred-parsing/hover-test.html

[Build Bot]

Created attachment 296881 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Dave Hyatt]

Created attachment 296883 [details]
Patch

[Dave Hyatt]

Created attachment 296885 [details]
Patch

[Darin Adler]

Comment on attachment 296885 [details]
Patch

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

Looks like a good optimization. I have some style suggestions.

I wonder about some of these classes, why they are reference counted and why are they designed to be heap allocated at all. Can’t some of them just be objects that we move around and not be heap allocated at all? Or use unique_ptr instead of RefPtr.

> Source/WebCore/css/StyleProperties.cpp:1376
> +{ }

These should be on two subsequent lines.

> Source/WebCore/css/StyleProperties.h:266
> +    static Ref<DeferredStyleProperties> create(const CSSParserTokenRange&, RefPtr<CSSDeferredParser>&);

The argument here should just be CSSDeferredParser* or CSSDeferredParser& if it can never be null; I see no reason to pass in a RefPtr&.

> Source/WebCore/css/StyleProperties.h:274
> +    RefPtr<CSSDeferredParser> m_parser;

Can this be Ref instead of RefPtr?

> Source/WebCore/css/StyleRule.cpp:335
> +{ }

These should not be on the same line like this.

> Source/WebCore/css/StyleRule.h:194
> +    static Ref<DeferredStyleGroupRuleList> create(const CSSParserTokenRange&, RefPtr<CSSDeferredParser>&);

Same comment as above about RefPtr&.

> Source/WebCore/css/StyleRule.h:197
> +    void parseDeferredRules(Vector<RefPtr<StyleRuleBase>>&);
> +    void parseDeferredKeyframes(StyleRuleKeyframes&);

These functions should use return values instead of out arguments.

> Source/WebCore/css/StyleRule.h:200
> +    DeferredStyleGroupRuleList(const CSSParserTokenRange&, RefPtr<CSSDeferredParser>&);

Same comment as above about RefPtr&.

> Source/WebCore/css/StyleRule.h:203
> +    RefPtr<CSSDeferredParser> m_parser;

Ref instead of RefPtr?

> Source/WebCore/css/StyleRule.h:215
> +    StyleRuleGroup(Type, Vector<RefPtr<StyleRuleBase>>&);

Since this adopts the Vector, it really should be Vector&& instead of Vector&. The old way was the way we did this kind of thing before we had move semantics. Not new to the patch, though.

> Source/WebCore/css/StyleSheetContents.cpp:432
> +            const StyleProperties* properties = downcast<StyleRule>(*rule).propertiesWithoutDeferredParsing();

auto* would work here.

> Source/WebCore/css/StyleSheetContents.cpp:442
> +            const Vector<RefPtr<StyleRuleBase>>* childRules = downcast<StyleRuleMedia>(*rule).childRulesWithoutDeferredParsing();

I would use auto* on a line like this so it’s not so crazy long.

> Source/WebCore/css/parser/CSSDeferredParser.h:53
> +    void parseRuleList(const CSSParserTokenRange&, Vector<RefPtr<StyleRuleBase>>&);
> +    void parseKeyframeList(const CSSParserTokenRange&, StyleRuleKeyframes&);

These functions should have return values instead of out arguments.

> Source/WebCore/css/parser/CSSDeferredParser.h:60
> +    StyleSheetContents* m_styleSheet; // Safe, just a back pointer.

I don’t buy the "safe" comment. What makes it safe? What guarantees this object never outlasts the StyleSheetContents? Do we clear this out or something? This is reference counted, so clearly someone could keep it around longer than they should.

> Source/WebCore/css/parser/CSSParser.h:53
> +    enum class RuleParsing {
> +        NormalRuleParsing,
> +        DeferredRuleParsing
> +    };

Shouldn’t repeat RuleParsing in the names. Also, I would write this on one line just before it’s used:

    enum class RuleParsing { Normal, Deferred };
    void parseSheet ...

> Source/WebCore/css/parser/CSSParser.h:55
>      WEBCORE_EXPORT CSSParser(const CSSParserContext&);

Should mark this explicit.

> Source/WebCore/css/parser/CSSParserImpl.cpp:62
>      , m_observerWrapper(nullptr)

This should be initialized in the class definition instead of here.

> Source/WebCore/css/parser/CSSParserImpl.cpp:65
>  {
> +    
> +}

Left a stray blank line.

> Source/WebCore/css/parser/CSSParserImpl.cpp:71
> +    , m_observerWrapper(nullptr)

This should be initialized in the class definition instead of here.

> Source/WebCore/css/parser/CSSParserImpl.cpp:185
> +    parser.consumeRuleList(tokenRange, RegularRuleList, [&childRules](RefPtr<StyleRuleBase> rule) {

I think this should be "const RefPtr<StyleRuleBase>&" instead of RefPtr, unless it can’t be.

> Source/WebCore/css/parser/CSSParserImpl.cpp:193
> +    parser.consumeRuleList(tokenRange, KeyframesRuleList, [&keyframeRule](RefPtr<StyleRuleBase> keyframe) {

I think this should be "const RefPtr<StyleRuleBase>&" instead of RefPtr, unless it can’t be.

> Source/WebCore/css/parser/CSSParserImpl.cpp:195
> +        RefPtr<StyleKeyframe> key = static_cast<StyleKeyframe*>(keyframe.get());
> +        keyframeRule.parserAppendKeyframe(key.releaseNonNull());

This doesn’t really work. We end up doing all this releaseNonNull stuff and we still churn the reference count. I would write this:

    keyframeRule.parserAppendKeyframe(*static_cast<StyleKeyframe*>(keyframe.get()));

But then it makes me wonder two things:

1) Why is the argument to the function in the consumeRuleList thing a RefPtr<StyleRuleBase>; it should either be StyleRuleBase& or Ref<StyleRuleBase>&&.
2) Why don’t we make a safe way to do these casts, like the downcast we made for the DOM? It seems risky to have to write out static_cast.

> Source/WebCore/css/parser/CSSParserImpl.cpp:536
> +        return StyleRuleMedia::create(MediaQueryParser::parseMediaQuerySet(prelude).releaseNonNull(), DeferredStyleGroupRuleList::create(block, m_deferredParser));

What guarantees parseMediaQuerySet won’t return null? It’s usually a danger sign to be calling releaseNonNull without a null check. Means the function should have been returning a Ref all along, there is a missing null check, or there is some secret erase we know it won’t be null that the type system doesn’t know.

> Source/WebCore/css/parser/CSSParserImpl.cpp:643
> +    RefPtr<StyleRuleKeyframes> keyframeRule = StyleRuleKeyframes::create(name);

Should use auto here.

> Source/WebCore/css/parser/CSSParserImpl.cpp:647
>      consumeRuleList(block, KeyframesRuleList, [keyframeRule](RefPtr<StyleRuleBase> keyframe) {
>          RefPtr<StyleKeyframe> key = static_cast<StyleKeyframe*>(keyframe.get());
>          keyframeRule->parserAppendKeyframe(key.releaseNonNull());
>      });

Same bad pattern here that I mentioned above, all that RefPtr and releaseNonNull is not really working the way we want it to. Maybe I should have read more of this code before. Didn’t see this before.

> Source/WebCore/testing/Internals.cpp:887
> +    StyleSheetContents& contents = downcast<CSSStyleSheet>(styleSheet).contents();
> +    return deferredStyleRulesCountForList(contents.childRules());

I would use auto& or write this all on one line without the local variable.

> Source/WebCore/testing/Internals.cpp:914
> +    StyleSheetContents& contents = downcast<CSSStyleSheet>(styleSheet).contents();
> +    return deferredGroupRulesCountForList(contents.childRules());

Ditto.

> Source/WebCore/testing/Internals.h:44
> +class CSSRuleList;

Not sure why we’re adding this. Don’t see it being used.

> Source/WebCore/testing/Internals.h:125
> +    int deferredStyleRulesCount(StyleSheet&);
> +    int deferredGroupRulesCount(StyleSheet&);
> +    int deferredKeyframesRulesCount(StyleSheet&);

Would have expected unsigned here instead of int. Obviously doesn’t matter, though.

> Source/WebCore/testing/Internals.idl:114
> +    long deferredStyleRulesCount(StyleSheet sheet);
> +    long deferredGroupRulesCount(StyleSheet sheet);
> +    long deferredKeyframesRulesCount(StyleSheet sheet);

unsigned long

> LayoutTests/platform/ios-simulator/TestExpectations:2753
> +# Hover test does not apply

Isn’t there a single place we can put this to cover all variations of iOS, rather than repeating it three times?

[Dave Hyatt]

(In reply to comment #13)
> Comment on attachment 296885 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296885&action=review
> 
> Looks like a good optimization. I have some style suggestions.
> 
> I wonder about some of these classes, why they are reference counted and why
> are they designed to be heap allocated at all. Can’t some of them just be
> objects that we move around and not be heap allocated at all? Or use
> unique_ptr instead of RefPtr.

Yeah, you are right. I am going to change DeferredStyleGroupRuleList to be held in a std::unique_ptr. It doesn't have to be reference counted.

Technically DeferredStyleProperties is only refcounted because it's acting as a "stand-in" for the real properties on StyleRule. I did that to avoid adding an extra member to all StyleRules (like I ended up doing with StyleRuleGroup).

If I did just add an extra member to StyleRule, then DeferredStyleProperties could be a std::unique_ptr also, but I liked the stand-in idea as a way of saving that extra pointer per rule once the rule's properties get parsed and all the deferred stuff goes away.


> > Source/WebCore/css/StyleProperties.h:274
> > +    RefPtr<CSSDeferredParser> m_parser;
> 
> Can this be Ref instead of RefPtr?
> 

It can yeah. Changing.


> > Source/WebCore/css/StyleRule.h:203
> > +    RefPtr<CSSDeferredParser> m_parser;
> 
> Ref instead of RefPtr?
>

Yeah, same as above. Will change.
 

> > Source/WebCore/css/parser/CSSDeferredParser.h:60
> > +    StyleSheetContents* m_styleSheet; // Safe, just a back pointer.
> 
> I don’t buy the "safe" comment. What makes it safe? What guarantees this
> object never outlasts the StyleSheetContents? Do we clear this out or
> something? This is reference counted, so clearly someone could keep it
> around longer than they should.
>

I'm going to try to eliminate this back pointer completely and just make it a non-issue.


> > Source/WebCore/css/parser/CSSParserImpl.cpp:536
> > +        return StyleRuleMedia::create(MediaQueryParser::parseMediaQuerySet(prelude).releaseNonNull(), DeferredStyleGroupRuleList::create(block, m_deferredParser));
> 
> What guarantees parseMediaQuerySet won’t return null? It’s usually a danger
> sign to be calling releaseNonNull without a null check. Means the function
> should have been returning a Ref all along, there is a missing null check,
> or there is some secret erase we know it won’t be null that the type system
> doesn’t know.
> 

I screwed this up when I converted Blink's MediaQueryParser to use our memory management. I had to go through a zillion files doing this conversion to Ref/RefPtr by hand, and I seem to have made a mistake here. The query returned cannot be null, but I used RefPtrs when I should have used Refs. That means this code is ok, but I will file a follow-up bug to patch MediaQueryParser to use Refs instead of RefPtrs.

> > Source/WebCore/css/parser/CSSParserImpl.cpp:643
> > +    RefPtr<StyleRuleKeyframes> keyframeRule = StyleRuleKeyframes::create(name);
> 
> Should use auto here.
> 
> > Source/WebCore/css/parser/CSSParserImpl.cpp:647
> >      consumeRuleList(block, KeyframesRuleList, [keyframeRule](RefPtr<StyleRuleBase> keyframe) {
> >          RefPtr<StyleKeyframe> key = static_cast<StyleKeyframe*>(keyframe.get());
> >          keyframeRule->parserAppendKeyframe(key.releaseNonNull());
> >      });
> 
> Same bad pattern here that I mentioned above, all that RefPtr and
> releaseNonNull is not really working the way we want it to. Maybe I should
> have read more of this code before. Didn’t see this before.
> 

The downcast does exist for this, so it can be written like this:

parser.consumeRuleList(tokenRange, KeyframesRuleList, [&keyframeRule](const RefPtr<StyleRuleBase>& keyframe) {
        keyframeRule.parserAppendKeyframe(downcast<const StyleKeyframe>(keyframe.get()));
    });

Better?


> > Source/WebCore/testing/Internals.h:44
> > +class CSSRuleList;
> 
> Not sure why we’re adding this. Don’t see it being used.
> 

Just clutter. I was originally using CSSRuleList as the API argument and then changed to CSSStyleSheet.


> > LayoutTests/platform/ios-simulator/TestExpectations:2753
> > +# Hover test does not apply
> 
> Isn’t there a single place we can put this to cover all variations of iOS,
> rather than repeating it three times?

Maybe? I'm not sure. I'll ask tomorrow.

Thanks for the thorough review. I think there's enough here to warrant a re-review, even though you gave it r+, so I'm going to post another patch once I get all this addressed.

[Dave Hyatt]

Created attachment 296920 [details]
Patch

[Build Bot]

Comment on attachment 296920 [details]
Patch

Attachment 296920 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2705707

New failing tests:
printing/page-rule-css-text.html

[Build Bot]

Created attachment 296925 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296920 [details]
Patch

Attachment 296920 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2705809

New failing tests:
printing/page-rule-css-text.html

[Build Bot]

Created attachment 296928 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 296920 [details]
Patch

Attachment 296920 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2705821

New failing tests:
printing/page-rule-css-text.html

[Build Bot]

Created attachment 296930 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Dave Hyatt]

Created attachment 296932 [details]
Patch making the back ptr as WeakPtr

[WebKit Commit Bot]

Attachment 296932 [details] did not pass style-queue:


ERROR: Source/WebCore/css/parser/CSSDeferredParser.h:57:  The parameter name "styleSheet" adds no information, so it should be removed.  [readability/parameter_name] [5]
WARNING: Not running on native Windows.
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Landed in r209718.

[Darin Adler]

Sorry, did not get a chance to re-review. But I will look again once I get a chance.

[Gavin Barraclough]

Rolled out in r209794 due to memory regression.

[Dave Hyatt]

(In reply to comment #26)
> Rolled out in r209794 due to memory regression.

Maybe next time you could mention that in your changelog so I don't sit there wondering why the hell this was rolled out.

[Dave Hyatt]

Created attachment 297105 [details]
Patch

[Dave Hyatt]

Landed in r209826.

[Csaba Osztrogonác]

(In reply to comment #29)
> Landed in r209826.

It broke the Apple Mac build everywhere, see build.webkit.org for details.

[Radar WebKit Bug Importer]

<rdar://problem/29779237>