Summary: | [CSS Shapes] Fix StyleBuilder code to use CSSValueNone to match spec and other code | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Bem Jones-Bey <bjonesbe> | ||||||
Component: | CSS | Assignee: | 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
Bem Jones-Bey
2014-12-12 15:54:03 PST
Created attachment 243224 [details]
Patch
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. Created attachment 243236 [details]
Patch
Update for comments
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. Committed r177289: <http://trac.webkit.org/changeset/177289> |