Bug 58244 – Implement experimental CSS rule disabling/enabling feature

• Component: CSS
• Status: RESOLVED INVALID

[Alexander Udalov]

Created attachment 89022 [details]
proposed patch

Implement experimental CSS rule disabling/enabling feature

[Dimitri Glazkov (Google)]

Comment on attachment 89022 [details]
proposed patch

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

Is this part of any spec? It might be worth to tie it into Annevk's CSSOM spec or similar effort.

Disabling a rule is clearly a super-unfriendly thing in WebKit -- forcing recalc of all styles for this one change seems like a waste of cycles. Perhaps we could start with understanding how this could be improved first?

Also, you're adding a bool to the CSSStyleRule. What's the memory impact of this change?

> Source/WebCore/css/CSSStyleRule.cpp:59
> +Document* CSSStyleRule::getDocument()

WebKit convention is to not have "get" prefix on getters. This should just be document()

> Source/WebCore/css/CSSStyleRule.cpp:77
> +    if (!doc) return;

Put return on its own line.

> Source/WebCore/css/CSSStyleRule.cpp:98
> +        result += " !disabled";

What's !disabled?

> Source/WebCore/css/CSSStyleRule.cpp:131
> +        if (doc) {
> +            doc->recalcStyle(Node::Force);
> +        }

No braces around one-liners.

> Source/WebCore/css/CSSStyleRule.cpp:142
> +        if (doc) {
> +            doc->recalcStyle(Node::Force);
> +        }

Ditto.

> Source/WebCore/css/CSSStyleRule.h:58
> +    CSSMutableStyleDeclaration* declaration() {

Func brace on new line.

> Source/WebCore/css/CSSStyleRule.h:64
> +        if (m_enabled)
> +            return m_style.get();
> +        if (!m_emptyStyle)
> +            m_emptyStyle = CSSMutableStyleDeclaration::create();
> +        return m_emptyStyle.get();
> +    }

Why is this in a header?

> Source/WebCore/css/CSSStyleRule.h:89
> +    static RefPtr<CSSMutableStyleDeclaration> m_emptyStyle;

Typically, we make these free-standing statics in the cpp file.

[Alexander Udalov]

This is part of a feature proposed by Tab about disabling/enabling rules, specifically style rules. There is no spec for it yet, only a proposal in the internal doc.
It sure is not the right thing to trigger a full layout of the page after disabling one single rule, so I thought this would be a good place to use someone's help and discuss how it could be implemented better.
Adding a bool is not a final decision either -- I guess we can take 1 bit from m_sourceLine (which surely does not need all of its 32 bits), not to waste any additional memory while still keeping it aligned well.
"!disabled" in cssText() means what it says -- this rule is disabled right now.
I'm attaching another version of patch (runned through style checker this time ><).

[Alexander Udalov]

Created attachment 89032 [details]
proposed patch

[Dave Hyatt]

Interesting idea, but I would suggest getting it into the CSSOM spec before we implement.

[Alexander Udalov]

Created attachment 89194 [details]
proposed patch

[Tab Atkins Jr.]

Hyatt:  Discussions of the APIs have started on the www-style list at <http://lists.w3.org/Archives/Public/www-style/2011Apr/0308.html>.

This API, and a few others discussed in that thread, need some experimental love before proper spec work, I think, so that we know they're actually implementable in reasonable ways.  The list currently seems supportive of the general approaches, though.

[Alexander Udalov]

Created attachment 90198 [details]
proposed patch

[Brent Fulgham]

This approach was abandoned. Please follow work on newer CSS spec implementations.