[webkit-reviews] review granted: [Bug 190039] Registered custom properties should support syntax parameter for <length> and * : [Attachment 352508] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 190039: Registered custom properties should support syntax parameter for
<length> and *
https://bugs.webkit.org/show_bug.cgi?id=190039

Attachment 352508: Patch

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




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

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

Looks good, r=me with the issue in PropertySetCSSStyleDeclaration::setProperty
fixed. 

It would also be nice to add a test case for that.

> Source/WebCore/css/CSSCustomPropertyValue.cpp:45
> +    auto visitor = WTF::makeVisitor([&](const
Ref<CSSVariableReferenceValue>& value) {
> +	   return value.get() ==
WTF::get<Ref<CSSVariableReferenceValue>>(other.m_value).get();
> +    }, [&](const CSSValueID& value) {
> +	   return value == WTF::get<CSSValueID>(other.m_value);
> +    }, [&](const Ref<CSSVariableData>& value) {
> +	   return value.get() ==
WTF::get<Ref<CSSVariableData>>(other.m_value).get();
> +    }, [&](const Length& value) {
> +	   return value == WTF::get<Length>(other.m_value);
> +    });
> +    return WTF::visit(visitor, m_value);

Variant has operator==. Why can't you use that?

> Source/WebCore/css/CSSCustomPropertyValue.h:44
> +using CSSCustomPropertyValueVariant =
Variant<Ref<CSSVariableReferenceValue>, CSSValueID, Ref<CSSVariableData>,
Length>;
> +
>  class CSSCustomPropertyValue final : public CSSValue {

You might consider scoping the Variant type into the CSSCustomPropertyValue
namespace? CSSCustomPropertyValue::VariantValue or something.

> Source/WebCore/css/CSSStyleSheet.h:84
> -    Document* ownerDocument() const;
> +    WEBCORE_EXPORT Document* ownerDocument() const;

Does this really need to be exported?

> Source/WebCore/css/DOMCSSRegisterCustomProperty.cpp:70
> +    document.styleScope().didChangeStyleSheetEnvironment();

This triggers full re-evaluation of stylesheets.  Is this something that is
expected to be rare in practice? Are there ways to optimize this in the future?

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:233
> +    ASSERT(parentElement() || parentStyleSheet());

This is not correct. You can have completely detached CSSStyleDeclaration, for
example:

let styleDecl = element.style;
element.removeAttribute("style");
// styleDecl is now detached

(didn't try, just by reading code)

Maybe willMutate() should return false in this case, but it currently doesn't.

> Source/WebCore/css/PropertySetCSSStyleDeclaration.cpp:241
> +    ASSERT(document);

As a result document may be null too.

> Source/WebCore/css/StyleResolver.cpp:432
> +    ApplyCascadedPropertyState applyState { this, &cascade, &result, { }, {
}, { }, { } };

Please get rid of { }, { }, { }, { } repeated everywhere. I think you can just
remove them, but if not, just add a constructor.

> Source/WebCore/css/StyleResolver.cpp:2342
> +	   if (state.inProgress.contains(propertyID)) {

'inProgressProperties' and 'inProgressCustomProperties' would be more
consistent and informative names.

> Source/WebCore/css/StyleResolver.h:539
> +// State to use when applying properties, to keep track of which custom and
high-priority
> +// properties have been resolved.
> +struct ApplyCascadedPropertyState {
> +    StyleResolver* styleResolver;
> +    StyleResolver::CascadedProperties* cascade;
> +    const StyleResolver::MatchResult* matchResult;
> +    HashSet<CSSPropertyID> appliedProperties;
> +    HashSet<String> appliedCustomProperties;
> +    HashSet<CSSPropertyID> inProgress;
> +    HashSet<String> inProgressCustom;
> +};

We should split out some of this stuff from StyleResolver at some point.

> Source/WebCore/css/parser/CSSParser.cpp:234
> +    auto visitor = WTF::makeVisitor([&](const
Ref<CSSVariableReferenceValue>& value) {
> +	   valueWithReferences = value.ptr();
> +    }, [&](const CSSValueID&) {
> +	   ASSERT_NOT_REACHED();
> +    }, [&](const Ref<CSSVariableData>&) {
> +	   ASSERT_NOT_REACHED();
> +    }, [&](const Length&) {
> +	   ASSERT_NOT_REACHED();
> +    });
> +    WTF::visit(visitor, customPropValue.value());

Please just use WTF::get<> instead of a visitor.

> Source/WebCore/css/parser/CSSPropertyParser.h:54
> +    CSSPropertyParser(const CSSParserTokenRange&, const CSSParserContext&,
Vector<CSSProperty, 256>*, bool consumeWhitespace = true);

Please use enum class for consumeWhitespace, booleans in call sites are
confusing.