[webkit-reviews] review granted: [Bug 121806] Initial implementation of text-decoration-skip ink : [Attachment 215763] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 121806: Initial implementation of text-decoration-skip ink
https://bugs.webkit.org/show_bug.cgi?id=121806

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215763&action=review


> Source/WebCore/rendering/InlineTextBox.cpp:816
> +    controlPointDistance = 3 * max<float>(2, strokeThickness);

std::max

> Source/WebCore/rendering/InlineTextBox.cpp:821
> +    step = 2 * max<float>(2, strokeThickness);

std::max

> Source/WebCore/rendering/InlineTextBox.cpp:923
> +/*
> + * Because finding the bounding box of an underline is structurally similar
to finding
> + * the bounding box of a strokethrough, we can pull out the computation and
parameterize
> + * by the location of the decoration (yOffset, underlineOffset, and
doubleOffset).
> + */

Should be a “//“ comment, not a “/* */“ comment.

Isn’t it normally called strikethrough rather than strokethrough?

> Source/WebCore/rendering/InlineTextBox.cpp:926
> +static FloatRect boundingBoxForSingleDecoration(GraphicsContext& context,
float textDecorationThickness,
> +    float width, const FloatPoint localOrigin, TextDecorationStyle
decorationStyle,
> +    bool isPrinting, float yOffset, float underlineOffset, float
doubleOffset)

const FloatPoint is wrong. Just FloatPoint, or const FloatPoint& if we thought
that was better for performance (we don’t).

> Source/WebCore/rendering/InlineTextBox.cpp:936
> +	   float controlPointDistance, step;

We normally don’t define more than one local in a line like this, although I
think it looks OK here.

> Source/WebCore/rendering/InlineTextBox.cpp:950
> +	   boundingBox =
context.computeLineBoundsForText(FloatPoint(localOrigin.x(), localOrigin.y() +
underlineOffset), width, isPrinting);
> +	   if (decorationStyle == TextDecorationStyleDouble)
> +	      
boundingBox.unite(context.computeLineBoundsForText(FloatPoint(localOrigin.x(),
localOrigin.y() + doubleOffset), width, isPrinting));

Might write this as localOrigin + FloatSize(0, underlineOffset) and localOrigin
+ FloatSize(0, doubleOffset) instead.

> Source/WebCore/rendering/InlineTextBox.cpp:959
> +	   TextUnderlinePosition underlinePosition =
lineStyle.textUnderlinePosition();

Not all that helpful to have this local. I would just evaluate this in the next
line.

> Source/WebCore/rendering/InlineTextBox.cpp:960
> +	   const int underlineOffset =
computeUnderlineOffset(underlinePosition, lineStyle.fontMetrics(),
&inlineTextBox, textDecorationThickness);

Remove the const here.

> Source/WebCore/rendering/InlineTextBox.cpp:1063
> +		   GraphicsContext* maskContext = imageBuffer->context();

Should use a reference instead of pointer here since this is never null.

> Source/WebCore/rendering/InlineTextBox.h:173
> +    void paintDecoration(GraphicsContext*, const FloatPoint& boxOrigin,
TextDecoration, TextDecorationStyle, const ShadowData*, TextPainter&);

Over time these functions should all take GraphicsContext& instead of
GraphicsContext*. Would be nice to do it for this function since we had to
touch all call sites anyway.