Bug 139601

Summary: [CSS Shapes] Fix StyleBuilder code to use CSSValueNone to match spec and other code
Product: WebKit Reporter: Bem Jones-Bey <bjonesbe>
Component: CSSAssignee: Bem Jones-Bey <bjonesbe>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, WebkitBugTracker, zoltan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch cdumez: review+, cdumez: commit-queue-

Description Bem Jones-Bey 2014-12-12 15:54:03 PST
This one part of the code didn't get updated when the rest was to use none instead of auto.
Comment 1 Bem Jones-Bey 2014-12-12 16:01:47 PST
Created attachment 243224 [details]
Patch
Comment 2 Chris Dumez 2014-12-12 16:15:01 PST
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.
Comment 3 Bem Jones-Bey 2014-12-12 17:29:14 PST
Created attachment 243236 [details]
Patch

Update for comments
Comment 4 Chris Dumez 2014-12-12 17:59:15 PST
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.
Comment 5 Bem Jones-Bey 2014-12-15 10:57:14 PST
Committed r177289: <http://trac.webkit.org/changeset/177289>