[webkit-reviews] review denied: [Bug 121806] Initial implementation of text-decoration-skip ink : [Attachment 214383] 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 214383: Patch
https://bugs.webkit.org/attachment.cgi?id=214383&action=review

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


> Source/WebCore/rendering/InlineTextBox.cpp:1009
> +	   if (clipDecorationToMask) {
> +	       context->save();

Look at GraphicsContextStateSaver.

> Source/WebCore/rendering/InlineTextBox.cpp:1017
> +	       IntRect enclosingDeviceRect =
enclosingIntRect(ctm.mapQuad(boxRect).boundingBox());
> +
> +	       OwnPtr<ImageBuffer> imageBuffer =
context->createCompatibleBuffer(enclosingDeviceRect.size());

Does this do the right thing in rotated contexts?

> Source/WebCore/rendering/InlineTextBox.cpp:1023
> +		   maskContext->concatCTM(ctm);

Did createCompatibleBuffer() already put the device scale into the CTM? If so,
you might be multiplying the device scale in twice. Did you test on retina?

> Source/WebCore/rendering/InlineTextBox.cpp:1031
> +		   context->concatCTM(ctm.inverse());
> +		   context->clipToImageBuffer(imageBuffer.get(),
enclosingDeviceRect);

I don't get why you concat the inverse of the CTM. This is basically converting
back to base space.

> Source/WebCore/rendering/TextPainter.cpp:58
> +    bool opaque = fillColor.alpha() == 255;

color.hasAlpha()

> Source/WebCore/rendering/TextPainter.cpp:73
> +	       if (emphasisMark.isEmpty())
> +		   context->drawText(font, textRun, textOrigin + extraOffset,
startOffset, endOffset);
> +	       else
> +		   context->drawEmphasisMarks(font, textRun, emphasisMark,
textOrigin + extraOffset + IntSize(0, emphasisMarkOffset), startOffset,
endOffset);

This pattern occurs three times. Move it into a function?

> Source/WebCore/rendering/TextPainter.h:56
> +    const FloatRect& boxRect() { return m_boxRect; }

Should be a const function.

> Source/WebCore/rendering/TextPainter.h:58
> +    void paintThickTextInMask(GraphicsContext&);

Not sure why it needs "thick" in the name. Why not pass in the "thickness"?
Also, the implementation doesn't paint into a mask, right?

> Source/WebCore/rendering/TextPainter.h:65
> +    int m_sPos;
> +    int m_ePos;

Names are too mysterious.

> Source/WebCore/rendering/TextPainter.h:72
> +    int m_emphasisMarkOffset;

Why not a float?

> Source/WebCore/rendering/TextPainter.h:73
> +    bool m_horizontal;

What is horizontal?

>
LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decor
ation-skip-roundtrip-multiple.html:11
> +    stylesheet.insertRule(".p { -webkit-text-decoration-skip: ink ink ink
ink ink; }", 0);

We normally have a single test with "parsing" in the name that test multiple
variants of the property value. Here, you should test stuff like "garbage ink",
"garbage", "ink garbage ink".

> LayoutTests/platform/mac/TestExpectations:832
> +webkit.org/b/58491 fast/css3-text/css3-text-decoration/text-decoration-skip
[ Pass ]

Why did you add a Pass here? You don't need to add anything for a new test that
passes.