Created attachment 125337 [details] patch

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.

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

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

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.

(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)

Created attachment 125360 [details] updated patch - fix a keyframe crasher - try to fix the qt build

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.

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

(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.