Bug 130397

Summary: Add CSS Custom Property Support (in preparation for CSS variables)
Product: WebKit Reporter: Dirk Schulze <krit>
Component: CSSAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, hyatt, jonlee, kling, koivisto, webkit-bug-importer, zoltan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://dev.w3.org/csswg/css-variables/#defining-variables
Attachments:
Description Flags
Patch from old custom property code w/o var()
none
Patch r? for feedback
none
Patch just to get EWS feedback
none
Patch just to get EWS feedback
none
Patch for review
none
Patch
none
Patch koivisto: review+

Description Dirk Schulze 2014-03-18 09:02:25 PDT
Created attachment 227055 [details]
Patch from old custom property code w/o var()

I looked at the old custom properties implementation that was removed a couple of weeks ago. I reduced the old implementation and removed support for the var() function.

There are certain things to notice:
* There is no real prefix check. From looking at the code, every unknown property might be a custom property. We check just the length in CSSParser.
* We use a Map in RenderStyle to store all property names and the values. We must store Strings of the passed value. This is unfortunate since abstractions can be much more memory efficient. We use Vectors for transform operations and filter functions. So I am not sure if Maps themselves are a problem though.
* All custom properties are mapped to the CSSPropertyID CSSPropertyCustom. This has effects on getting element style or computed style. It is not possible to map back from CSSPropertyCustom to the requested custom property. ComputedStyleExtractor::propertyValue(CSSPropertyID propertyID...)
Comment 1 Zoltan Horvath 2014-07-23 14:43:43 PDT
Created attachment 235381 [details]
Patch r? for feedback
Comment 2 Dave Hyatt 2015-09-23 16:22:40 PDT
Created attachment 261846 [details]
Patch just to get EWS feedback
Comment 3 Dave Hyatt 2015-09-23 16:27:25 PDT
Created attachment 261848 [details]
Patch just to get EWS feedback
Comment 4 WebKit Commit Bot 2015-09-23 16:29:58 PDT
Attachment 261848 [details] did not pass style-queue:


ERROR: Source/WebCore/css/StyleResolver.cpp:2671:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/style/StyleCustomPropertyData.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/style/StyleCustomPropertyData.h:48:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:11968:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:11969:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Dave Hyatt 2015-09-24 09:52:01 PDT
Created attachment 261870 [details]
Patch for review
Comment 6 WebKit Commit Bot 2015-09-24 09:53:29 PDT
Attachment 261870 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/style/StyleCustomPropertyData.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/style/StyleCustomPropertyData.h:49:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Dave Hyatt 2015-09-24 09:57:51 PDT
Created attachment 261871 [details]
Patch
Comment 8 WebKit Commit Bot 2015-09-24 10:00:50 PDT
Attachment 261871 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/style/StyleCustomPropertyData.h:48:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/style/StyleCustomPropertyData.h:51:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/rendering/style/StyleCustomPropertyData.h:52:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Dave Hyatt 2015-09-24 10:13:35 PDT
Created attachment 261872 [details]
Patch
Comment 10 Antti Koivisto 2015-09-24 10:42:49 PDT
Comment on attachment 261872 [details]
Patch

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

Looks good r=me

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3598
> +    const HashMap<AtomicString, String>* customProperties = style->customProperties();

Could be auto*

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3620
> +    const HashMap<AtomicString, String>* customProperties = style->customProperties();

auto*

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3709
> +    Node* styledNode = this->styledNode();
> +    if (styledNode) {

if (auto* styledNode = this->styledNode())

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3711
> +        RefPtr<RenderStyle> style = computeRenderStyleForProperty(styledNode, m_pseudoElementSpecifier, CSSPropertyCustom);
> +        if (style) {

similarly

> Source/WebCore/css/StyleProperties.cpp:1260
> +    // Convert the propertyID into an uint16_t to compare it with the metadata's m_propertyID to avoid
> +    // the compiler converting it to an int multiple times in the loop.
> +    uint16_t id = static_cast<uint16_t>(CSSPropertyCustom);

Probably unnecessary. Compiler will likely figure this out.

> Source/WebCore/css/StyleProperties.cpp:1278
> +    // Convert the propertyID into an uint16_t to compare it with the metadata's m_propertyID to avoid
> +    // the compiler converting it to an int multiple times in the loop.
> +    uint16_t id = static_cast<uint16_t>(CSSPropertyCustom);

Probably unnecessary. Compiler will likely figure this out.

> Source/WebCore/css/StyleProperties.h:95
> +    PassRefPtr<CSSValue> getCustomPropertyCSSValue(const String& propertyName) const;

Just use RefPtr instead. PassRefPtr is obsolete since we got C++11 move semantics.

> Source/WebCore/css/StyleResolver.cpp:854
> +    // Resolve variables first.

"Resolve custom properties first" (though the code is clear and the comment might not be needed in the first place.

> Source/WebCore/css/StyleResolver.cpp:1022
> +    // Resolve variables first.

and here

> Source/WebCore/css/StyleResolver.cpp:1701
> +        // Resolve variables first.

etc

> Source/WebCore/css/StyleResolver.cpp:2559
> +            memset(property.cssValue, 0, sizeof(property.cssValue));

I think you can just say property.cssValue = {} these days.

> Source/WebCore/rendering/style/StyleRareInheritedData.h:82
> +    DataRef<StyleCustomPropertyData> m_customProperties;

I think it is slightly wrong that these are in RenderStyle. RenderStyle is supposed to be the input data structure for rendering yet custom properties by definition are something rendering doesn't know about. It would be nice to generalize StyleResolver so that it could compute things other than RenderStyles. 

Still, this is clearly the right thing to do to get this going.
Comment 11 Dave Hyatt 2015-09-24 11:16:23 PDT
Landed in r190209.
Comment 12 Brent Fulgham 2015-09-29 10:54:14 PDT
This change *may* have introduced Windows regressions (see https://bugs.webkit.org/show_bug.cgi?id=149632).
Comment 13 Radar WebKit Bug Importer 2015-12-04 18:19:42 PST
<rdar://problem/23771256>