Bug 93664 – Add copy constructor to CSSSelector

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Shane Stephens]

Add copy constructor to CSSSelector

[Shane Stephens]

Created attachment 157572 [details]
Patch

[Tony Chang]

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?

[Shane Stephens]

> > 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.

[Shane Stephens]

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.

[Tony Chang]

(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.

[Tony Chang]

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.

[Shane Stephens]

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