Bug 139601 - [CSS Shapes] Fix StyleBuilder code to use CSSValueNone to match spec and other code
Summary: [CSS Shapes] Fix StyleBuilder code to use CSSValueNone to match spec and othe...
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Bem Jones-Bey
Depends on:
Reported: 2014-12-12 15:54 PST by Bem Jones-Bey
Modified: 2014-12-15 10:57 PST (History)
3 users (show)

See Also:

Patch (3.56 KB, patch)
2014-12-12 16:01 PST, Bem Jones-Bey
no flags Details | Formatted Diff | Diff
Patch (8.14 KB, patch)
2014-12-12 17:29 PST, Bem Jones-Bey
cdumez: review+
cdumez: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Chris Dumez 2014-12-12 16:15:01 PST
Comment on attachment 243224 [details]

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]

Update for comments
Comment 4 Chris Dumez 2014-12-12 17:59:15 PST
Comment on attachment 243236 [details]

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


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