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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 215747: Patch
https://bugs.webkit.org/attachment.cgi?id=215747&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=215747&action=review


> Source/WebCore/ChangeLog:11
> +	   3. Redraw text into this ImageBuffer with a thicker stroke

Just Draw, it's not drawing again.

> Source/WebCore/ChangeLog:14
> +	   6. Un-apply the mask

One does not really un-apply a mask.

> Source/WebCore/rendering/InlineTextBox.cpp:814
> +static float getWavyStrokeControlPointDistance(float strokeThickness)
> +{
> +    return 3 * max<float>(2, strokeThickness);
> +}
> +
> +static float getWavyStrokeStep(float strokeThickness)

We only use getFoo for functions with out params.

> Source/WebCore/rendering/InlineTextBox.cpp:923
> +static FloatRect getSingleDecorationBoundingBox(GraphicsContext& context,
const float textDecorationThickness,

"get" again.

What does "single decoration" mean? A comment would clarify.

We don't tend to use "const" on POD types in function arguments.

> Source/WebCore/rendering/InlineTextBox.cpp:940
> +	   FloatPoint start(localOrigin.x(), localOrigin.y() + yOffset);
> +	   FloatPoint end = start + FloatSize(width, 0);
> +	   context.adjustLineToPixelBoundaries(start, end,
textDecorationThickness, context.strokeStyle());
> +	   float controlPointDistance =
getWavyStrokeControlPointDistance(textDecorationThickness);
> +	   float step = getWavyStrokeStep(textDecorationThickness);
> +	   adjustStepToDecorationLength(step, controlPointDistance, width);
> +	   controlPointDistance += textDecorationThickness;
> +	   FloatPoint boundingBoxOrigin = start - FloatSize(0,
controlPointDistance);
> +	   FloatSize boundingBoxSize = FloatSize(width, 2 *
controlPointDistance);
> +	   boundingBox.unite(FloatRect(boundingBoxOrigin, boundingBoxSize));

Some blank lines would help to break up this wall of code.

Maybe combine getWavyStrokeControlPointDistance and getWavyStrokeStep into one
function?

boundingBox.unite here is confusing.

> Source/WebCore/rendering/InlineTextBox.cpp:944
> +	  
boundingBox.unite(context.computeLineBoundsForText(FloatPoint(localOrigin.x(),
localOrigin.y() + underlineOffset), width, isPrinting));

Confusing to .unite() here because nothing has previously set the rect.

> Source/WebCore/rendering/InlineTextBox.cpp:951
> +static FloatRect getDecorationBoundingBox(InlineTextBox& inlineTextBox,
GraphicsContext& context, TextDecoration deco, const float
textDecorationThickness, const float width, const float doubleOffset, const
TextDecorationStyle decorationStyle, const FloatPoint localOrigin, const
RenderStyle& lineStyle, const bool isPrinting, const int baseline)

"get" again. Don't abbreviate "deco". Don't use const on POD.

> Source/WebCore/rendering/InlineTextBox.cpp:963
> +	   boundingBox.unite(getSingleDecorationBoundingBox(context,
textDecorationThickness, width, localOrigin, decorationStyle, isPrinting,
underlineOffset + doubleOffset, underlineOffset, baseline + 1));
> +    }
> +    if (deco & TextDecorationOverline)
> +	   boundingBox.unite(getSingleDecorationBoundingBox(context,
textDecorationThickness, width, localOrigin, decorationStyle, isPrinting,
-doubleOffset, 0, -doubleOffset));
> +    if (deco & TextDecorationLineThrough)
> +	   boundingBox.unite(getSingleDecorationBoundingBox(context,
textDecorationThickness, width, localOrigin, decorationStyle, isPrinting, 2 *
baseline / 3, 2 * baseline / 3, doubleOffset + 2 * baseline / 3));

Don't use .unite unless you mean it.

> Source/WebCore/rendering/InlineTextBox.cpp:1045
> +	   TextDecorationSkip textDecorationSkip =
lineStyle.textDecorationSkip();

I, like Darin before me, don't see the need for a separate variable.

> Source/WebCore/rendering/InlineTextBox.cpp:1051
> +	       const float skipInkGapWidth = 1.f;

No need for .f

> Source/WebCore/rendering/InlineTextBox.cpp:1055
> +	       const FloatRect underlineRect = getDecorationBoundingBox(*this,
*context, deco, textDecorationThickness, width, doubleOffset, decorationStyle,
localOrigin, lineStyle, isPrinting, baseline);

We don't tend to use const on local variables.