[Webkit-unassigned] [Bug 77740] Split CSSMutableStyleDeclaration into separate internal and CSSOM types

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=77740





--- Comment #5 from Darin Adler <darin at apple.com>  2012-02-03 10:01:53 PST ---
(From update of attachment 125337)
View in context: https://bugs.webkit.org/attachment.cgi?id=125337&action=review

The EWS results seem to indicate that something isn’t working, so I’m not actually setting review+, but otherwise I would.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:237
> +    // StylePropertySet and CSSStyleDeclaration ref each other. When we have a declaration and
> +    // our refcount drops to one we know it is the only thing keeping us alive.
> +    if (m_cssStyleDeclaration && hasOneRef())
> +        m_cssStyleDeclaration.clear();

I think there is a better design pattern for this.

My suggestion is that you change CSSStyleDeclaration so it does not inherit from RefCounted, and has ref and deref functions that call the ref and deref on the StylePropertySet. Then we have only one shared reference count. CSSStyleDeclaration will use a raw pointer to point to StylePropertySet. StylePropertySet will use an OwnPtr for m_cssStyleDeclaration. What do you think of that?

> Source/WebCore/css/CSSMutableStyleDeclaration.h:37
> +class StylePropertySet : public RefCounted<StylePropertySet> {

Normally a “set” is an unordered collection. This collection has an order. So using the word set in the name is not so great, but perhaps OK.

This class should inherit from RefCountedBase, not RefCounted<StylePropertySet>. The only thing that RefCounted adds to RefCountedBase is a deref function, and we are providing our own. But you won’t need to change that if you take my suggestion about how to handle the reference counting.

>> Source/WebCore/css/CSSParser.cpp:611
>> +bool CSSParser::parseDeclaration(StylePropertySet* declaration, const String& string, RefPtr<CSSStyleSourceData>* styleSourceData, CSSStyleSheet* contextStyleSheet)
> 
> The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]

This is a bug in the style checker.

>> Source/WebCore/editing/EditingStyle.h:226
>> +int getIdentifierValue(StylePropertySet* style, CSSPropertyID);
> 
> The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]

I agree with the style bot here. We should remove the name style.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.