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

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied 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 159732: Patch
https://bugs.webkit.org/attachment.cgi?id=159732&action=review

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


r- for the 'double' decoration which I think is badly implemented. I think you
will need a bit more testing of this case as it's more complex.

> Source/WebCore/ChangeLog:16
> +	   94093 and should be landed first.

If 94093 wasn't landed, this change wouldn't compile when the flag is enabled.
I hope you are ensuring this ordering to happen so it's not super useful to
mention in the ChangeLog.

> Source/WebCore/rendering/InlineTextBox.cpp:988
> +#endif // CSS3_TEXT_DECORATION

Changing the function signature to always have |decorationStyle| will likely
break the build due to the unused variable if you disabled the compile time
flag. You can use UNUSED_PARAM in an #else branch here to solve the issue.

> Source/WebCore/rendering/InlineTextBox.cpp:1008
> +#if ENABLE(CSS3_TEXT_DECORATION)
> +	   context->setStrokeStyle(strokeStyle);
> +#else
> +	   context->setStrokeStyle(SolidStroke);
> +#endif // CSS3_TEXT_DECORATION

If you move the definition of |strokeStyle| out of your #if-def above, you can
replace that with context->setStrokeStyle(strokeStyle);

> Source/WebCore/rendering/InlineTextBox.cpp:1031
> +		   context->drawLineForText(FloatPoint(localOrigin.x(),
localOrigin.y() + 1.5 * baseline / 3), width, isPrinting);
> +		   context->drawLineForText(FloatPoint(localOrigin.x(),
localOrigin.y() + 2.5 * baseline / 3), width, isPrinting);

I don't think this is right. 'double' should match 'border-style' and here is
what CSS 2.1 says about 'double':

Two parallel solid lines with some space between them. (The thickness of the
lines is not specified, but the sum of the lines and the space must equal
‘border-width’.)

You are basically drawing a border that will be too thick. Also what is
preventing those 2 lines to overlap?

Probably a crazy idea but shouldn't there be a way to unify some of this code
with our border painting code (see RenderObject::drawLineForBoxSide)? We
already support 'border-style: double' (and other values) and I would rather
avoid re-implementing that logic with more bugs in it.

> Source/WebCore/rendering/InlineTextBox.h:187
> +#if ENABLE(CSS3_TEXT_DECORATION)
> +    void paintDecoration(GraphicsContext*, const FloatPoint& boxOrigin,
ETextDecoration textDecorations, TextDecorationStyle, const ShadowData*);
> +#else
>      void paintDecoration(GraphicsContext*, const FloatPoint& boxOrigin, int
decoration, const ShadowData*);
> +#endif // CSS3_TEXT_DECORATION

We shouldn't have 2 different declarations / definitions of this function. If
you need to pass TextDecorationStyle then let's do it.

> LayoutTests/ChangeLog:11
> +	   *
fast/css3-text-decoration/repaint/repaint-text-decoration-style-expected.png:
Added.

It's sad that we land a bad baseline but we don't have much choice here, unless
you implement Mac or Chromium. It's worth mentioning that the baseline is known
to over-repaint.

>
LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-style.htm
l:12
> +	   <script src="../../repaint/resources/repaint.js"
type="text/javascript"></script>

We don't really need a repaint test to test the basic functionality. I would
rather have a regular paint test for that and a small repaint test to see that
invalidation are done properly.

>
LayoutTests/fast/css3-text-decoration/repaint/repaint-text-decoration-style.htm
l:18
> +		  
document.getElementById("test-underline-dashed").style.webkitTextDecorationStyl
e = 'dashed';

We don't really see the dashed one due to the small length of your text below.
I definitely think it would be better to have one long line per invalidation as
it would make the results more obvious.