Summary: | Implement experimental CSS rule disabling/enabling feature | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Udalov <udalov> | ||||||||||
Component: | CSS | Assignee: | Alexander Udalov <udalov> | ||||||||||
Status: | RESOLVED INVALID | ||||||||||||
Severity: | Enhancement | CC: | ap, arv, bfulgham, hyatt, jackalmage, koivisto, komoroske, mike, peter, shanestephens, simon.fraser | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
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. 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 ><). Created attachment 89032 [details]
proposed patch
Interesting idea, but I would suggest getting it into the CSSOM spec before we implement. Created attachment 89194 [details]
proposed patch
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. Created attachment 90198 [details]
proposed patch
This approach was abandoned. Please follow work on newer CSS spec implementations. |
Created attachment 89022 [details] proposed patch Implement experimental CSS rule disabling/enabling feature