Split CSSMutableStyleDeclaration into separate internal and CSSOM types

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

CSSStyleDeclaration is part of the external CSSOM interface. WebCore shouldn't use for storing properties internally.

[Antti Koivisto]

Created attachment 125337 [details]
patch

[WebKit Review Bot]

Attachment 125337 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSParser.h:77:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/editing/EditingStyle.h:226:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSParser.cpp:611:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 125337 [details]
patch

Attachment 125337 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11421303

[WebKit Review Bot]

Comment on attachment 125337 [details]
patch

Attachment 125337 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11422314

New failing tests:
animations/missing-from-to-transforms.html
animations/fill-mode-missing-from-to-keyframes.html
animations/animation-add-events-in-handler.html
animations/missing-from-to.html
animations/animation-on-inline-crash.html
animations/empty-keyframes.html

[Darin Adler]

Comment on attachment 125337 [details]
patch

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.

[Antti Koivisto]

(In reply to comment #5)
> The EWS results seem to indicate that something isn’t working, so I’m not actually setting review+, but otherwise I would.

Will update.

> 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?

Sounds sensible but I'll leave that to a followup. The computed style refcounting needs to be considered too.

> 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 is primarily a set (used almost mainly through set-like interface) so I think the name fits. (and we have ordered ListHashSet too)

[Antti Koivisto]

Created attachment 125360 [details]
updated patch

- fix a keyframe crasher
- try to fix the qt build

[WebKit Review Bot]

Attachment 125360 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSParser.h:77:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/css/CSSParser.cpp:611:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 125360 [details]
updated patch

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

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

As I said in my first review, this should derive from RefCountedBase, not RefCounted<>.

[Antti Koivisto]

http://trac.webkit.org/changeset/106681

[Antti Koivisto]

(In reply to comment #9)
> As I said in my first review, this should derive from RefCountedBase, not RefCounted<>.

Yeah, the landed patch has that fix too.