| Summary: | Implement CSS Variables Standard. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Luke Macpherson <macpherson> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | Luke Macpherson <macpherson> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | abarth, adrian.yanes, arv, catfish.man, cmarcelo, darin, dglazkov, divya, dstorey, eoconnor, eric, gustavo, hyatt, jdaggett, joethomas, kling, koivisto, macpherson, mathias, menard, mikelawther, ojan, peter, philn, rafael.lobo, rakuco, vestbo, webkit-bugzilla15a, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | WebExposed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 86338, 86424, 86575, 87462, 112108, 112415 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Description
Luke Macpherson
2012-05-03 22:09:40 PDT
Created attachment 140158 [details]
Patch
Comment on attachment 140158 [details] Patch Attachment 140158 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12543228 Comment on attachment 140158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140158&action=review Cool! Looking forward to css variables. :-) I may have some nitpicks, feel free to ignore if they're not valid. > Source/WebCore/css/CSSParser.cpp:2730 > + for (unsigned i = 0; i < value->size(); i++) { Maybe you should do: for(unsigned i = 0, size = value->size(); i < size ... > Source/WebCore/css/CSSValue.h:87 > + bool isVariableValue() const {return m_classType == VariableClass; } Maybe missing space before return? I don't remember if this is on style guidelines. > Source/WebCore/css/StylePropertySet.cpp:479 > + for (int n = m_properties.size() - 1 ; n >= 0; --n) { Is there any convention about using "n" or "i" on a for-loop? Comment on attachment 140158 [details] Patch Attachment 140158 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12631053 Comment on attachment 140158 [details] Patch Attachment 140158 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12632054 New failing tests: fast/css/variables/shorthand.html Created attachment 140164 [details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 140158 [details] Patch Attachment 140158 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12629071 New failing tests: fast/css/variables/shorthand.html Created attachment 140172 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 140158 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=140158&action=review >> Source/WebCore/css/CSSParser.cpp:2730 >> + for (unsigned i = 0; i < value->size(); i++) { > > Maybe you should do: for(unsigned i = 0, size = value->size(); i < size ... Or just move it outside the for declaration in a local variable as many place in WebKit is doing. But here value->size() is not a big operation so no big deal. It's not an iterator here. > Source/WebCore/css/CSSParser.cpp:2736 > + return false; Seems like you need to return true if you add it no? if no why you need a bool as a return. >> Source/WebCore/css/CSSValue.h:87 >> + bool isVariableValue() const {return m_classType == VariableClass; } > > Maybe missing space before return? I don't remember if this is on style guidelines. It's missing. Created attachment 140414 [details]
Patch
Comment on attachment 140414 [details] Patch Attachment 140414 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12634562 Comment on attachment 140414 [details] Patch Attachment 140414 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12637290 Created attachment 140468 [details]
Fixes variable cascade, adds additional tests.
Comment on attachment 140468 [details] Fixes variable cascade, adds additional tests. Attachment 140468 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12626929 Comment on attachment 140468 [details] Fixes variable cascade, adds additional tests. Attachment 140468 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12628937 Created attachment 140472 [details]
Fix efl build.
Comment on attachment 140472 [details] Fix efl build. Attachment 140472 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12507954 Comment on attachment 140472 [details] Fix efl build. Attachment 140472 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12642238 New failing tests: http/tests/appcache/different-origin-manifest.html http/tests/appcache/document-write-html-element-2.html accessibility/aria-checkbox-checked.html http/tests/appcache/document-write-html-element.html animations/3d/matrix-transform-type-animation.html http/tests/appcache/cyrillic-uri.html accessibility/anchor-linked-anonymous-block-crash.html animations/animation-add-events-in-handler.html http/tests/appcache/deferred-events-delete-while-raising.html animations/additive-transform-animations.html animations/3d/replace-filling-transform.html http/tests/appcache/deferred-events-delete-while-raising-timer.html http/tests/appcache/crash-when-navigating-away-then-back.html animations/animation-border-overflow.html animations/animation-direction-alternate-reverse.html http/tests/appcache/credential-url.html accessibility/anonymous-render-block-in-continuation-causes-crash.html http/tests/appcache/access-via-redirect.php http/tests/appcache/different-scheme.html animations/3d/change-transform-in-end-event.html http/tests/appcache/destroyed-frame.html animations/animation-controller-drt-api.html http/tests/appcache/deferred-events.html animations/3d/state-at-end-event-transform.html animations/3d/transform-origin-vs-functions.html animations/animation-css-rule-types.html http/tests/appcache/empty-manifest.html http/tests/appcache/disabled.html accessibility/adjacent-continuations-cause-assertion-failure.html http/tests/appcache/detached-iframe.html Created attachment 140484 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 140611 [details]
Fix windows build.
Comment on attachment 140611 [details] Fix windows build. View in context: https://bugs.webkit.org/attachment.cgi?id=140611&action=review > Source/WebCore/css/CSSParser.cpp:1443 > + addProperty(propId, cssValuePool().createValue(value->string, CSSPrimitiveValue::CSS_VARIABLE_NAME), important); CSSValuePool won't actually pool this, so it'd be more clear to just use CSSPrimitiveValue::create(...) directly. > Source/WebCore/css/CSSParser.cpp:2729 > + StringBuilder b; We don't use single-character names like this in WebKit. 'builder' would be more appropriate. > Source/WebCore/css/CSSParser.cpp:2734 > + for (unsigned i = 0, size = value->size(); i < size; i++) { > + String v = value->valueAt(i)->createCSSValue()->cssText(); > + b.append(v); > + b.append(' '); > + } This will construct a string that ends with an extra space (' ') character. Also, there's no need for the temporary String. > Source/WebCore/css/CSSVariableValue.cpp:34 > +String CSSVariableValue::customCssText() const { return m_value; } Why on earth are we adding a new file for this trivial getter? > Source/WebCore/css/CSSVariableValue.h:40 > + static PassRefPtr<CSSVariableValue> create(String name, String value = String()) I don't see any call sites that omit the second argument, so it shouldn't have a default value. > Source/WebCore/css/CSSVariableValue.h:48 > + const String& name() { return m_name; } > + const String& value() { return m_value; } These functions should be marked const. > Source/WebCore/css/CSSVariableValue.h:50 > + PassRefPtr<CSSValue> valueForProperty(CSSPropertyID); Implementation missing. > Source/WebCore/css/CSSVariableValue.h:53 > + CSSVariableValue(String name, String value) These arguments should be passed by const reference to avoid extra ref/deref churn. Same comment for other functions that take String arguments. > Source/WebCore/css/CSSVariableValue.h:61 > + const String m_name; > + const String m_value; These two should probably be AtomicStrings. At the very least, m_name should be, or this will scale horribly. > Source/WebCore/css/StyleResolver.cpp:3109 > +void StyleResolver::applyProperty(CSSPropertyID id, CSSValue *value, HashSet<String> *cycles) 'cycles' is not a good name, something like 'parsedVariables' or similar would be more informative. Coding style, asterisk placement. > Source/WebCore/css/StyleResolver.cpp:3140 > + if (!CSSParser::parseValue(resultSet.get(), id, style()->variable(name), false, CSSQuirksMode, 0)) Do we actually want CSSQuirksMode here? > Source/WebCore/css/StyleResolver.cpp:3143 > + HashSet<String> nextLevel = cycles ? HashSet<String>(*cycles) : HashSet<String>(); What does 'nextLevel' mean in this context? > Source/WebCore/css/StyleResolver.cpp:3146 > + for (unsigned i = 0; i < resultSet->propertyCount(); i++) { Can propertyCount() be anything other than 1 here? If not, we should have simply ASSERT that and grab propertyAt(0) instead of looping. > Source/WebCore/rendering/style/RenderStyle.h:409 > + bool hasVariable(String name) const { return rareInheritedData->m_variables.contains(name); } rareInheritedData could be null here. > Source/WebCore/rendering/style/StyleRareInheritedData.cpp:68 > + , m_variables(HashMap<String, String>()) This line is not necessary. > Source/WebCore/rendering/style/StyleRareInheritedData.cpp:186 > + && m_variables == o.m_variables; We should avoid comparing two hashmaps here as that involves iterating them, comparing each entry. > Source/WebCore/rendering/style/StyleRareInheritedData.h:116 > + HashMap<String, String> m_variables; This will bloat StyleRareInheritedData by a considerable amount, which is unacceptable. It would be better to use an OwnPtr to hold this hash map while minimizing overhead for instances that don't have one. Comment on attachment 140611 [details] Fix windows build. View in context: https://bugs.webkit.org/attachment.cgi?id=140611&action=review >> Source/WebCore/css/CSSParser.cpp:1443 >> + addProperty(propId, cssValuePool().createValue(value->string, CSSPrimitiveValue::CSS_VARIABLE_NAME), important); > > CSSValuePool won't actually pool this, so it'd be more clear to just use CSSPrimitiveValue::create(...) directly. Done. >> Source/WebCore/css/CSSParser.cpp:2729 >> + StringBuilder b; > > We don't use single-character names like this in WebKit. 'builder' would be more appropriate. Done. >> Source/WebCore/css/CSSParser.cpp:2734 >> + } > > This will construct a string that ends with an extra space (' ') character. > Also, there's no need for the temporary String. Done. >> Source/WebCore/css/CSSVariableValue.cpp:34 >> +String CSSVariableValue::customCssText() const { return m_value; } > > Why on earth are we adding a new file for this trivial getter? Removed. >> Source/WebCore/css/CSSVariableValue.h:40 >> + static PassRefPtr<CSSVariableValue> create(String name, String value = String()) > > I don't see any call sites that omit the second argument, so it shouldn't have a default value. Done. >> Source/WebCore/css/CSSVariableValue.h:48 >> + const String& value() { return m_value; } > > These functions should be marked const. Done. >> Source/WebCore/css/CSSVariableValue.h:50 >> + PassRefPtr<CSSValue> valueForProperty(CSSPropertyID); > > Implementation missing. Removed. >> Source/WebCore/css/CSSVariableValue.h:53 >> + CSSVariableValue(String name, String value) > > These arguments should be passed by const reference to avoid extra ref/deref churn. > Same comment for other functions that take String arguments. Done. >> Source/WebCore/css/StyleResolver.cpp:3109 >> +void StyleResolver::applyProperty(CSSPropertyID id, CSSValue *value, HashSet<String> *cycles) > > 'cycles' is not a good name, something like 'parsedVariables' or similar would be more informative. > Coding style, asterisk placement. Done. >> Source/WebCore/css/StyleResolver.cpp:3140 >> + if (!CSSParser::parseValue(resultSet.get(), id, style()->variable(name), false, CSSQuirksMode, 0)) > > Do we actually want CSSQuirksMode here? Changed to Strict - maybe we need to find out what was used elsewhere, but would that be at the point the variable was defined, or the point that it is used? >> Source/WebCore/css/StyleResolver.cpp:3143 >> + HashSet<String> nextLevel = cycles ? HashSet<String>(*cycles) : HashSet<String>(); > > What does 'nextLevel' mean in this context? I've changed the way this works to share a single copy of the map that is modified as we traverse the tree of variable definitions. >> Source/WebCore/css/StyleResolver.cpp:3146 >> + for (unsigned i = 0; i < resultSet->propertyCount(); i++) { > > Can propertyCount() be anything other than 1 here? > If not, we should have simply ASSERT that and grab propertyAt(0) instead of looping. Yes it can, when the property being handled is a shorthand the parser will fill this with the expanded set of properties. >> Source/WebCore/rendering/style/RenderStyle.h:409 >> + bool hasVariable(String name) const { return rareInheritedData->m_variables.contains(name); } > > rareInheritedData could be null here. If I understand correctly, this will never be null, but if access() has never been called it will point to our parent's rareInheritedData. This is done to avoid unnecessary copies of rareInheritedData for nodes that don't change any rare inherited properties. >> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:68 >> + , m_variables(HashMap<String, String>()) > > This line is not necessary. Removed. >> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:186 >> + && m_variables == o.m_variables; > > We should avoid comparing two hashmaps here as that involves iterating them, comparing each entry. I'm not sure that's avoidable - variables are essentially user-defined properties, so I think for correctness we should compare them. >> Source/WebCore/rendering/style/StyleRareInheritedData.h:116 >> + HashMap<String, String> m_variables; > > This will bloat StyleRareInheritedData by a considerable amount, which is unacceptable. > It would be better to use an OwnPtr to hold this hash map while minimizing overhead for instances that don't have one. I've changed to having a separate copy-on-write DataRef on RenderStyle. This means that the HashMap will only ever be minimally instantiated during cascade (and unused if variables are not used on the page.) Created attachment 140666 [details]
Fixes for Kling's comments.
Comment on attachment 140666 [details] Fixes for Kling's comments. Attachment 140666 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12650257 Comment on attachment 140666 [details] Fixes for Kling's comments. Attachment 140666 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12655183 Comment on attachment 140666 [details] Fixes for Kling's comments. Attachment 140666 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12642545 Created attachment 140678 [details]
Patch
Comment on attachment 140678 [details] Patch Attachment 140678 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12643563 Comment on attachment 140678 [details] Patch Attachment 140678 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12652286 Created attachment 140684 [details]
Build file fixes.
Comment on attachment 140684 [details] Build file fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=140684&action=review Just some typo, I will look into details more later :) > Source/WebCore/GNUmakefile.list.am:1666 > + Source/WebCore/css/CSSValueList.cpp \ Tab issue here. > Source/WebCore/WebCore.gypi:2486 > + 'css/CSSValueList.cpp', Whitespace. > Source/WebCore/css/CSSPrimitiveValue.h:127 > + CSS_VARIABLE_NAME = 115, extra , Comment on attachment 140684 [details] Build file fixes. View in context: https://bugs.webkit.org/attachment.cgi?id=140684&action=review >> Source/WebCore/GNUmakefile.list.am:1666 >> + Source/WebCore/css/CSSValueList.cpp \ > > Tab issue here. Done. >> Source/WebCore/WebCore.gypi:2486 >> + 'css/CSSValueList.cpp', > > Whitespace. Done. >> Source/WebCore/css/CSSPrimitiveValue.h:127 >> + CSS_VARIABLE_NAME = 115, > > extra , I don't know if we have a policy on that, but in general I think it's a good idea because it means the next person's diff need only add a line, not modify an existing one. That makes patches go in more cleanly, and makes repo tools like blame work better. Created attachment 140805 [details]
Whitespace fixes to build files.
Created attachment 140860 [details]
Use AtomicString for variable names.
Created attachment 141078 [details]
Add more tests.
Created attachment 141327 [details]
Introduce ENABLE(CSS_VARIABLES) flag.
Comment on attachment 141327 [details] Introduce ENABLE(CSS_VARIABLES) flag. Attachment 141327 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12675157 New failing tests: fast/css/variables/redefinition.html fast/css/variables/shorthand.html fast/css/variables/variable-chain.html fast/css/variables/computed-style.html fast/css/variables/use-before-defined.html fast/css/variables/inline-styles.html fast/css/variables/inherited-values.html fast/css/variables/colors-test.html fast/css/variables/var-inside-shorthand.html Created attachment 141360 [details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 141327 [details] Introduce ENABLE(CSS_VARIABLES) flag. Attachment 141327 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12669353 New failing tests: fast/css/variables/redefinition.html fast/css/variables/shorthand.html fast/css/variables/variable-chain.html fast/css/variables/computed-style.html fast/css/variables/use-before-defined.html fast/css/variables/inline-styles.html fast/css/variables/inherited-values.html fast/css/variables/colors-test.html fast/css/variables/var-inside-shorthand.html Created attachment 141370 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
I think this is starting to look really nice. I think we can start landing this stuff!! * Land CSS_VARIABLES flag * Adding HighPriority/LowPriorityProperties * Land tests and disable them * Land bits and pieces and start making tests pass. Really exciting stuff. I volunteer to review smaller patches. Created attachment 142118 [details]
Rebaseline.
Comment on attachment 142118 [details] Rebaseline. Attachment 142118 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12694924 New failing tests: fast/css/variables/redefinition.html fast/css/variables/shorthand.html fast/css/variables/variable-chain.html fast/css/variables/computed-style.html fast/css/variables/use-before-defined.html fast/css/variables/inline-styles.html fast/css/variables/inherited-values.html fast/css/variables/colors-test.html fast/css/variables/var-inside-shorthand.html Created attachment 142192 [details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 142426 [details]
checkpoint after merge.
On a related note, I can't be the only one who thinks the CSS syntax is beyond awkward: -webkit-var-foo: red; background-color: -webkit-var(foo); IMO this would look heaps better as: $foo: red; background-color: $foo; I might be (fashionably) late to the party, but it seems a shame to waste this opportunity to make CSS look a little *nicer* for a change. (In reply to comment #46) > On a related note, I can't be the only one who thinks the CSS syntax is beyond awkward: > $foo: red; > background-color: $foo; > > I might be (fashionably) late to the party, but it seems a shame to waste this opportunity to make CSS look a little *nicer* for a change. Agreed+++ Tab had some rationale for why this can't be the case. But I think it would definitely be so much better than this var prefix. Please take feedback on the spec to the CSS working group (www-style@w3.org). Personally I'm happy to do whatever can get agreement from the working group and thus support from other browsers. If I recall correctly a $foo like syntax was discussed and rejected at the TPAC meeting. Created attachment 142938 [details]
Handle properties that generate value lists (eg. transform).
(In reply to comment #48) > Please take feedback on the spec to the CSS working group (www-style@w3.org). Personally I'm happy to do whatever can get agreement from the working group and thus support from other browsers. If I recall correctly a $foo like syntax was discussed and rejected at the TPAC meeting. Peeps, please speak up on this thread: http://lists.w3.org/Archives/Public/www-style/2012May/thread.html#msg744. Tab is trying to make an argument for it, but there's resistance. We probably shouldn't land this feature before the WG has decided what sort of bike shed they want. (In reply to comment #51) > We probably shouldn't land this feature before the WG has decided what sort of bike shed they want. Just the opposite! Having _something_ to actually try out and play with will go a long way in settling down the bikeshedding debates. We shouldn't be shipping it -- that's for sure. But landing -- the sooner the better. Also -- please don't just stand on the sidelines of the discussion. You have an opinion and respect of many browser developers. It would be awesome if you could be part of it. Or at least make Simon do it :) Created attachment 143921 [details]
Patch
Created attachment 143937 [details]
Patch
Comment on attachment 143937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=143937&action=review Aside from one place, the rest looks like one patch, or at least something that would be hard to split up further. Antti, Master Kling -- WDYT? By the way, the debate in the WG is about syntax, which is the trivial part to change for a behind-flag implementation. The meat of the functionality has been long agreed upon. > Source/WebCore/css/WebKitCSSTransformValue.cpp:74 > - break; > + StringBuilder result; > + if (m_type != UnknownTransformOperation) { > + result.append(transformName[m_type]); > + result.append('('); > + result.append(CSSValueList::customCssText()); > + result.append(')'); There's a small patch on its own. > LayoutTests/ChangeLog:11 > + * fast/css/variables/colors-test.html: Added. > + * fast/css/variables/complex-cycle-expected.html: Added. > + * fast/css/variables/complex-cycle.html: Added. Don't need this changelog anymore. (In reply to comment #55) > By the way, the debate in the WG is about syntax, which is the > trivial part to change for a behind-flag implementation. The meat of > the functionality has been long agreed upon. That's not really right. Most folks in the WG agree that this is a good proposal but I think a lot of the details have yet to be fully spec'ed out completely and I can see several areas where the behavior could potentially change - validation of variable definitions, treatment of inherit/initial in variable definition, use value for undefined variables, computed value for values including invalid variable use, etc. The notation issues are just what attracts the most attention. Created attachment 144419 [details]
Patch
Comment on attachment 144419 [details]
Patch
This patch is now small enough to be reviewed on its own.
(In reply to comment #58) > (From update of attachment 144419 [details]) > This patch is now small enough to be reviewed on its own. For a bit more context: I would really like to get CSS variables landed soon, so that Web developers could start experimenting with it in nightlies/canary and: 1) understand what it really means 2) see whether it really does work as a replacement for shadow pseudos in shadow DOM. We can tweak the syntax later as it settles. Comment on attachment 144419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144419&action=review > Source/WebCore/ChangeLog:44 > + * CMakeLists.txt: Nitpick from the CMake side: the file is mentioned here in the ChangeLog but does not seem to be changed in the patch. Even though the options are present in FeatureList.pm and Source/cmake/WebKitFeatures.cmake, the definition in Source/cmakeconfig.h.cmake is still missing, which means CMake-based ports will always have the feature undefined so far. Comment on attachment 144419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144419&action=review This is difficult to review. It is supposed be an experimental feature so things don't need to be optimal and regressing performance may be ok. We should make sure the architectural direction is right though as it is likely to stick. r- is for a bunch of smaller issues but please think if you can come up with architecture that does not involve RenderStyle (or explain why it makes sense). > Source/WebCore/ChangeLog:26 > + Tests: fast/css/variables/colors-test-expected.html > + fast/css/variables/colors-test.html > + fast/css/variables/complex-cycle-expected.html > + fast/css/variables/complex-cycle.html I don't see the tests in the patch. > Source/WebCore/css/CSSGrammar.y:138 > +%token VAR_SYM VARFUNCTION would seem more appropriate with the current syntax. > Source/WebCore/css/CSSParser.cpp:1196 > + AtomicString name = static_cast<CSSVariableValue*>(property.value())->name(); const AtomicString& > Source/WebCore/css/CSSParser.cpp:2840 > +bool CSSParser::storeVariableDeclaration(const CSSParserString& name, PassOwnPtr<CSSParserValueList> value, bool important) What does the returned boolean signify? It is always true. > Source/WebCore/css/CSSParser.cpp:2848 > + StringBuilder builder; > + for (unsigned i = 0, size = value->size(); i < size; i++) { > + if (i) > + builder.append(' '); > + builder.append(value->valueAt(i)->createCSSValue()->cssText()); > + } > + addProperty(CSSPropertyVariable, CSSVariableValue::create(name, builder.toString()), important, false); Letting the parser parse the string as expr and then smashing it back together into string by concatenating cssTexts seems less than ideal (and the API method cssText shouldn't be used for internal purposes in general). Can't we have a grammar that just gives the string? > Source/WebCore/css/CSSPrimitiveValue.cpp:1090 > +#if ENABLE(CSS_VARIABLES) > +String CSSPrimitiveValue::customCssText(const HashMap<AtomicString, String>& variables) const > +{ > + if (m_primitiveUnitType == CSS_VARIABLE_NAME && variables.contains(m_value.string)) > + return variables.get(m_value.string); > + return customCssText(); > +} > +#endif As far as I can see the idea here (and in other similar functions you add) is to find the value for a variable name. This little to do with the CSSOM cssText() function (which is for serializing style and shouldn't be used for internal purposes) so I'm confused about the naming and the factoring in general. Why invoke this at all if it is not CSS_VARIABLE_NAME? > Source/WebCore/css/CSSVariableValue.h:17 > + * 3. Neither the name of Apple Computer, Inc. ("Apple") nor the names of > + * its contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY Are you sure this is the legal text you want? > Source/WebCore/css/StyleResolver.cpp:2848 > +#if ENABLE(CSS_VARIABLES) > + // First apply all variable definitions, as they may be used during application of later properties. > + applyMatchedProperties<VariableDefinitions>(matchResult, false, 0, matchResult.matchedProperties.size() - 1, applyInheritedOnly); > + applyMatchedProperties<VariableDefinitions>(matchResult, true, matchResult.ranges.firstAuthorRule, matchResult.ranges.lastAuthorRule, applyInheritedOnly); > + applyMatchedProperties<VariableDefinitions>(matchResult, true, matchResult.ranges.firstUserRule, matchResult.ranges.lastUserRule, applyInheritedOnly); > + applyMatchedProperties<VariableDefinitions>(matchResult, true, matchResult.ranges.firstUARule, matchResult.ranges.lastUARule, applyInheritedOnly); > +#endif Additional iterations searching for variables are likely to slow down common case. We need a better solution. Should be ok for experimental implementation though. > Source/WebCore/css/StyleResolver.cpp:3176 > +static bool hasVariableReference(CSSValue* value) > +{ > + if (value->isPrimitiveValue() && static_cast<CSSPrimitiveValue*>(value)->isVariableName()) > + return true; > + > + for (CSSValueListIterator i = value; i.hasMore(); i.advance()) { > + if (hasVariableReference(i.value())) > + return true; > + } > + > + return false; > +} An efficient implementation would cache the existence of variable ref to a bit in the top level CSSValue. > Source/WebCore/css/StyleResolver.cpp:3183 > +void StyleResolver::resolveVariables(CSSPropertyID id, CSSValue* value, HashSet<String>* knownExpressions) > { > + if (!hasVariableReference(value)) { > + applyProperty(id, value); > + return; > + } The top level call site already calls hasVariableReference(). This could move to the loop below. > Source/WebCore/css/StyleResolver.cpp:3185 > + String expression = value->cssText(*style()->variables()); As noted above calling something called cssText() here is strange and (as noted below) having variables part of the RenderStyle seems suspicious. > Source/WebCore/css/StyleResolver.cpp:3188 > + if (!knownExpressions->add(getPropertyName(id) + expression).isNewEntry) > + return; // cycle detected. The key shouldn't be formed by concatenating strings. You can use pair<> of any hashable type as hash key. I don't understand how this prevents cycles. Could you explain? > Source/WebCore/css/StyleResolver.cpp:3192 > + RefPtr<StylePropertySet> resultSet = StylePropertySet::create(); > + if (!CSSParser::parseValue(resultSet.get(), id, expression, false, CSSStrictMode, 0)) > + return; // expression failed to parse. Parsing CSS values on style resolve time is going to be horribly slow. A good approach might be to cache the parsed values to the variable declaration value itself (with id/type key). > Source/WebCore/css/StyleResolver.cpp:3205 > +void StyleResolver::applyProperty(CSSPropertyID id, CSSValue* value) > +{ > +#if ENABLE(CSS_VARIABLES) > + if (hasVariableReference(value)) { > + HashSet<String> knownExpressions; Initializing and tearing down a HashSet for applying a single property seems costly. > Source/WebCore/css/StyleResolver.cpp:3225 > - // check lookup table for implementations and use when available > - const PropertyHandler& handler = m_styleBuilder.propertyHandler(id); > - if (handler.isValid()) { > - if (isInherit) > - handler.applyInheritValue(this); > - else if (isInitial) > - handler.applyInitialValue(this); > - else > - handler.applyValue(this, value); > - return; > + // Check lookup table for implementations and use when available. > + if (id >= firstCSSProperty) { Why this change? > Source/WebCore/rendering/style/RenderStyle.h:216 > +#if ENABLE(CSS_VARIABLES) > + DataRef<StyleVariableData> m_variables; > +#endif Having variables as part of the RenderStyle seems architecturally suspicious. The RenderStyle is the input for rendering and layout. Rendering sees variables as resolved values so it doesn't need to know about them. We don't have a precedent, this is the first CSS property type that has no effect after style resolve. I'm not sure what the right way to go here is. In practical terms adding a pointer to RenderStyle has significant memory cost whether variables are used in a document or not. > Source/WebCore/rendering/style/StyleVariableData.h:52 > +class StyleVariableData : public RefCounted<StyleVariableData> { > +public: > + static PassRefPtr<StyleVariableData> create() { return adoptRef(new StyleVariableData()); } > + PassRefPtr<StyleVariableData> copy() const { return adoptRef(new StyleVariableData(*this)); } > + > + bool operator==(const StyleVariableData& other) const { return other.m_data == m_data; } > + bool operator!=(const StyleVariableData& other) const { return !(*this == other); } > + > + void setVariable(const AtomicString& name, const String& value) { m_data.set(name, value); } > + bool hasVariable(const AtomicString& name) const { return m_data.contains(name); } > + String variable(const AtomicString& name) const { return m_data.get(name); } > + > + HashMap<AtomicString, String> m_data; HashMaps are large. This class is very cost if there are definitions on multiple levels and the instances don't get shared. Comment on attachment 144419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144419&action=review >> Source/WebCore/ChangeLog:26 >> + fast/css/variables/complex-cycle.html > > I don't see the tests in the patch. Tests have already been landed (see bugs 86575 and 86987) I have a few more here to land as well, but that will be in a separate patch. >> Source/WebCore/css/CSSGrammar.y:138 >> +%token VAR_SYM > > VARFUNCTION would seem more appropriate with the current syntax. done. >> Source/WebCore/css/CSSParser.cpp:1196 >> + AtomicString name = static_cast<CSSVariableValue*>(property.value())->name(); > > const AtomicString& done. >> Source/WebCore/css/CSSParser.cpp:2840 >> +bool CSSParser::storeVariableDeclaration(const CSSParserString& name, PassOwnPtr<CSSParserValueList> value, bool important) > > What does the returned boolean signify? It is always true. Changed to void. >> Source/WebCore/css/CSSParser.cpp:2848 >> + addProperty(CSSPropertyVariable, CSSVariableValue::create(name, builder.toString()), important, false); > > Letting the parser parse the string as expr and then smashing it back together into string by concatenating cssTexts seems less than ideal (and the API method cssText shouldn't be used for internal purposes in general). Can't we have a grammar that just gives the string? Yes, but that seems to come at the cost of duplicating the whole grammar below expr and returning just the string, unless we reduce how strict we are about what a valid variable value is. Right now it can only be a value that would be valid for an existing CSS property. Doing so would be ok because we always have to re-validate once the property that a value is used for is known, so I'll look into it. >> Source/WebCore/css/CSSPrimitiveValue.cpp:1090 >> +#endif > > As far as I can see the idea here (and in other similar functions you add) is to find the value for a variable name. This little to do with the CSSOM cssText() function (which is for serializing style and shouldn't be used for internal purposes) so I'm confused about the naming and the factoring in general. Why invoke this at all if it is not CSS_VARIABLE_NAME? The problem being solved here is to resolve the variable references inside a CSSValue. Because variables can exist deep inside value lists, calc expressions etc. it is not sufficient to only check if the value itself is a variable. Happy to change the name if that helps to make it clearer. >> Source/WebCore/css/CSSVariableValue.h:17 >> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY > > Are you sure this is the legal text you want? Seems likely not :) >> Source/WebCore/css/StyleResolver.cpp:2848 >> +#endif > > Additional iterations searching for variables are likely to slow down common case. We need a better solution. Should be ok for experimental implementation though. We already do something like 9 of these passes - I'm not sure the cost is actually significant since we only do work when a variable is encountered. >> Source/WebCore/css/StyleResolver.cpp:3176 >> +} > > An efficient implementation would cache the existence of variable ref to a bit in the top level CSSValue. Agreed. I want to do caching here and elsewhere in future iterations. >> Source/WebCore/css/StyleResolver.cpp:3183 >> + } > > The top level call site already calls hasVariableReference(). This could move to the loop below. Agreed - I actually had that locally but not uploaded yet. >> Source/WebCore/css/StyleResolver.cpp:3188 >> + return; // cycle detected. > > The key shouldn't be formed by concatenating strings. You can use pair<> of any hashable type as hash key. > > I don't understand how this prevents cycles. Could you explain? Good tip re:Pair - I didn't think of that. This took me a while to figure out - initially I was trying to keep a HashSet of seen variable names, which worked well when solving the simple case of having only a single variable in an expression, however when dealing with list values it turned out to be unsuitable. For example: -webkit-var-a: 0; -webkit-var-b: -webkit-var(a); some-list-valued-property: -webkit-var(a) -webkit-var(b); a) resolves to: some-list-valued-property: 0 -webkit-var(a); b) resolves to: some-list-valued-property: 0 0; Here keeping a list of dereferenced variables would incorrectly detect a cycle, because a is dereferenced twice, but in different contexts. I first thought about trying to track the variables used at each position, but that turned out to be way too complicated. A more elegant solution is to track which expressions you have already tried to resolve, and bail when you find a duplicate expression. Cases like "-webkit-var-a: -webkit-var(a) green;" should be eliminated when they fail to re-parse once the expression list gets too long for the property to which they are applied, but I'll add some test cases for that. >> Source/WebCore/css/StyleResolver.cpp:3192 >> + return; // expression failed to parse. > > Parsing CSS values on style resolve time is going to be horribly slow. A good approach might be to cache the parsed values to the variable declaration value itself (with id/type key). Absolutely agreed, adding caching here will be critical to performance, and the subject of follow up work. >> Source/WebCore/css/StyleResolver.cpp:3205 >> + HashSet<String> knownExpressions; > > Initializing and tearing down a HashSet for applying a single property seems costly. Fair point - since knownExpressions should be very small in most cases (I don't expect massively long chains of variables), using Vector<String> here would probably be fine too. >> Source/WebCore/css/StyleResolver.cpp:3225 >> + if (id >= firstCSSProperty) { > > Why this change? Because css variables are essentially user-defined properties, I've made CSSPropertyVariable come before firstCSSProperty in the enum. Might be better to merge this with the case CSSPropertyVariable below and just early return instead. >> Source/WebCore/rendering/style/StyleVariableData.h:52 >> + HashMap<AtomicString, String> m_data; > > HashMaps are large. This class is very cost if there are definitions on multiple levels and the instances don't get shared. Instances are shared copy-on-write up the inheritance chain. It's hard to predict precisely what developers do, but our expectation is that variables will mostly be defined at the root level in the DOM, and at specific boundaries (like the root of a new component). In this scenario there will be few instances of HashMap required as they will just be shared with children. Created attachment 144703 [details]
Patch
Comment on attachment 144703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144703&action=review > Source/WebCore/rendering/style/RenderStyle.h:215 > + DataRef<StyleVariableData> m_variables; If we're going to keep variables on RenderStyle, then it should be on StyleRareInheritedData in order to conserve memory for pages that don't use variables. > Source/WebCore/rendering/style/RenderStyle.h:414 > + bool hasVariable(const AtomicString& name) const { return m_variables->hasVariable(name); } > + String variable(const AtomicString& name) const { return m_variables->variable(name); } These two are unused. Do you have future patches that will need this? Can we add them as part of those patches? > Source/WebCore/rendering/style/StyleVariableData.h:50 > + bool hasVariable(const AtomicString& name) const { return m_data.contains(name); } > + String variable(const AtomicString& name) const { return m_data.get(name); } Ditto re being unused. Created attachment 144971 [details]
Patch
Comment on attachment 144703 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144703&action=review >> Source/WebCore/rendering/style/RenderStyle.h:215 >> + DataRef<StyleVariableData> m_variables; > > If we're going to keep variables on RenderStyle, then it should be on StyleRareInheritedData in order to conserve memory for pages that don't use variables. Done. >> Source/WebCore/rendering/style/RenderStyle.h:414 >> + String variable(const AtomicString& name) const { return m_variables->variable(name); } > > These two are unused. Do you have future patches that will need this? Can we add them as part of those patches? Removed. >> Source/WebCore/rendering/style/StyleVariableData.h:50 >> + String variable(const AtomicString& name) const { return m_data.get(name); } > > Ditto re being unused. Removed. An alternative to putting these on RenderStyle or RareInheritedData, would be to have a document-level (global?) HashMap from RenderStyle to StyleVariableData for each RenderStyle that has a unique list of variables. Then, when resolving variables, you walk up the RenderStyle tree till you find the first StyleVariableData in the HashMap and use that. It trades off CPU for memory and all the variable logic out of RenderStyle, which is architecturally more correct. Taking this a step further, you could isntead have a document-level/global HashMap of Pair<RenderStyle, String>, where String is variable name. Then, when walking up the RenderStyle* tree you walk up until you find a an entry with that variable name. Then you don't need StyleVariableData at all. I'm not sure how this would perform in practice, or how much memory would be saved as I could imagine the HashMap getting quite large. But I imagine the vast majority of sites won't be using more than a few dozen variables. (In reply to comment #67) > An alternative to putting these on RenderStyle or RareInheritedData, would be to have a document-level (global?) HashMap from RenderStyle to StyleVariableData for each RenderStyle that has a unique list of variables. Then, when resolving variables, you walk up the RenderStyle tree till you find the first StyleVariableData in the HashMap and use that. It trades off CPU for memory and all the variable logic out of RenderStyle, which is architecturally more correct. > > Taking this a step further, you could isntead have a document-level/global HashMap of Pair<RenderStyle, String>, where String is variable name. Then, when walking up the RenderStyle* tree you walk up until you find a an entry with that variable name. Then you don't need StyleVariableData at all. > > I'm not sure how this would perform in practice, or how much memory would be saved as I could imagine the HashMap getting quite large. But I imagine the vast majority of sites won't be using more than a few dozen variables. I'm just brainstorming here. I'm not convinced either of these is a great solution. Antti, do either of these sound like plausible solutions to you? Created attachment 145176 [details]
Rebase
I am of the opinion that keeping this data on RenderStyle is the right thing to do. 1) The already-resolved variables need to be persistent, so that when appendAuthorStylesheets is called we can incrementally load those style sheets without having to reload all stylesheets. This means that you can't really discard variables after they have been resolved, because you never know when a new stylesheet might be injected onto the page. 2) The model of CSS Variables that is in the spec is in essence one of user-defined properties that are inherited. Storing these user-defined properties on RenderStyle keeps the behavior of these user-defined properties consistent with pre-defined CSS properties, and gives us the same cascade & inheritance behavior as other inherited properties. While technically possible, treating user-defined properties differently to pre-defined properties could make the code more difficult to understand by introducing two parallel mechanisms that are designed to achieve the same purpose. 3) The global (or document level) HashMap idea has the algorithmic downside of having variable resolution cost that scales with the depth of the DOM, instead of being O(1) as the current implementation is. It may save memory though, depending on how many variables are defined, and where in the DOM they are used. If. I would suggest that this could be considered an optimization to make in the future if we found that memory usage was a concern in practice. (In reply to comment #70) > 3) The global (or document level) HashMap idea has the algorithmic downside of having variable resolution cost that scales with the depth of the DOM, instead of being O(1) as the current implementation is. It may save memory though, depending on how many variables are defined, and where in the DOM they are used. If. I would suggest that this could be considered an optimization to make in the future if we found that memory usage was a concern in practice. I should also mention that my expected use case is to define variables sparingly (eg. for things like theme colors), and then use them in many places. This means I expect variable dereference to occur with much greater frequency than variable definition on real websites. (In reply to comment #70) > 1) The already-resolved variables need to be persistent, so that when appendAuthorStylesheets is called we can incrementally load those style sheets without having to reload all stylesheets. This means that you can't really discard variables after they have been resolved, because you never know when a new stylesheet might be injected onto the page. Not sure I understand this code well enough to understand this, but we can get the majority of the memory savings in the HashMap approach without throwing away the variables, so I'm not too set on throwing away the variable tree. > 2) The model of CSS Variables that is in the spec is in essence one of user-defined properties that are inherited. Storing these user-defined properties on RenderStyle keeps the behavior of these user-defined properties consistent with pre-defined CSS properties, and gives us the same cascade & inheritance behavior as other inherited properties. While technically possible, treating user-defined properties differently to pre-defined properties could make the code more difficult to understand by introducing two parallel mechanisms that are designed to achieve the same purpose. Hard to say. That's not my intuition, but this could be argued either way. > 3) The global (or document level) HashMap idea has the algorithmic downside of having variable resolution cost that scales with the depth of the DOM, instead of being O(1) as the current implementation is. It may save memory though, depending on how many variables are defined, and where in the DOM they are used. If. I would suggest that this could be considered an optimization to make in the future if we found that memory usage was a concern in practice. This is a memory versus performance tradeoff. My intuition is to do the inverse; err on the side of saving memory and do the performance optimization if it turns out to be a problem in practice. We only need to do this when we're resolving variable values, right? Not when we're accessing css property values that were set using variable values, e.g. height: var(myColor); We'd only need to walk up the RenderStyle tree when setting the height in the RenderStyle instance, right? (In reply to comment #72) > (In reply to comment #70) > > 1) The already-resolved variables need to be persistent, so that when appendAuthorStylesheets is called we can incrementally load those style sheets without having to reload all stylesheets. This means that you can't really discard variables after they have been resolved, because you never know when a new stylesheet might be injected onto the page. > > Not sure I understand this code well enough to understand this, but we can get the majority of the memory savings in the HashMap approach without throwing away the variables, so I'm not too set on throwing away the variable tree. Yes. This point is an explanation of why variables need to be persistent, however it does not dictate an underlying storage mechanism. > > 3) The global (or document level) HashMap idea has the algorithmic downside of having variable resolution cost that scales with the depth of the DOM, instead of being O(1) as the current implementation is. It may save memory though, depending on how many variables are defined, and where in the DOM they are used. If. I would suggest that this could be considered an optimization to make in the future if we found that memory usage was a concern in practice. > > This is a memory versus performance tradeoff. My intuition is to do the inverse; err on the side of saving memory and do the performance optimization if it turns out to be a problem in practice. You can't actually know there is a significant memory saving (other than a pointer in RareStyleInheritedData) until you see how variables are used in practice, because it depends on how much sharing can be achieved by copy-on-write in the current approach for real web pages. > We only need to do this when we're resolving variable values, right? Not when we're accessing css property values that were set using variable values, e.g. > > height: var(myColor); > > We'd only need to walk up the RenderStyle tree when setting the height in the RenderStyle instance, right? With the global HashMap approach you proposed, on dereference you'd need to start at the child, and walk up the RenderStyle tree to the root, so for our expected use case where most variables are set on (or near) :root you'll be paying a lot of HashMap lookups for each variable dereference. If variable reference is orders of magnitude more common than variable definition, that's a big downside. (In reply to comment #73) > (In reply to comment #72) > > (In reply to comment #70) > > > 3) The global (or document level) HashMap idea has the algorithmic downside of having variable resolution cost that scales with the depth of the DOM, instead of being O(1) as the current implementation is. It may save memory though, depending on how many variables are defined, and where in the DOM they are used. If. I would suggest that this could be considered an optimization to make in the future if we found that memory usage was a concern in practice. > > > > This is a memory versus performance tradeoff. My intuition is to do the inverse; err on the side of saving memory and do the performance optimization if it turns out to be a problem in practice. > > You can't actually know there is a significant memory saving (other than a pointer in RareStyleInheritedData) until you see how variables are used in practice, because it depends on how much sharing can be achieved by copy-on-write in the current approach for real web pages. The memory concern is about the extra pointer on RareStyleInheritedData. I don't have an intuition for how many RareStyleInheritedDatas we have on popular pages, but I imagine it's not as rare as we'd like. This would be an easy thing to measure by instrumenting a local build of WebKit and loading popular sites. > > We only need to do this when we're resolving variable values, right? Not when we're accessing css property values that were set using variable values, e.g. > > > > height: var(myColor); > > > > We'd only need to walk up the RenderStyle tree when setting the height in the RenderStyle instance, right? > > With the global HashMap approach you proposed, on dereference you'd need to start at the child, and walk up the RenderStyle tree to the root, so for our expected use case where most variables are set on (or near) :root you'll be paying a lot of HashMap lookups for each variable dereference. If variable reference is orders of magnitude more common than variable definition, that's a big downside. It's hard to say what the performance will be like on real-world usage. Also, it's not the depth of the render tree, it's the depth of the RenderStyle tree, which I don't expect to get too deep in practice (although I don't have data to back that assertion). That's another thing that would be easy to get data for by instrumenting a local build. (In reply to comment #74) > The memory concern is about the extra pointer on RareStyleInheritedData. I don't have an intuition for how many RareStyleInheritedDatas we have on popular pages, but I imagine it's not as rare as we'd like. This would be an easy thing to measure by instrumenting a local build of WebKit and loading popular sites. Instrumented StyleRareInheritedData and visited a few "popular" (I can't back that up, it's just the sites I picked off the top of my head). wsj.com: allocated 53 max 62 slashdot.org: allocated 31 max 33 news.google.com: allocated 58 max 116 store.apple.com/us: allocated 35 max 49 These should at least be indicative of the kind of order-of-magnitude of allocations you might expect for rare inherited data. The "allocated" column is number of allocations remaining after the page has loaded, and the "max" column is the maximum number of objects simultaneously allocated during the page load. As you can see, if you have the ENABLE(CSS_VARIABLES) flag on, the worst-case-scenario was allocating memory for 116 pointers in the pages above (ie. variables flag enabled but not used on the page). Of course we with ENABLE(CSS_VARIABLES) off as I expect it to be until more optimizations in, it's not going to cost anything at all. > It's hard to say what the performance will be like on real-world usage. Also, it's not the depth of the render tree, it's the depth of the RenderStyle tree, which I don't expect to get too deep in practice (although I don't have data to back that assertion). That's another thing that would be easy to get data for by instrumenting a local build. wsj.com: max renderstyle tree depth 16 slashdot.org: max renderstyle tree depth 14 news.google.com: max renderstyle tree depth 32 store.apple.com/us: max renderstyle tree depth 13 So in the worse-case scenario for news.google.com, the global hashmap would require 32 lookups to resolve a single variable on a leaf RenderStyle. That seems like too much overhead, especially when you consider that dereference is likely to be much more common than definition. Comment on attachment 145176 [details] Rebase View in context: https://bugs.webkit.org/attachment.cgi?id=145176&action=review > Source/WebCore/css/CSSValueList.cpp:147 > +#if ENABLE(CSS_VARIABLES) > +String CSSValueList::customCssText(const HashMap<AtomicString, String>& variables) const > +{ > + StringBuilder result; > + String separator; > + switch (m_valueListSeparator) { > + case SpaceSeparator: > + separator = " "; > + break; These should have more descriptive name, something like CSSValue::serializeResolvingVariables(). In general I still don't like all this round-tripping to strings and back. Why can't we just make a copy that replaces the found variable references with parsed values? Created attachment 146161 [details]
Rename cssText().
(In reply to comment #76) > These should have more descriptive name, something like CSSValue::serializeResolvingVariables(). Done, apologies I know that was requested earlier. > In general I still don't like all this round-tripping to strings and back. Why can't we just make a copy that replaces the found variable references with parsed values? It would be nice to do that, but it is difficult for a few reasons: 1) The logic for validating expressions is deep inside the CSSParser / CSSGrammar.y, so it is difficult to determine that a rule will be valid without re-parsing, other than to completely duplicate that logic. 2) It may not even be possible to parse a variable correctly until you know the context in which it is used - for example how do you know whether to parse something as a font name or an ident unless you know the context in which it occurs? 3) Shorthand expansion happens inside the parser - eg. border: var(x); will expand to a different set of property/value pairs to apply depending on the value of x (which could be a list value). We might get to the point where you can validate a variable without re-parsing eventually, but it would take a lot of work to get there, and I don't think we want to make the kind of changes to the rest of the CSS engine that would be required to do it at this stage in the life of CSS variables. (In reply to comment #75) > (In reply to comment #74) > > The memory concern is about the extra pointer on RareStyleInheritedData. I don't have an intuition for how many RareStyleInheritedDatas we have on popular pages, but I imagine it's not as rare as we'd like. This would be an easy thing to measure by instrumenting a local build of WebKit and loading popular sites. > > Instrumented StyleRareInheritedData and visited a few "popular" (I can't back that up, it's just the sites I picked off the top of my head). > > wsj.com: allocated 53 max 62 > slashdot.org: allocated 31 max 33 > news.google.com: allocated 58 max 116 > store.apple.com/us: allocated 35 max 49 FWIW, I tried a few sites myself. I was surprised to find that even at gmail.com and plus.google.com there were <300. On the other hand, Facebook's news feed had an ever growing number the more content you show. I got in the single-digit thousands. I'm OK writing that off as an area where we need to improve RenderStyle sharing though. > > These should at least be indicative of the kind of order-of-magnitude of allocations you might expect for rare inherited data. The "allocated" column is number of allocations remaining after the page has loaded, and the "max" column is the maximum number of objects simultaneously allocated during the page load. As you can see, if you have the ENABLE(CSS_VARIABLES) flag on, the worst-case-scenario was allocating memory for 116 pointers in the pages above (ie. variables flag enabled but not used on the page). Of course we with ENABLE(CSS_VARIABLES) off as I expect it to be until more optimizations in, it's not going to cost anything at all. > > > It's hard to say what the performance will be like on real-world usage. Also, it's not the depth of the render tree, it's the depth of the RenderStyle tree, which I don't expect to get too deep in practice (although I don't have data to back that assertion). That's another thing that would be easy to get data for by instrumenting a local build. > > wsj.com: max renderstyle tree depth 16 > slashdot.org: max renderstyle tree depth 14 > news.google.com: max renderstyle tree depth 32 > store.apple.com/us: max renderstyle tree depth 13 > > So in the worse-case scenario for news.google.com, the global hashmap would require 32 lookups to resolve a single variable on a leaf RenderStyle. That seems like too much overhead, especially when you consider that dereference is likely to be much more common than definition. That's the data I was looking for. Thanks for doing the legwork. With all the above data, I'm fine with adding the member variable to StyleRareInheritedData as you have it in your current patch. Comment on attachment 146161 [details] Rename cssText(). View in context: https://bugs.webkit.org/attachment.cgi?id=146161&action=review I think this is in a good place for a first iteration. Antti, it sounds like you don't have strong objections at this point. If you disagree, feel free to R- my R+. (In reply to comment #78) > (In reply to comment #76) > > In general I still don't like all this round-tripping to strings and back. Why can't we just make a copy that replaces the found variable references with parsed values? > > It would be nice to do that, but it is difficult for a few reasons: > 1) The logic for validating expressions is deep inside the CSSParser / CSSGrammar.y, so it is difficult to determine that a rule will be valid without re-parsing, other than to completely duplicate that logic. > 2) It may not even be possible to parse a variable correctly until you know the context in which it is used - for example how do you know whether to parse something as a font name or an ident unless you know the context in which it occurs? > 3) Shorthand expansion happens inside the parser - eg. border: var(x); will expand to a different set of property/value pairs to apply depending on the value of x (which could be a list value). > > We might get to the point where you can validate a variable without re-parsing eventually, but it would take a lot of work to get there, and I don't think we want to make the kind of changes to the rest of the CSS engine that would be required to do it at this stage in the life of CSS variables. I think this is fine for now. Can you add FIXMEs to all these round-tripping cases to get to this point though? > Source/WebCore/ChangeLog:10 > + As an overview, Nit: No need for this line. This bit of the ChangeLog description is always meant to be an overview. :) > Source/WebCore/ChangeLog:12 > + That HashMap is copy-on-write, and unless variables is simply a pointer to the parent's definitions. There's some grammar problem here. I'm not sure if you have extra words or are missing a clause after "unless...definitions". > Source/WebCore/css/CSSParser.cpp:1187 > +#if ENABLE(CSS_VARIABLES) > +static inline void filterProperties(bool important, const CSSParser::ParsedPropertyVector& input, Vector<CSSProperty, 256>& output, size_t& unusedEntries, BitArray<numCSSProperties>& seenProperties, HashSet<AtomicString>& seenVariables) > +#else > static inline void filterProperties(bool important, const CSSParser::ParsedPropertyVector& input, Vector<CSSProperty, 256>& output, size_t& unusedEntries, BitArray<numCSSProperties>& seenProperties) > +#endif This is fine. I'd also be OK with always having this take a seenVariables argument and just passing in 0 if CSS_VARIABLES is not enabled. Comment on attachment 146161 [details] Rename cssText(). View in context: https://bugs.webkit.org/attachment.cgi?id=146161&action=review >> Source/WebCore/ChangeLog:10 >> + As an overview, > > Nit: No need for this line. This bit of the ChangeLog description is always meant to be an overview. :) Done. >> Source/WebCore/ChangeLog:12 >> + That HashMap is copy-on-write, and unless variables is simply a pointer to the parent's definitions. > > There's some grammar problem here. I'm not sure if you have extra words or are missing a clause after "unless...definitions". Right you are, it was missing some words. Fixed. >> Source/WebCore/css/CSSParser.cpp:1187 >> +#endif > > This is fine. I'd also be OK with always having this take a seenVariables argument and just passing in 0 if CSS_VARIABLES is not enabled. Leaving as is because I want to go by reference instead of using a pointer (this function should be inlined). Maybe the compiler could optimize out the pointer anyway, but since I don't know that for sure I'll just make it easy for the compiler to get it right. > Source/WebCore/css/StyleResolver.cpp:3200 > + if (!CSSParser::parseValue(resultSet.get(), id, expression.second, false, CSSStrictMode, 0)) Adding the requested FIXME here as it is the only place we pass strings back into the parser, so anyone tracing through this behavior should be able to find it here. Created attachment 147181 [details]
Patch for landing
Comment on attachment 147181 [details] Patch for landing Clearing flags on attachment: 147181 Committed r120154: <http://trac.webkit.org/changeset/120154> All reviewed patches have been landed. Closing bug. *** Bug 46594 has been marked as a duplicate of this bug. *** |