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...)
Created attachment 235381 [details] Patch r? for feedback
Created attachment 261846 [details] Patch just to get EWS feedback
Created attachment 261848 [details] Patch just to get EWS feedback
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.
Created attachment 261870 [details] Patch for review
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.
Created attachment 261871 [details] Patch
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.
Created attachment 261872 [details] Patch
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.
Landed in r190209.
This change *may* have introduced Windows regressions (see https://bugs.webkit.org/show_bug.cgi?id=149632).
<rdar://problem/23771256>