[webkit-reviews] review denied: [Bug 121806] Initial implementation of text-decoration-skip ink : [Attachment 212399] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Sep 23 16:53:38 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 212399: Patch
https://bugs.webkit.org/attachment.cgi?id=212399&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=212399&action=review
> Source/WebCore/ChangeLog:10
> + Tests:
fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-i
nk-zoomed.html
> +
fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-i
nk.html
> +
fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-r
oundtrip.html
Tests should be listed below the description of the change.
> Source/WebCore/ChangeLog:13
> + The implementation draws text with a thick stroke into a mask,
> + then uses the mask to draw underlines.
This needs more words. You should link to the spec, say whether it's on by
default, and summarize how the implementation works, and the major changes you
had to make.
Basically someone who has no knowledge of text-decoration or the code should be
able to get a good understanding of the change by reading this.
> Source/WebCore/ChangeLog:46
> + * Configurations/FeatureDefines.xcconfig:
> + * css/CSSComputedStyleDeclaration.cpp:
> + (WebCore::renderTextDecorationSkipFlagsToCSSValue):
> + (WebCore::ComputedStyleExtractor::propertyValue):
> + * css/CSSParser.cpp:
> + (WebCore::CSSParser::parseValue):
> + (WebCore::CSSParser::parseTextDecorationSkip):
> + * css/CSSParser.h:
> + * css/CSSPrimitiveValueMappings.h:
> + (WebCore::CSSPrimitiveValue::operator TextDecorationSkip):
> + * css/CSSPropertyNames.in:
> + * css/CSSValueKeywords.in:
> + * css/DeprecatedStyleBuilder.cpp:
> + (WebCore::ApplyPropertyTextDecorationSkip::applyValue):
> + (WebCore::ApplyPropertyTextDecorationSkip::createHandler):
> + (WebCore::DeprecatedStyleBuilder::DeprecatedStyleBuilder):
> + * css/StyleResolver.cpp:
> + (WebCore::StyleResolver::applyProperty):
> + * rendering/InlineTextBox.cpp:
> + (WebCore::InlineTextBox::paintText):
> + (WebCore::InlineTextBox::paint):
> + (WebCore::computeUnderlineOffset):
> + (WebCore::InlineTextBox::paintDecoration):
> + * rendering/InlineTextBox.h:
> + * rendering/style/RenderStyle.h:
> + * rendering/style/RenderStyleConstants.h:
> + (WebCore::operator| ):
> + (WebCore::operator|= ):
> + * rendering/style/StyleRareInheritedData.cpp:
> + (WebCore::StyleRareInheritedData::StyleRareInheritedData):
> + (WebCore::StyleRareInheritedData::operator==):
> + * rendering/style/StyleRareInheritedData.h:
Any of the per-file changes that are non-obvious should be explained here.
> Source/WebCore/rendering/InlineTextBox.cpp:535
> +struct PaintTextArguments {
> + GraphicsContext* context;
> + bool paintSelectedTextOnly;
> + bool paintSelectedTextSeparately;
> + float textStrokeWidth;
> + Color textStrokeColor;
> + Color textFillColor;
> + RenderStyle* styleToUse;
> + const Font& font;
> + int sPos;
> + int ePos;
> + int length;
> + const AtomicString& emphasisMark;
> + Color emphasisMarkColor;
> + RenderCombineText* combinedText;
> + TextRun& textRun;
> + FloatRect& boxRect;
> + FloatPoint& textOrigin;
> + const ShadowData* textShadow;
> + int emphasisMarkOffset;
> + float selectionStrokeWidth;
> + Color selectionFillColor;
> + const ShadowData* selectionShadow;
> + Color selectionEmphasisMarkColor;
> + Color selectionStrokeColor;
> +};
This is pretty gross. Should some of this code be factored into a new class?
> Source/WebCore/rendering/InlineTextBox.cpp:537
> +void InlineTextBox::paintText(PaintTextArguments* args)
If args is required, it should be a reference.
> Source/WebCore/rendering/InlineTextBox.cpp:619
> + if (!paintSelectedTextOnly) {
> + // For stroked painting, we have to change the text drawing mode.
It's probably dangerous to leave that mutated as a side
> + // effect, so only when we know we're stroking, do a save/restore.
> + GraphicsContextStateSaver stateSaver(*context, textStrokeWidth > 0);
> +
> + updateGraphicsContext(context, textFillColor, textStrokeColor,
textStrokeWidth, styleToUse->colorSpace());
> + if (!paintSelectedTextSeparately || ePos <= sPos) {
> + // FIXME: Truncate right-to-left text correctly.
> + paintTextWithShadows(context, font, textRun, nullAtom, 0, 0,
length, length, textOrigin, boxRect, textShadow, textStrokeWidth > 0,
isHorizontal());
> + } else
> + paintTextWithShadows(context, font, textRun, nullAtom, 0, ePos,
sPos, length, textOrigin, boxRect, textShadow, textStrokeWidth > 0,
isHorizontal());
> +
> + if (!emphasisMark.isEmpty()) {
> + updateGraphicsContext(context, emphasisMarkColor,
textStrokeColor, textStrokeWidth, styleToUse->colorSpace());
> +
> + DEFINE_STATIC_LOCAL(TextRun, objectReplacementCharacterTextRun,
(&objectReplacementCharacter, 1));
> + TextRun& emphasisMarkTextRun = combinedText ?
objectReplacementCharacterTextRun : textRun;
> + FloatPoint emphasisMarkTextOrigin = combinedText ?
FloatPoint(boxOrigin.x() + boxRect.width() / 2, boxOrigin.y() +
font.fontMetrics().ascent()) : textOrigin;
> + if (combinedText)
> + context->concatCTM(rotation(boxRect, Clockwise));
> +
> + if (!paintSelectedTextSeparately || ePos <= sPos) {
> + // FIXME: Truncate right-to-left text correctly.
> + paintTextWithShadows(context, combinedText ?
combinedText->originalFont() : font, emphasisMarkTextRun, emphasisMark,
emphasisMarkOffset, 0, length, length, emphasisMarkTextOrigin, boxRect,
textShadow, textStrokeWidth > 0, isHorizontal());
> + } else
> + paintTextWithShadows(context, combinedText ?
combinedText->originalFont() : font, emphasisMarkTextRun, emphasisMark,
emphasisMarkOffset, ePos, sPos, length, emphasisMarkTextOrigin, boxRect,
textShadow, textStrokeWidth > 0, isHorizontal());
> +
> + if (combinedText)
> + context->concatCTM(rotation(boxRect, Counterclockwise));
> + }
> + }
> +
> + if ((paintSelectedTextOnly || paintSelectedTextSeparately) && sPos <
ePos) {
> + // paint only the text that is selected
> + GraphicsContextStateSaver stateSaver(*context, selectionStrokeWidth
> 0);
> +
> + updateGraphicsContext(context, selectionFillColor,
selectionStrokeColor, selectionStrokeWidth, styleToUse->colorSpace());
> + paintTextWithShadows(context, font, textRun, nullAtom, 0, sPos,
ePos, length, textOrigin, boxRect, selectionShadow, selectionStrokeWidth > 0,
isHorizontal());
> + if (!emphasisMark.isEmpty()) {
> + updateGraphicsContext(context, selectionEmphasisMarkColor,
textStrokeColor, textStrokeWidth, styleToUse->colorSpace());
> +
> + DEFINE_STATIC_LOCAL(TextRun, objectReplacementCharacterTextRun,
(&objectReplacementCharacter, 1));
> + TextRun& emphasisMarkTextRun = combinedText ?
objectReplacementCharacterTextRun : textRun;
> + FloatPoint emphasisMarkTextOrigin = combinedText ?
FloatPoint(boxOrigin.x() + boxRect.width() / 2, boxOrigin.y() +
font.fontMetrics().ascent()) : textOrigin;
> + if (combinedText)
> + context->concatCTM(rotation(boxRect, Clockwise));
> +
> + paintTextWithShadows(context, combinedText ?
combinedText->originalFont() : font, emphasisMarkTextRun, emphasisMark,
emphasisMarkOffset, sPos, ePos, length, emphasisMarkTextOrigin, boxRect,
selectionShadow, selectionStrokeWidth > 0, isHorizontal());
> +
> + if (combinedText)
> + context->concatCTM(rotation(boxRect, Counterclockwise));
> + }
> + }
> +}
Are there any behavior changes in this big chunk of code that got moved?
> Source/WebCore/rendering/InlineTextBox.cpp:850
> + PaintTextArguments arguments = {context, paintSelectedTextOnly,
paintSelectedTextSeparately, textStrokeWidth, textStrokeColor, textFillColor,
styleToUse, font, sPos, ePos, length, emphasisMark, emphasisMarkColor,
combinedText, textRun, boxRect, textOrigin, textShadow, emphasisMarkOffset,
selectionStrokeWidth, selectionFillColor, selectionShadow,
selectionEmphasisMarkColor, selectionStrokeColor};
PaintTextArguments could have a ctor.
> Source/WebCore/rendering/InlineTextBox.cpp:859
> + // Clobbers |arguments|
Evil.
> Source/WebCore/rendering/InlineTextBox.cpp:1047
> + const int gap = max<int>(1, ceilf(textDecorationThickness * 3.0 / 4.0));
You shouldn't make changes to hardcoded values without justification. Also, in
theory, this would require a lot of pixel tests to be rebaselined.
> Source/WebCore/rendering/InlineTextBox.cpp:1058
> + return inlineTextBox->logicalHeight() + gap + max<float>(offset,
0.0f);
You can just say max<float>(offset, 0);
> Source/WebCore/rendering/InlineTextBox.cpp:1194
> + const float textDecorationThickness = args->font.size() / 12.0f;
More magical hardcoded values being changed.
> Source/WebCore/rendering/InlineTextBox.cpp:1269
> + if (clipDecorationToMask) {
> + context->save();
Use GraphicsContextStateSaver.
> Source/WebCore/rendering/InlineTextBox.cpp:1272
> + AffineTransform ctm =
context->getCTM(GraphicsContext::DefinitelyIncludeDeviceScale);
We discussed how to just use the ctm's scale.
> Source/WebCore/rendering/InlineTextBox.cpp:1281
> + OwnPtr<ImageBuffer> imageBuffer =
ImageBuffer::create(enclosingDeviceRect.size());
I think this should be createCompatibleBuffer() (but note that it takes the
device scale into account).
> Source/WebCore/rendering/InlineTextBox.cpp:1282
> + GraphicsContext* maskContext = imageBuffer->context();
What if the buffer allocation fails. Did you test crazy context scaling?
> Source/WebCore/rendering/InlineTextBox.cpp:1283
> + maskContext->setShouldAntialias(true);
Isn't it true by default?
> Source/WebCore/rendering/InlineTextBox.cpp:1349
> + if (clipDecorationToMask)
> + context->restore();
GraphicsContextStateSaver does this for you.
> Source/WebCore/rendering/style/RenderStyleConstants.h:376
> +enum TextDecorationSkip {
> + TextDecorationSkipNone = 0x0, TextDecorationSkipInk = 0x4
> +};
> +inline TextDecorationSkip operator| (TextDecorationSkip a,
TextDecorationSkip b) { return TextDecorationSkip(int(a) | int(b)); }
> +inline TextDecorationSkip& operator|= (TextDecorationSkip& a,
TextDecorationSkip b) { return a = a | b; }
I don't really like this approach (even though I see us doing it for
TextDecoration etc). I think it's better to explicitly list the bit shifting:
enum Thing {
First = 1 << 0,
Second = 1 << 1,
...}
typedef unsigned Things; Or there may be a cooler C++y way to do this now.
> Source/WebCore/rendering/style/StyleRareInheritedData.h:127
> + unsigned m_textDecorationSkip : 5; // TextDecorationSkip
You only need 3 bits, right?
More information about the webkit-reviews
mailing list