Bug 78257 - Use underlying property set to refcount PropertySetCSSStyleDeclaration
Summary: Use underlying property set to refcount PropertySetCSSStyleDeclaration
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-02-09 11:22 PST by Antti Koivisto
Modified: 2012-04-11 20:51 PDT (History)
5 users (show)

See Also:


Attachments
patch (25.70 KB, patch)
2012-02-09 15:13 PST, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
try to fix qt build, other cleanups (26.59 KB, patch)
2012-02-09 16:21 PST, Antti Koivisto
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
still fixing qt build (27.31 KB, patch)
2012-02-09 20:36 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
refcount by hand instead of calling to RefCountedBase (27.33 KB, patch)
2012-02-10 09:35 PST, 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-02-09 11:22:03 PST
StylePropertySetCSSStyleDeclaration refcounting can be done in a cleaner way.
Comment 1 Antti Koivisto 2012-02-09 15:13:08 PST
Created attachment 126383 [details]
patch
Comment 2 Early Warning System Bot 2012-02-09 16:14:26 PST
Comment on attachment 126383 [details]
patch

Attachment 126383 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11473424
Comment 3 Antti Koivisto 2012-02-09 16:21:21 PST
Created attachment 126402 [details]
try to fix qt build, other cleanups
Comment 4 WebKit Review Bot 2012-02-09 16:24:27 PST
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.
Comment 5 Early Warning System Bot 2012-02-09 18:00:23 PST
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
Comment 6 Andreas Kling 2012-02-09 18:25:51 PST
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"
Comment 7 Antti Koivisto 2012-02-09 20:25:32 PST
(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.
Comment 8 Antti Koivisto 2012-02-09 20:36:44 PST
Created attachment 126444 [details]
still fixing qt build
Comment 9 WebKit Review Bot 2012-02-09 20:39:28 PST
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.
Comment 10 Andreas Kling 2012-02-10 01:11:41 PST
(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.
Comment 11 Andreas Kling 2012-02-10 01:19:38 PST
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.
Comment 12 Antti Koivisto 2012-02-10 09:35:21 PST
Created attachment 126525 [details]
refcount by hand instead of calling to RefCountedBase
Comment 13 Antti Koivisto 2012-02-10 09:36:00 PST
(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.
Comment 14 WebKit Review Bot 2012-02-10 09:36:51 PST
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.
Comment 15 Antti Koivisto 2012-02-10 10:23:31 PST
http://trac.webkit.org/changeset/107407