[webkit-reviews] review granted: [Bug 229069] Addition of CSSUnparsedValue. (TypedOM) : [Attachment 436750] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Qiaosong Zhou
<qiaosong_zhou at apple.com>'s request for review:
Bug 229069: Addition of CSSUnparsedValue. (TypedOM)
https://bugs.webkit.org/show_bug.cgi?id=229069

Attachment 436750: Patch

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




--- Comment #21 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 436750
  --> https://bugs.webkit.org/attachment.cgi?id=436750
Patch

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

Great!	I think once these final few comments are addressed it'll be ready to
land.

> Source/WebCore/css/typedom/CSSOMVariableReferenceValue.h:48
> +    CSSUnparsedValue* fallback() const { return m_fallback.get(); }

This should probably be one of these instead:
const CSSUnparsedValue* fallback() const;
CSSUnparsedValue* fallback();

> Source/WebCore/css/typedom/CSSStyleValue.cpp:67
> +    bool important = true;

constexpr

> Source/WebCore/css/typedom/CSSStyleValue.cpp:84
> +	       auto reifiedValue = CSSStyleValue::reifyValue(propertyID,
currentValue.copyRef());
> +	       if (reifiedValue)

Nit: these can go on the same line:
if (auto reifiedValue = CSSStyleValue::reifyValue(propertyID,
currentValue.copyRef()))

> Source/WebCore/css/typedom/CSSStyleValue.cpp:90
> +    } else {
> +	   auto reifiedValue = CSSStyleValue::reifyValue(propertyID,
cssValue.copyRef());
> +	   if (reifiedValue)

else if (auto reifiedValue = CSSStyleValue::reifyValue(propertyID,
cssValue.copyRef()))

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:51
> +CSSUnparsedValue::CSSUnparsedValue(const Vector<CSSUnparsedSegment>&
segments)

Can this be a Vector<CSSUnparsedSegment>&& instead?  That would prevent a
Vector copy if it were.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:52
> +    : CSSStyleValue(nullptr)

This line is probably unnecessary since CSSStyleValue has a default constructor
that does exactly this.

> Source/WebCore/css/typedom/CSSUnparsedValue.cpp:65
> +void CSSUnparsedValue::serialize(StringBuilder& builder) const

I don't think this needs to be a separate function unless you're planning to
use it elsewhere in the future.

> Source/WebCore/css/typedom/CSSUnparsedValue.h:32
> +#include <wtf/Variant.h>

This is probably not needed

> Source/WebCore/css/typedom/CSSUnparsedValue.h:33
> +#include <wtf/text/StringBuilder.h>

This is not needed.  <wtf/Forward.h> should have enough.

> Source/WebCore/css/typedom/CSSUnparsedValue.h:52
> +    ExceptionOr<CSSUnparsedSegment> setItem(size_t, CSSUnparsedSegment);

Can this be a CSSUnparsedSegment&& or a const CSSUnparsedSegment&?  As it
stands now, you're probably unnecessarily instantiating a whole object when
calling this function.

> Source/WebCore/css/typedom/StylePropertyMapReadOnly.cpp:58
>      // FIXME: should use raw CSSStyleValue

I think this comment can be removed since we're using CSSStyleValue now.