Bug 71382 – Devirtualize CSSRule.

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

[Andreas Kling]

The only remaining virtual method in CSSRule is the destructor.
If we make it non-virtual, each instance will shrink by one pointer (the vptr.)

[Andreas Kling]

Created attachment 113347 [details]
Possibly a patch

[Antti Koivisto]

Comment on attachment 113347 [details]
Possibly a patch

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

r=me with a few comments.

> Source/WebCore/css/CSSImportRule.h:60
> +    class ImportedStyleSheetClient : public CachedStyleSheetClient {

This could use a comment explaining why the class itself can't implement the interface.

> Source/WebCore/css/CSSRule.cpp:76
> +void CSSRule::deref()
> +{
> +    if (!derefBase())
> +        return;
> +    switch (type()) {
> +    case UNKNOWN_RULE:

The deletion part should be factored into a function. deref() can be inlined.

> Source/WebCore/css/CSSRule.h:40
> +    // Override RefCounted's deref() to ensure operator delete is called on
> +    // the appropriate subclass type.
> +    void deref();

You should explain that this class is non-virtual for a reason.

[Darin Adler]

Comment on attachment 113347 [details]
Possibly a patch

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

> Source/WebCore/css/CSSRule.h:36
> +    ~CSSRule() { }

This base class destructor should be protected.

[Antti Koivisto]

Maybe it would make sense to have RefCounted do deleting by calling

static_cast<T*>(this)->destruct();

instead of calling delete directly (with the default implementation just doing delete)? This way you would only need to override destruct() in cases like this. We may want to devirtualize some more in the future...

[Darin Adler]

(In reply to comment #4)
> Maybe it would make sense to have RefCounted do deleting by calling
> 
> static_cast<T*>(this)->destruct();
> 
> instead of calling delete directly (with the default implementation just doing delete)? This way you would only need to override destruct() in cases like this. We may want to devirtualize some more in the future.

Seems OK.

I would call the function “destroy”, not “destruct”, though.

[Andreas Kling]

Committed r99171: <http://trac.webkit.org/changeset/99171>