Bug 93664

Created attachment 157572 [details] Patch

Comment on attachment 157572 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157572&action=review > Source/WebCore/ChangeLog:11 > + .a { & .b { ... } } results in two rules: .a { } and .a .b { ... }, and the .a component > + is present in both rules. For this reason, a copy constructor is required for CSS Selectors. Would it be possible for the .a component to be shared by both rules instead of making a copy? > Source/WebCore/css/CSSSelector.h:34 > + class RefcountedCSSSelectorList : public RefCounted<RefcountedCSSSelectorList> { Why make a wrapper class instead of making CSSselectorList refcounted?

> > Source/WebCore/ChangeLog:11 > > + .a { & .b { ... } } results in two rules: .a { } and .a .b { ... }, and the .a component > > + is present in both rules. For this reason, a copy constructor is required for CSS Selectors. > > Would it be possible for the .a component to be shared by both rules instead of making a copy? After parsing, CSSSelectors are memcpy'd into arrays in CSSSelectorLists before being used for rendering. This memcpy (see CSSSelector.h:move for the implementation, and CSSSelectorList.cpp:adoptSelectorVector for its use) frees the old location without destructing the class after moving it. Having a shared CSSSelector doesn't play nice with this approach as we'd need to reference-count the parsed version in order to decide whether to free after memcpy or not. > > Source/WebCore/css/CSSSelector.h:34 > > + class RefcountedCSSSelectorList : public RefCounted<RefcountedCSSSelectorList> { > > Why make a wrapper class instead of making CSSselectorList refcounted? I tried making CSSSelectorList refcounted initially but something went wrong. Unfortunately I can't remember what. I'll have another go and see what happens, so that at least we can record the rationale in the bug.

I think the issue is that CSSSelectorLists are explicitly copied around (they have a copy constructor, which doesn't play nice with RefCounted, right?). Maybe I should be looking at a bigger refactor here, but I'm uncomfortable with poking at so much of the innards of the CSS Engine - I suspect a lot of these hacks (copying CSSSelectors, copying CSSSelectorLists, etc.) are here for performance reasons.

(In reply to comment #4) > I think the issue is that CSSSelectorLists are explicitly copied around (they have a copy constructor, which doesn't play nice with RefCounted, right?). Maybe I should be looking at a bigger refactor here, but I'm uncomfortable with poking at so much of the innards of the CSS Engine - I suspect a lot of these hacks (copying CSSSelectors, copying CSSSelectorLists, etc.) are here for performance reasons. I think it would be fine to do a refactor that switches from using a copy constructor to a copy() method or clone() method and use WTF_MAKE_NONCOPYABLE to prevent accidental copying. We do this in other classes like StyleRule or CSSValueList.

Comment on attachment 157572 [details] Patch r- while we investigate making CSSSelectorList refcounted. That might be a patch by itself (separate from the CSSSelector copy constructor changes). The perf test in PerformanceTests/Parser/css-parser-yui.html should exercise this pretty well.

CSSSelector already has a copy constructor that works quite well. We should just use that.