[webkit-reviews] review granted: [Bug 223252] Parse CSS contain property : [Attachment 423721] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 223252: Parse CSS contain property
https://bugs.webkit.org/show_bug.cgi?id=223252

Attachment 423721: Patch

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




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 423721
  --> https://bugs.webkit.org/attachment.cgi?id=423721
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3487
> +	       if (renderer && !renderer->settings().cssContainmentEnabled())
> +		   return nullptr;

It’s not right to use the renderer to get to settings. We need this to get the
correct settings when there is no renderer. I think we should use m_element.

    if (m_element && !
m_element->document().settings().cssContainmentEnabled())

I think the same mistake is repeated in many other places in this function, and
can likely be tested by using computed style APIs for non-rendered elements.

> Source/WebCore/style/StyleBuilderCustom.h:1219
> +	   if (downcast<CSSPrimitiveValue>(value).valueID() == CSSValueNone)
> +	       return
builderState.style().setContain(RenderStyle::initialContainment());
> +	   if (downcast<CSSPrimitiveValue>(value).valueID() == CSSValueStrict)
> +	       return
builderState.style().setContain(RenderStyle::strictContainment());

Why not put the valueID into a local variable?

Consider switch instead of cascading if statements.

> Source/WebCore/style/StyleBuilderCustom.h:1229
> +	   CSSPrimitiveValue& value = downcast<CSSPrimitiveValue>(item.get());

auto&

But also, why not have the local variable be the valueID instead of the
CSSPrimitiveValue object?

> Source/WebCore/style/StyleBuilderCustom.h:1230
> +	   if (value.valueID() == CSSValueSize)

Consider switch instead of cascading if statements.