WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Shane Stephens
Comment 1
2012-08-09 16:16:43 PDT
Created
attachment 157572
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug