[CSS Parser] Implement deferred parsing of properties, @media, @supports and @keyframes
Created attachment 296875 [details] Patch
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.
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
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
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
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
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
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
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
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
Created attachment 296883 [details] Patch
Created attachment 296885 [details] Patch
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?
(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.
Created attachment 296920 [details] Patch
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
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
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
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
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
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
Created attachment 296932 [details] Patch making the back ptr as WeakPtr
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.
Landed in r209718.
Sorry, did not get a chance to re-review. But I will look again once I get a chance.
Rolled out in r209794 due to memory regression.
(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.
Created attachment 297105 [details] Patch
Landed in r209826.
(In reply to comment #29) > Landed in r209826. It broke the Apple Mac build everywhere, see build.webkit.org for details.
<rdar://problem/29779237>