Bug 139601 – [CSS Shapes] Fix StyleBuilder code to use CSSValueNone to match spec and other code

• Component: CSS
• Status: RESOLVED FIXED

[Bem Jones-Bey]

This one part of the code didn't get updated when the rest was to use none instead of auto.

[Bem Jones-Bey]

Created attachment 243224 [details]
Patch

[Chris Dumez]

Comment on attachment 243224 [details]
Patch

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

> Source/WebCore/css/StyleBuilderCustom.h:228
>  inline void StyleBuilderCustom::applyValueWebkitShapeOutside(StyleResolver& styleResolver, CSSValue& value)

Based on the assertions, it looks like we *always* end up calling RenderStyle::setShapeOutside(). If so, I think it would be great to get rid of this custom code and use a standard:
PassRefPtr<ShapeValue> convertShapeValue(StyleResolver&, CSSValue&);
in StyleBuilderConverter.h

Then use Converter=ShapeValue for this property in CSSPropertyNames.in, instead of Custom=*.

> Source/WebCore/css/StyleBuilderCustom.h:237
> +    if (is<CSSImageValue>(value) || is<CSSImageGeneratorValue>(value) || is<CSSImageSetValue>(value)) {

Looks like the last one should be protected by #if ENABLE(CSS_IMAGE_SET) ?

> Source/WebCore/css/StyleBuilderCustom.h:243
> +    if (is<CSSValueList>(value)) {

Based on the parser code, it looks to me that it can only be a CSSValueList at this point. I would get rid of this check and rely on the assertion in downcast<CSSValueList>(value) below.

[Bem Jones-Bey]

Created attachment 243236 [details]
Patch

Update for comments

[Chris Dumez]

Comment on attachment 243236 [details]
Patch

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

r=me, very nice, thanks. Just a couple of nits before landing.

> Source/WebCore/ChangeLog:10
> +        In doing this change, it became apparent that thwere is nothing

"there"

> Source/WebCore/css/StyleBuilderConverter.h:560
> +static inline bool isImageShape(CSSValue& value)

nit: "const CSSValue&"

> Source/WebCore/css/StyleBuilderConverter.h:572
> +        return nullptr;

if (is<CSSPrimitiveValue>(value)) {
  ASSERT(downcast<CSSPrimitiveValue>(value).getValueID() == CSSValueNone);
  return nullptr;
}

Because CSSValueNone is the only CSSPrimitiveValue allowed by the parser.

> Source/WebCore/css/StyleBuilderConverter.h:587
> +            referenceBox = CSSBoxType(primitiveValue);

nit: "CSSBoxType()" is likely unnecessary.

[Bem Jones-Bey]

Committed r177289: <http://trac.webkit.org/changeset/177289>