Bug 78257 – Use underlying property set to refcount PropertySetCSSStyleDeclaration

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

StylePropertySetCSSStyleDeclaration refcounting can be done in a cleaner way.

[Antti Koivisto]

Created attachment 126383 [details]
patch

[Early Warning System Bot]

Comment on attachment 126383 [details]
patch

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

[Antti Koivisto]

Created attachment 126402 [details]
try to fix qt build, other cleanups

[WebKit Review Bot]

Attachment 126402 [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/CSSStyleDeclaration.h:39:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 20 files


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

[Early Warning System Bot]

Comment on attachment 126402 [details]
try to fix qt build, other cleanups

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

[Andreas Kling]

Comment on attachment 126402 [details]
try to fix qt build, other cleanups

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

TD;DR

> Source/WebCore/css/StylePropertySet.cpp:50
> +    PropertySetCSSStyleDeclaration(StylePropertySet* propertySet) : m_propertySet(propertySet) { }

explicit

> Source/WebCore/css/StylePropertySet.h:170
> -    mutable RefPtr<PropertySetCSSStyleDeclaration> m_cssStyleDeclaration;
> +    mutable OwnPtr<PropertySetCSSStyleDeclaration> m_cssStyleDeclaration;

We should move this to a hashmap and have a bit for "has CSSOM object"

[Antti Koivisto]

(In reply to comment #6)
> TD;DR

TD?
 
> explicit

How explicit?

> We should move this to a hashmap and have a bit for "has CSSOM object"

Sure Sherlock, but not in this patch.

[Antti Koivisto]

Created attachment 126444 [details]
still fixing qt build

[WebKit Review Bot]

Attachment 126444 [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/CSSStyleDeclaration.h:39:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 21 files


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

[Andreas Kling]

(In reply to comment #7)
> (In reply to comment #6)
> > TD;DR
> 
> TD?

Too drunk; didn't review. :/

> > explicit
> 
> How explicit?

I meant you should make this constructor explicit.

[Andreas Kling]

Comment on attachment 126444 [details]
still fixing qt build

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

> Source/WebCore/css/CSSComputedStyleDeclaration.h:48
> +class CSSComputedStyleDeclaration : public CSSStyleDeclaration, private WTF::RefCountedBase {

Why do we need to inherit from RefCountedBase here?
Shadowing ref() and deref() in RefCountedBase with overridden virtuals from CSSStyleDeclaration is a little confusing and the ChangeLog doesn't explain what's going on.

> Source/WebCore/css/StylePropertySet.cpp:50
> +    PropertySetCSSStyleDeclaration(StylePropertySet* propertySet) : m_propertySet(propertySet) { }

explicit PropertySetCSSStyleDeclaration(StylePropertySet* propertySet) : m_propertySet(propertySet) { }
Because you're worth it.

[Antti Koivisto]

Created attachment 126525 [details]
refcount by hand instead of calling to RefCountedBase

[Antti Koivisto]

(In reply to comment #11)
> > Source/WebCore/css/StylePropertySet.cpp:50
> > +    PropertySetCSSStyleDeclaration(StylePropertySet* propertySet) : m_propertySet(propertySet) { }
> 
> explicit PropertySetCSSStyleDeclaration(StylePropertySet* propertySet) : m_propertySet(propertySet) { }
> Because you're worth it.

I don't see what value that would add.

[WebKit Review Bot]

Attachment 126525 [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/CSSStyleDeclaration.h:39:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 21 files


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

[Antti Koivisto]

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