[webkit-reviews] review denied: [Bug 190038] Registered custom properties should allow inheritance to be controlled : [Attachment 351124] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 190038: Registered custom properties should allow inheritance to be
controlled
https://bugs.webkit.org/show_bug.cgi?id=190038

Attachment 351124: Patch

https://bugs.webkit.org/attachment.cgi?id=351124&action=review




--- Comment #22 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 351124
  --> https://bugs.webkit.org/attachment.cgi?id=351124
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2628
> +    auto* value = CustomPropertyValueMapCascade({
&style->inheritedCustomProperties(), &style->nonInheritedCustomProperties()
}).get(propertyName);

That's a pretty indirect way of doing two hash lookups.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4166
> -    auto results = copyToVector(customProperties.keys());
> -    return results.at(index);
> +    const auto& inheritedCustomProperties =
style->inheritedCustomProperties();
> +
> +    if (i < numComputedProperties + inheritedCustomProperties.size()) {
> +	   auto results = copyToVector(inheritedCustomProperties.keys());
> +	   return results.at(i - numComputedProperties);
> +    }
> +
> +    const auto& nonInheritedCustomProperties =
style->nonInheritedCustomProperties();
> +    auto results = copyToVector(nonInheritedCustomProperties.keys());
> +    return results.at(i - inheritedCustomProperties.size() -
numComputedProperties);

This code is weird. We are indexing into hash maps that have completely random
order. There is also unnecessary copying into a vector. I know this patch
didn't add these things but still...

> Source/WebCore/css/CSSCustomPropertyValue.h:81
> +    Length* resolvedTypedValue() { return m_resolvedTypedValue.get(); }
> +    const Length* resolvedTypedValue() const { return
m_resolvedTypedValue.get(); }

Returning std::optional<Length> would be more modern.

> Source/WebCore/css/CSSCustomPropertyValue.h:120
> +    // FIXME: It should not be possible to express an invalid state, such as
containsVariables() && resolvedTypedValue().
> +    std::unique_ptr<Length> m_resolvedTypedValue { nullptr };

I think you want std::optional. Also explicit initialization is unnecessary.

> Source/WebCore/css/CSSValue.cpp:499
> +CSSCustomPropertyValue* CustomPropertyValueMapCascade::get(const
AtomicString& name) const
> +{
> +    for (auto* map : m_map) {
> +	   if (!map)
> +	       continue;
> +	   if (auto* val = map->get(name))
> +	       return val;
> +    }
> +    return nullptr;
> +}

Maybe this should just be a helper on RenderStyle? (see below)

> Source/WebCore/css/CSSValue.h:292
> +// Wrapper class to merge a number of CustomPropertyValueMaps into one,
picking the first map
> +// that contains a particular key.
> +class CustomPropertyValueMapCascade {
> +public:
> +    CustomPropertyValueMapCascade(const Vector<const
CustomPropertyValueMap*>& map)
> +	   : m_map { map }
> +    {
> +    }
> +
> +    CSSCustomPropertyValue* get(const AtomicString& name) const;
> +
> +private:
> +    const Vector<const CustomPropertyValueMap*> m_map;
> +};

What guarantees the CustomPropertyValueMaps stay alive during lifetime of this
object?

> Source/WebCore/css/StyleBuilderCustom.h:1883
> +    auto* initialVal = registered ? registered->initialValue.get() :
nullptr;
> +    if (initialVal) {

initialVal local doesn't add anything and could be removed. Also WebKit style
is to use full words ('initialValue').

> Source/WebCore/css/StyleBuilderCustom.h:1903
> +inline void StyleBuilderCustom::applyValueCustomProperty(StyleResolver&
styleResolver, const CSSRegisteredCustomProperty* registered, CSSValue& value)
> +{
> +    auto* customPropertyValue = &downcast<CSSCustomPropertyValue>(value);

How do we know this cast is safe? The function should probably be taking
CSSCustomPropertyValue and leaving casting to the clients.

> Source/WebCore/css/StyleResolver.cpp:1710
> +    CustomPropertyValueMapCascade customProperties({
&m_state.style()->inheritedCustomProperties(),
&m_state.style()->nonInheritedCustomProperties() });
> +
> +    return parser.parseValueWithVariableReferences(propID, value,
customProperties, document().getCSSRegisteredCustomPropertySet(),
*m_state.style());

You now pass both CustomPropertyValueMapCascade and style almost everywhere.
This makes the whole CustomPropertyValueMapCascade type sort of pointless (and
it doesn't do much in the first place) as all the same information is available
from RenderStyle. How about just getting rid of CustomPropertyValueMapCascade
and simply adding a property name getter to RenderStyle that resolves the value
from right maps?

> Source/WebCore/css/makeprop.pl:1041
> +	   if (isInitial)
> +	       StyleBuilderCustom::applyInitialCustomProperty(styleResolver,
registered, downcast<CSSCustomPropertyValue>(value).name());
> +	   else if (isInherit)
> +	       StyleBuilderCustom::applyInheritCustomProperty(styleResolver,
registered, downcast<CSSCustomPropertyValue>(value).name());
> +	   else
> +	       StyleBuilderCustom::applyValueCustomProperty(styleResolver,
registered, value);

Why does't call to applyValueCustomProperty cast the value to
CSSCustomPropertyValue here like the other functions do?

> Source/WebCore/css/parser/CSSParser.cpp:216
> +	   if (value.isVariableReferenceValue()) {
> +	       const CSSVariableReferenceValue& valueWithReferences =
downcast<CSSVariableReferenceValue>(value);

is<CSSVariableReferenceValue>(value) pairs better with downcast<>

> Source/WebCore/css/parser/CSSPropertyParser.cpp:3953
> +    case CSSPropertyCustom: // FIXME this should be based on syntax.

I don't understand the comment.

> Source/WebCore/rendering/style/RenderStyle.cpp:2249
> -    if (!m_rareInheritedData->customProperties->containsVariables)
> -	   return;
> +    auto& inheritedPropertyData =
m_rareInheritedData.access().customProperties.access();
> +    auto& nonInheritedPropertyData =
m_rareNonInheritedData.access().customProperties.access();

This now copies both rareInheritedData and rareNonInheritedData
unconditionally. You should check what actually needs to be mutated before
triggering COW.