Bug 130397 – Add CSS Custom Property Support (in preparation for CSS variables)

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dirk Schulze]

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...)

[Zoltan Horvath]

Created attachment 235381 [details]
Patch r? for feedback

[Dave Hyatt]

Created attachment 261846 [details]
Patch just to get EWS feedback

[Dave Hyatt]

Created attachment 261848 [details]
Patch just to get EWS feedback

[WebKit Commit Bot]

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.

[Dave Hyatt]

Created attachment 261870 [details]
Patch for review

[WebKit Commit Bot]

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.

[Dave Hyatt]

Created attachment 261871 [details]
Patch

[WebKit Commit Bot]

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.

[Dave Hyatt]

Created attachment 261872 [details]
Patch

[Antti Koivisto]

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.

[Dave Hyatt]

Landed in r190209.

[Brent Fulgham]

This change *may* have introduced Windows regressions (see https://bugs.webkit.org/show_bug.cgi?id=149632).

[Radar WebKit Bug Importer]

<rdar://problem/23771256>