[webkit-reviews] review canceled: [Bug 94094] [css3-text] Add rendering support for -webkit-text-decoration-style : [Attachment 168745] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Bruno Abinader
<bruno.abinader at basyskom.com>'s request for review:
Bug 94094: [css3-text] Add rendering support for -webkit-text-decoration-style
https://bugs.webkit.org/show_bug.cgi?id=94094

Attachment 168745: Patch
https://bugs.webkit.org/attachment.cgi?id=168745&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168745&action=review


> Source/WebCore/platform/graphics/GraphicsContext.h:147
> +#if ENABLE(CSS3_TEXT_DECORATION)
> +	   DoubleStroke,
> +	   DottedStroke,
> +	   DashedStroke,
> +	   WavyStroke
> +#else
>	   DottedStroke,
>	   DashedStroke
> +#endif // CSS3_TEXT_DECORATION

You may as well just put the DoubleStroke and WavyStroke at the end. It would
be more readable to me even if not alphabetically ordered.

> Source/WebCore/rendering/style/RenderStyle.h:-622
> -#if ENABLE(CSS3_TEXT_DECORATION)
>      TextDecorationStyle textDecorationStyle() const { return
static_cast<TextDecorationStyle>(rareNonInheritedData->m_textDecorationStyle);
}
> -#endif // CSS3_TEXT_DECORATION

This could return the only available value (TextDecorationStyleSolid) when
CSS3_TEXT_DECORATION is disabled instead of having to remove the #if
everywhere.

> Source/WebCore/rendering/style/RenderStyleConstants.h:349
>  };

Exposing all the values is what breaks all platforms as they need to be handled
in 'switch' statements. I would just define TextDecorationStyleSolid if
CSS3_TEXT_DECORATION is off and return this value in textDecorationStyle above.
Open to better suggestions though.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:-183
> -#if ENABLE(CSS3_TEXT_DECORATION)
>      unsigned m_textDecorationStyle : 3; // TextDecorationStyle
> -#endif // CSS3_TEXT_DECORATION

You are basically adding the cost of the new feature even if we don't enable it
which is wrong. This should still be behind #if guard.