[webkit-reviews] review granted: [Bug 183994] border-radius inline style serializes with invalid syntax : [Attachment 442839] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 29 10:38:06 PDT 2021


Darin Adler <darin at apple.com> has granted Joonghun Park
<jh718.park at samsung.com>'s request for review:
Bug 183994: border-radius inline style serializes with invalid syntax
https://bugs.webkit.org/show_bug.cgi?id=183994

Attachment 442839: Patch

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




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

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

Looks OK, but I don’t think there is enough testing for edge cases here. I’m
going to say review+ but I think we should refine this more before landing.

> Source/WebCore/css/StyleProperties.cpp:372
> +	   bool showBottomLeft = !(topRight == bottomLeft);
> +	   bool showBottomRight = !(topLeft == bottomRight) || showBottomLeft;
> +	   bool showTopRight = !(topLeft == topRight) || showBottomRight;

Why not use != instead of !(==)?

Why does this compare the pointers instead of comparing the values? I suggest
we compute all four strings, and compare equality of the strings. It’s OK to
optimize the case where the pointers are identical, but we can’t count on
comparing the pointers to see if the values are the same.

> Source/WebCore/css/StyleProperties.cpp:388
> +	   StringBuilder result;
> +	   result.append(topLeft->cssText());
> +	   if (showTopRight) {
> +	       result.append(' ');
> +	       result.append(topRight->cssText());
> +	   }
> +	   if (showBottomRight) {
> +	       result.append(' ');
> +	       result.append(bottomRight->cssText());
> +	   }
> +	   if (showBottomLeft) {
> +	       result.append(' ');
> +	       result.append(bottomLeft->cssText());
> +	   }
> +	   return result.toString();

This can likely be done much more efficiently with makeString.

> Source/WebCore/css/StyleProperties.cpp:398
> +    // All 4 properties must be specified.
> +    if (!topLeft || !topRight || !bottomRight || !bottomLeft)
> +	   return String();

I don’t understand this. What does "must be specified" mean here. Is this
really an assertion? Why is returning the null string OK?

> Source/WebCore/css/StyleProperties.cpp:400
> +    // FIXME: pair should be replaced with CSSValuePair as specified in
CSSPropertyParser's FIXME-NEWPARSER.

Do we really need this FIXME? How will this help us when we do make the change.

> Source/WebCore/css/StyleProperties.cpp:404
> +    const auto* topLeftPair =
downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderTopLeftRadius
)).pairValue();
> +    const auto* topRightPair =
downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderTopRightRadiu
s)).pairValue();
> +    const auto* bottomRightPair =
downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderBottomRightRa
dius)).pairValue();
> +    const auto* bottomLeftPair =
downcast<CSSPrimitiveValue>(*getPropertyCSSValue(CSSPropertyBorderBottomLeftRad
ius)).pairValue();

Should be using the *topLeft, not call getPropertyCSSValue again.

The const here isn’t important. If these are guaranteed to be non-null, how
about references instead of pointers?

> Source/WebCore/css/StyleProperties.cpp:414
> +    StringBuilder builder;
> +    builder.append(serialize(topLeftPair->first(), topRightPair->first(),
bottomRightPair->first(), bottomLeftPair->first()));
> +
> +    if (!(topLeftPair->first() == topLeftPair->second()) ||
!(topRightPair->first() == topRightPair->second()) ||
!(bottomRightPair->first() == bottomRightPair->second() ||
!(bottomLeftPair->first() == bottomLeftPair->second()))) {
> +	   builder.append(" / ");
> +	   builder.append(serialize(topLeftPair->second(),
topRightPair->second(), bottomRightPair->second(), bottomLeftPair->second()));
> +    }
> +
> +    return builder.toString();

Same comment about comparing pointers instead of strings. We should compare the
strings. Also, this shouldn’t be done with StringBuilder. We can just call
makeString twice:

    auto first = serialize(topLeftPair->first(), topRightPair->first(),
bottomRightPair->first(), bottomLeftPair->first());
    auto second = serialize(topLeftPair->second(), topRightPair->second(),
bottomRightPair->second(), bottomLeftPair->second());
    if (first == second)
	return first;
    return makeString(first, " / ", second);


More information about the webkit-reviews mailing list