[webkit-reviews] review denied: [Bug 190303] Properly determine if css custom property values are computationally independent : [Attachment 351657] 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 190303: Properly determine if css custom property values are
computationally independent
https://bugs.webkit.org/show_bug.cgi?id=190303

Attachment 351657: Patch

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




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

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

Looks generally fine, but one more round would be good.

> Source/WebCore/css/CSSCalculationValue.cpp:282
> +    void getDirectComputationalDependencies(HashSet<CSSPropertyID>& values)
const final
> +    {
> +	   auto dependencies = m_value->getDirectComputationalDependencies();
> +	   values.add(dependencies.begin(), dependencies.end());
> +    }
> +
> +    void getDirectRootComputationalDependencies(HashSet<CSSPropertyID>&
values) const final
> +    {
> +	   auto dependencies =
m_value->getDirectRootComputationalDependencies();
> +	   values.add(dependencies.begin(), dependencies.end());
> +    }

Maybe all versions of these functions should take the HashSet as an argument? 
It would avoid moving values from one hash to another.

> Source/WebCore/css/CSSCalculationValue.h:73
> +    virtual void getDirectComputationalDependencies(HashSet<CSSPropertyID>&)
const = 0;
> +    virtual void
getDirectRootComputationalDependencies(HashSet<CSSPropertyID>&) const = 0;

collect* instead of get* would be better names for these.

> Source/WebCore/css/CSSCalculationValue.h:108
> +    HashSet<CSSPropertyID> getDirectComputationalDependencies() const;
> +    HashSet<CSSPropertyID> getDirectRootComputationalDependencies() const;

Here too. It signals they are not cheap accessors.

> Source/WebCore/css/CSSCustomPropertyValue.cpp:51
> +Vector<CSSParserToken>& CSSCustomPropertyValue::tokens(const
CSSRegisteredCustomPropertySet& registeredProperties, const RenderStyle& style)
const
> +{
> +    m_tokensReturnValue.clear();

It appears m_tokensReturnValue is only needed so you can return a reference? It
is not clear to me how that is better than just returning the vector as a value
and it is certainly more bug prone. Move semantics should keep it cheap. An
alternative is the pass the vector in as a reference parameter (and call this
something like collectTokens).

> Source/WebCore/css/CSSPrimitiveValue.cpp:1231
> +	   return HashSet<CSSPropertyID>({CSSPropertyFontSize});

I think you can just say

return { CSSPropertyFontSize };

> Source/WebCore/css/CSSPrimitiveValue.cpp:1235
> +    return HashSet<CSSPropertyID>();

return { };

> Source/WebCore/css/CSSPrimitiveValue.cpp:1246
> +    return HashSet<CSSPropertyID>();

return { };

> Source/WebCore/css/CSSValue.cpp:124
> +    return HashSet<CSSPropertyID>();

return { };

> Source/WebCore/css/CSSValue.cpp:131
> +    return HashSet<CSSPropertyID>();

return { };