Bug 82832 - CSSStyleRules should own their CSSStyleDeclarations
Summary: CSSStyleRules should own their CSSStyleDeclarations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 77745
  Show dependency treegraph
 
Reported: 2012-03-31 09:02 PDT by Antti Koivisto
Modified: 2012-04-11 20:52 PDT (History)
4 users (show)

See Also:


Attachments
patch (21.62 KB, patch)
2012-03-31 09:59 PDT, Antti Koivisto
kling: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2012-03-31 09:02:30 PDT
Move the rule properties CSSOM wrapper ownership from the StylePropertySet to the rule itself. This is preparation for bug 82728 (Split remaining CSSRules into internal and CSSOM types) easier to do. This temporarily grows the size of CSSStyleRule by a pointer (82728 will give the memory back and more).
Comment 1 Antti Koivisto 2012-03-31 09:59:20 PDT
Created attachment 134961 [details]
patch
Comment 2 Andreas Kling 2012-03-31 10:18:06 PDT
Comment on attachment 134961 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134961&action=review

r=me with some sherlocking.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:93
> +    ~StyleRuleCSSStyleDeclaration();

Should be marked virtual.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.h:97
> +    virtual CSSRule* parentRule() const { return m_parentRule;  }
>      virtual void setNeedsStyleRecalc();
>      virtual CSSStyleSheet* contextStyleSheet() const;

We're missing some OVERRIDE decorations here.
Comment 3 Antti Koivisto 2012-03-31 11:03:09 PDT
http://trac.webkit.org/changeset/112798
Comment 4 Eric Seidel (no email) 2012-03-31 11:58:36 PDT
Comment on attachment 134961 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134961&action=review

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:309
> +void StyleRuleCSSStyleDeclaration::ref()
> +{ 
> +    ++m_refCount;
> +}

Isn't this just RefCounted?
Comment 5 Andreas Kling 2012-03-31 12:04:30 PDT
Comment on attachment 134961 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=134961&action=review

>> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:309
>> +}
> 
> Isn't this just RefCounted?

ref() and deref() are virtual here, for this subclass it's a simple RefCounted workalike, for e.g PropertySetCSSStyleDeclaration it forwards the refs/derefs to the underlying StylePropertySet.