Bug 58244 - Implement experimental CSS rule disabling/enabling feature
: Implement experimental CSS rule disabling/enabling feature
Status: NEW
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-04-11 10:02 PST by
Modified: 2011-06-23 02:40 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-04-11 10:02:36 PST
Created an attachment (id=89022) [details]
proposed patch

Implement experimental CSS rule disabling/enabling feature
------- Comment #1 From 2011-04-11 10:27:25 PST -------
(From update of attachment 89022 [details])
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.
------- Comment #2 From 2011-04-11 11:07:31 PST -------
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 From 2011-04-11 11:08:11 PST -------
Created an attachment (id=89032) [details]
proposed patch
------- Comment #4 From 2011-04-11 15:32:59 PST -------
Interesting idea, but I would suggest getting it into the CSSOM spec before we implement.
------- Comment #5 From 2011-04-12 06:19:44 PST -------
Created an attachment (id=89194) [details]
proposed patch
------- Comment #6 From 2011-04-12 08:21:40 PST -------
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 From 2011-04-19 07:10:52 PST -------
Created an attachment (id=90198) [details]
proposed patch