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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 19 12:43:44 PDT 2021

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 223252: Parse CSS contain property

Attachment 423721: Patch


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

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 && !

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
> +	   if (downcast<CSSPrimitiveValue>(value).valueID() == CSSValueStrict)
> +	       return

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());


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.

More information about the webkit-reviews mailing list