Bug 93664 - Add copy constructor to CSSSelector
Summary: Add copy constructor to CSSSelector
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shane Stephens
URL:
Keywords:
Depends on:
Blocks: 79939
  Show dependency treegraph
 
Reported: 2012-08-09 16:08 PDT by Shane Stephens
Modified: 2012-08-13 22:43 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.48 KB, patch)
2012-08-09 16:16 PDT, Shane Stephens
tony: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shane Stephens 2012-08-09 16:08:24 PDT
Add copy constructor to CSSSelector
Comment 1 Shane Stephens 2012-08-09 16:16:43 PDT
Created attachment 157572 [details]
Patch
Comment 2 Tony Chang 2012-08-09 17:01:16 PDT
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?
Comment 3 Shane Stephens 2012-08-09 17:34:13 PDT
> > 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.
Comment 4 Shane Stephens 2012-08-09 17:53:20 PDT
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.
Comment 5 Tony Chang 2012-08-09 17:59:51 PDT
(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 6 Tony Chang 2012-08-12 20:51:56 PDT
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.
Comment 7 Shane Stephens 2012-08-13 22:43:06 PDT
CSSSelector already has a copy constructor that works quite well. We should just use that.