RESOLVED INVALID 93664
Add copy constructor to CSSSelector
https://bugs.webkit.org/show_bug.cgi?id=93664
Summary Add copy constructor to CSSSelector
Shane Stephens
Reported 2012-08-09 16:08:24 PDT
Add copy constructor to CSSSelector
Attachments
Patch (7.48 KB, patch)
2012-08-09 16:16 PDT, Shane Stephens
tony: review-
Shane Stephens
Comment 1 2012-08-09 16:16:43 PDT
Tony Chang
Comment 2 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?
Shane Stephens
Comment 3 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.
Shane Stephens
Comment 4 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.
Tony Chang
Comment 5 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.
Tony Chang
Comment 6 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.
Shane Stephens
Comment 7 2012-08-13 22:43:06 PDT
CSSSelector already has a copy constructor that works quite well. We should just use that.
Note You need to log in before you can comment on or make changes to this bug.