[webkit-reviews] review canceled: [Bug 94094] [css3-text] Add rendering support for -webkit-text-decoration-style : [Attachment 162523] 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 162523: Patch
https://bugs.webkit.org/attachment.cgi?id=162523&action=review

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


Sorry for the delay.

Some comments but the general direction is good. If we can get another round
with passing EWS, it should be good to go.

> Source/WebCore/rendering/InlineTextBox.cpp:763
> +#if ENABLE(CSS3_TEXT_DECORATION)
> +	   paintDecoration(context, boxOrigin, textDecorations,
styleToUse->textDecorationStyle(), textShadow);
> +#else
> +	   paintDecoration(context, boxOrigin, textDecorations,
TextDecorationStyleSolid, textShadow);
> +#endif // CSS3_TEXT_DECORATION

If textDecorationStyle was not #if'defed out you could just write this down as:


paintDecoration(context, boxOrigin, textDecorations,
styleToUse->textDecorationStyle(), textShadow);

which looks better to me.

> Source/WebCore/rendering/InlineTextBox.cpp:985
> +    StrokeStyle strokeStyle = SolidStroke;
> +#if ENABLE(CSS3_TEXT_DECORATION)
> +    switch (decorationStyle) {
> +    case TextDecorationStyleSolid:
> +	   break;
> +    case TextDecorationStyleDouble:
> +	   strokeStyle = DoubleStroke;
> +	   break;
> +    case TextDecorationStyleDotted:
> +	   strokeStyle = DottedStroke;
> +	   break;
> +    case TextDecorationStyleDashed:
> +	   strokeStyle = DashedStroke;
> +	   break;
> +    case TextDecorationStyleWavy:
> +	   // FIXME: https://bugs.webkit.org/show_bug.cgi?id=92868 - Needs
platform support.
> +	   strokeStyle = WavyStroke;
> +	   break;
> +    }

This should go in a helper function so that you just write:

StrokeStyle strokeStyle = textDecorationStyleToStrokeStyle(decorationStyle);

It also would remove the weird TextDecorationStyleSolid case and the
UNUSED_PARAM which are artificial.

>>> Source/WebCore/rendering/InlineTextBox.cpp:1028
>>> +		     context->drawLineForText(FloatPoint(localOrigin.x(),
localOrigin.y() + doubleOffset + 2 * baseline / 3), width, isPrinting);
>> 
>> Where does this formula come from? Is this matching what Firefox or IE are
doing?
> 
> This formula mimics the behavior from single line-through (see line 1030),
with the addition of doubleOffset (which is never zero, so it avoids lines
colliding with each other). As far as I've checked it matches what Firefox is
doing.

Please add some comment about that in the ChangeLog.

> Source/WebCore/rendering/style/RenderStyleConstants.h:-351
> -#endif // CSS3_TEXT_DECORATION

You should still hide the non-reachable value for TextDecorationStyle if
CSS3_TEXT_DECORATION is disabled.

>>> LayoutTests/ChangeLog:14
>>> +	     *
fast/css3-text-decoration/repaint/repaint-text-decoration-style-expected.png:
Added.
>> 
>> This baseline is wrong and was generated on a platform that doesn't support
repaint tracking.
> 
> Shall I completely remove pixel results from both Qt and Chromium, then? I
could add a line to TestExpectations to ignore pixel results from this test
until Chromium fixes the line style rendering issues.

Fine by me.