[webkit-reviews] review granted: [Bug 188697] [css-logical] Flow relative margin, padding and border shorthands : [Attachment 350367] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Oriol Brufau
<obrufau at igalia.com>'s request for review:
Bug 188697: [css-logical] Flow relative margin, padding and border shorthands
https://bugs.webkit.org/show_bug.cgi?id=188697

Attachment 350367: Patch

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




--- Comment #57 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 350367
  --> https://bugs.webkit.org/attachment.cgi?id=350367
Patch

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

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:91
> +ENABLE_CSS_LOGICAL = ENABLE_EXPERIMENTAL_FEATURES;

I would prefer this be called ENABLE_CSS_LOGICAL_PROPERTIES. Otherwise, the
reader asks "enable CSS logical what?".

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:4235
> +    if (showEnd)

You could just do if (!compareCSSValuePtr(startValue, endValue)) here.

> Source/WebCore/css/StyleProperties.cpp:414
> +	       // All components are "initial" and start is not implicit.

This comment duplicates the previous line of code.

> Source/WebCore/css/StyleProperties.cpp:424
> +    if (showEnd) {

if (!start.value()->equals(*end.value()))

> Source/WebCore/css/StyleProperties.cpp:1067
> +		   if (!borderBlockFallbackShorthandProperty)
> +		       borderBlockFallbackShorthandProperty =
CSSPropertyBorderBlockColor;
> +		   // FIXME: Deal with cases where only some of
border-block-(start|end) are specified.
> +		   ASSERT(CSSPropertyBorderBlock - firstCSSProperty <
shorthandPropertyAppeared.size());
> +		   if (!shorthandPropertyAppeared[CSSPropertyBorderBlock -
firstCSSProperty]) {
> +		       value = borderPropertyValue(borderBlockWidthShorthand(),
borderBlockStyleShorthand(), borderBlockColorShorthand());
> +		       if (value.isNull())
> +			   shorthandPropertyAppeared.set(CSSPropertyBorderBlock
- firstCSSProperty);
> +		       else
> +			   shorthandPropertyID = CSSPropertyBorderBlock;
> +		   } else if (shorthandPropertyUsed[CSSPropertyBorderBlock -
firstCSSProperty])
> +		       shorthandPropertyID = CSSPropertyBorderBlock;
> +		   if (!shorthandPropertyID)
> +		       shorthandPropertyID =
borderBlockFallbackShorthandProperty;

This is complicated enough that maybe it can move into a separate function.

> Source/WebCore/css/StyleProperties.cpp:1094
> +		   if (!borderInlineFallbackShorthandProperty)
> +		       borderInlineFallbackShorthandProperty =
CSSPropertyBorderInlineColor;
> +		   // FIXME: Deal with cases where only some of
border-inline-(start|end) are specified.
> +		   ASSERT(CSSPropertyBorderInline - firstCSSProperty <
shorthandPropertyAppeared.size());
> +		   if (!shorthandPropertyAppeared[CSSPropertyBorderInline -
firstCSSProperty]) {
> +		       value =
borderPropertyValue(borderInlineWidthShorthand(), borderInlineStyleShorthand(),
borderInlineColorShorthand());
> +		       if (value.isNull())
> +			  
shorthandPropertyAppeared.set(CSSPropertyBorderInline - firstCSSProperty);
> +		       else
> +			   shorthandPropertyID = CSSPropertyBorderInline;
> +		   } else if (shorthandPropertyUsed[CSSPropertyBorderInline -
firstCSSProperty])
> +		       shorthandPropertyID = CSSPropertyBorderInline;
> +		   if (!shorthandPropertyID)
> +		       shorthandPropertyID =
borderInlineFallbackShorthandProperty;

Ditto.

> Source/WebCore/css/parser/CSSPropertyParser.h:69
> +    bool consume2Values(const StylePropertyShorthand&, bool important);

I would call this consume2ValueShorthand()

> Source/WebCore/css/parser/CSSPropertyParser.h:70
>      bool consume4Values(const StylePropertyShorthand&, bool important);

And rename this accordingly.