[webkit-reviews] review denied: [Bug 121806] Initial implementation of text-decoration-skip ink : [Attachment 214383] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 16 12:11:04 PDT 2013
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.
More information about the webkit-reviews
mailing list