Bug 58244 - Implement experimental CSS rule disabling/enabling feature
Summary: Implement experimental CSS rule disabling/enabling feature
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Alexander Udalov
Depends on:
Reported: 2011-04-11 10:02 PDT by Alexander Udalov
Modified: 2011-06-23 02:40 PDT (History)
10 users (show)

See Also:

proposed patch (4.03 KB, patch)
2011-04-11 10:02 PDT, Alexander Udalov
no flags Details | Formatted Diff | Diff
proposed patch (4.03 KB, patch)
2011-04-11 11:08 PDT, Alexander Udalov
no flags Details | Formatted Diff | Diff
proposed patch (3.76 KB, patch)
2011-04-12 06:19 PDT, Alexander Udalov
no flags Details | Formatted Diff | Diff
proposed patch (3.79 KB, patch)
2011-04-19 07:10 PDT, Alexander Udalov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Udalov 2011-04-11 10:02:36 PDT
Created attachment 89022 [details]
proposed patch

Implement experimental CSS rule disabling/enabling feature
Comment 1 Dimitri Glazkov (Google) 2011-04-11 10:27:25 PDT
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);
> +        }


> 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.
Comment 2 Alexander Udalov 2011-04-11 11:07:31 PDT
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 ><).
Comment 3 Alexander Udalov 2011-04-11 11:08:11 PDT
Created attachment 89032 [details]
proposed patch
Comment 4 Dave Hyatt 2011-04-11 15:32:59 PDT
Interesting idea, but I would suggest getting it into the CSSOM spec before we implement.
Comment 5 Alexander Udalov 2011-04-12 06:19:44 PDT
Created attachment 89194 [details]
proposed patch
Comment 6 Tab Atkins Jr. 2011-04-12 08:21:40 PDT
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.
Comment 7 Alexander Udalov 2011-04-19 07:10:52 PDT
Created attachment 90198 [details]
proposed patch