Bug 121806

Created attachment 212399 [details] Patch

Comment on attachment 212399 [details] Patch Attachment 212399 [details] did not pass gtk-wk2-ews (gtk-wk2): Output: http://webkit-queues.appspot.com/results/1947169

Comment on attachment 212399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212399&action=review > Source/WebCore/rendering/InlineTextBox.cpp:1275 > + ctm.setE(0.0); These two lines aren't quite right.

Comment on attachment 212399 [details] Patch 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-ink-zoomed.html > + fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-ink.html > + fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip.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?

<rdar://problem/15100907>

*** Bug 95856 has been marked as a duplicate of this bug. ***

Comment on attachment 212399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212399&action=review >> Source/WebCore/ChangeLog:10 >> + fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip.html > > Tests should be listed below the description of the change. Done. >> Source/WebCore/ChangeLog:13 >> + 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. I've added more of an explanation >> Source/WebCore/ChangeLog:46 >> + * rendering/style/StyleRareInheritedData.h: > > Any of the per-file changes that are non-obvious should be explained here. I think all of the per-file changes are fairly obvious, except for the changes to InlineTextBox. However, I think the description of the entire change does a good job at explaining the changes to InlineTextBox. Let me know if I should be more explicit than I already am. >> Source/WebCore/rendering/InlineTextBox.cpp:535 >> +}; > > This is pretty gross. Should some of this code be factored into a new class? I put it into a new class with a constructor. I feel like this class should probably stay within this .cpp file though, since it wouldn't be used anywhere else. This class only has semantic meaning with respect to PaintText() - it doesn't signify any structural piece of a webpage. Let me know if you think I should move it into its own file. >> Source/WebCore/rendering/InlineTextBox.cpp:537 >> +void InlineTextBox::paintText(PaintTextArguments* args) > > If args is required, it should be a reference. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:619 >> +} > > Are there any behavior changes in this big chunk of code that got moved? Nope. Pure copy and paste. >> 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. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:859 >> + // Clobbers |arguments| > > Evil. Done. >> 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. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1058 >> + return inlineTextBox->logicalHeight() + gap + max<float>(offset, 0.0f); > > You can just say max<float>(offset, 0); Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1194 >> + const float textDecorationThickness = args->font.size() / 12.0f; > > More magical hardcoded values being changed. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1269 >> + context->save(); > > Use GraphicsContextStateSaver. I don't think I can do that, because the code that I want to surround with save()/restore() doesn't have lexical scope. I want to restore in after drawing the underline but before drawing the line through. I think adding an extra pair of {}s to create a new block would be worse than just explicitly calling save() and restore() >> Source/WebCore/rendering/InlineTextBox.cpp:1272 >> + AffineTransform ctm = context->getCTM(GraphicsContext::DefinitelyIncludeDeviceScale); > > We discussed how to just use the ctm's scale. After talking to Dean about this, I think I have a better solution. >> Source/WebCore/rendering/InlineTextBox.cpp:1275 >> + ctm.setE(0.0); > > These two lines aren't quite right. Done. >> 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). Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1282 >> + GraphicsContext* maskContext = imageBuffer->context(); > > What if the buffer allocation fails. Did you test crazy context scaling? Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1283 >> + maskContext->setShouldAntialias(true); > > Isn't it true by default? Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1349 >> + context->restore(); > > GraphicsContextStateSaver does this for you. See above. >> Source/WebCore/rendering/style/RenderStyleConstants.h:376 >> +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. Done. >> Source/WebCore/rendering/style/StyleRareInheritedData.h:127 >> + unsigned m_textDecorationSkip : 5; // TextDecorationSkip > > You only need 3 bits, right? there are 5 values for text-decoration-skip, and the user can specify any subset of them

Created attachment 214024 [details] Patch

Attachment 214024 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-ink-zoomed-expected.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-ink-zoomed.html', u'LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip-expected.txt', u'LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip.html', u'LayoutTests/platform/mac/TestExpectations', u'Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/ChangeLog', u'Source/WebCore/Configurations/FeatureDefines.xcconfig', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/CSSValueKeywords.in', u'Source/WebCore/css/DeprecatedStyleBuilder.cpp', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/InlineTextBox.cpp', u'Source/WebCore/rendering/InlineTextBox.h', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/RenderStyleConstants.h', u'Source/WebCore/rendering/style/StyleRareInheritedData.cpp', u'Source/WebCore/rendering/style/StyleRareInheritedData.h', u'Source/WebKit/mac/Configurations/FeatureDefines.xcconfig', u'Source/WebKit2/Configurations/FeatureDefines.xcconfig', u'Tools/Scripts/webkitperl/FeatureList.pm']" exit_code: 1 Source/WebCore/css/DeprecatedStyleBuilder.cpp:1274: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 214024 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214024&action=review >> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1274 >> + static TextDecorationSkip valueToDecorationSkip(CSSValue* value) { > > Place brace on its own line for function definitions. [whitespace/braces] [4] Done.

Created attachment 214065 [details] Patch

Comment on attachment 214065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214065&action=review Patch does not apply, so you'll need to rebase it. I took a quick look and have a few comments, mostly on coding style. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2371 > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > Source/WebCore/css/CSSParser.cpp:2377 > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > Source/WebCore/css/CSSParser.h:289 > #if ENABLE(CSS3_TEXT) > bool parseTextUnderlinePosition(bool important); > -#endif > +#endif // CSS3_TEXT > +#if ENABLE(CSS3_TEXT_DECORATION) > + bool parseTextDecorationSkip(bool important); > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1291 > + static TextDecorationSkip valueToDecorationSkip(CSSValue* value) > + { > + CSSPrimitiveValue* pv = static_cast<CSSPrimitiveValue*>(value); > + > + ASSERT(pv->isValueID()); > + > + switch (pv->getValueID()) { > + case CSSValueNone: > + return TextDecorationSkipNone; > + case CSSValueInk: > + return TextDecorationSkipInk; > + default: > + break; > + } > + > + ASSERT_NOT_REACHED(); > + return TextDecorationSkipNone; > + } Since the argument is never null, it should be a reference rather than a pointer. Since the function works only on a CSSPrimitiveValue, the typecast should be done at the call site, so the argument should be CSSPrimitiveValue&. Please don’t use abbreviations made out of letters instead of words, such as "pv", for variable names in WebKit code. The code would be easier to read if the variable was named primitiveValue. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1300 > + TextDecorationSkip t = RenderStyle::initialTextDecorationSkip(); The name "t" doesn’t make sense. We avoid single letter variable names except for certain traditional places. I suggest the name "skip" here, since that's the term we chose in CSS. Or something else made with words. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:1304 > + CSSValue* item = i.value(); > + ASSERT_WITH_SECURITY_IMPLICATION(item->isPrimitiveValue()); > + t |= valueToDecorationSkip(item); The assertion should be done inside a casting function, toCSSPrimitiveValue, so we don’t have to write it out here. Is there a rule against listing the same thing over and over again? "ink ink ink ink ink"? If so, where is that enforced? If not, then we should have test cases for that. > Source/WebCore/css/DeprecatedStyleBuilder.cpp:2324 > +#if ENABLE(CSS3_TEXT_DECORATION) > + setPropertyHandler(CSSPropertyWebkitTextDecorationSkip, ApplyPropertyTextDecorationSkip::createHandler()); > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > Source/WebCore/css/StyleResolver.cpp:3051 > +#if ENABLE(CSS3_TEXT_DECORATION) > + case CSSPropertyWebkitTextDecorationSkip: > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > Source/WebCore/rendering/InlineTextBox.cpp:521 > +struct PaintTextArguments { > + PaintTextArguments(GraphicsContext* context, bool paintSelectedTextOnly, bool paintSelectedTextSeparately, const Font& font, > + int sPos, int ePos, int length, const AtomicString& emphasisMark, RenderCombineText* combinedText, TextRun& textRun, > + FloatRect& boxRect, FloatPoint& textOrigin, const ShadowData* textShadow, int emphasisMarkOffset, const ShadowData* selectionShadow, > + TextPaintStyle& textPaintStyle, TextPaintStyle& selectionPaintStyle) > + : m_context(context) > + , m_paintSelectedTextOnly(paintSelectedTextOnly) > + , m_paintSelectedTextSeparately(paintSelectedTextSeparately) > + , m_font(font) > + , m_sPos(sPos) > + , m_ePos(ePos) > + , m_length(length) > + , m_emphasisMark(emphasisMark) > + , m_combinedText(combinedText) > + , m_textRun(textRun) > + , m_boxRect(boxRect) > + , m_textOrigin(textOrigin) > + , m_textShadow(textShadow) > + , m_emphasisMarkOffset(emphasisMarkOffset) > + , m_selectionShadow(selectionShadow) > + , m_textPaintStyle(textPaintStyle) > + , m_selectionPaintStyle(selectionPaintStyle) > + { > + } > + GraphicsContext* m_context; > + bool m_paintSelectedTextOnly; > + bool m_paintSelectedTextSeparately; > + const Font& m_font; > + int m_sPos; > + int m_ePos; > + int m_length; > + const AtomicString& m_emphasisMark; > + RenderCombineText* m_combinedText; > + TextRun& m_textRun; > + FloatRect& m_boxRect; > + FloatPoint& m_textOrigin; > + const ShadowData* m_textShadow; > + int m_emphasisMarkOffset; > + const ShadowData* m_selectionShadow; > + TextPaintStyle& m_textPaintStyle; > + TextPaintStyle& m_selectionPaintStyle; > +}; This is a poor pattern. If we are going to have a struct with lots of members like this, they should not have "m_" prefixes. Those are for members of classes, where there is competition between the function members and the data members. But here using "m_" just makes it harder to read the code using the class. GraphicsContext* should be a reference, since it’s never null. Maybe some of the others as well. But also, putting all this state into an "arguments to a function" struct tells us that perhaps we need a class. Or that these should be in more logical groupings in some smaller structs. This mechanical approach where the struct is literally just "all the arguments to the function" is unlikely to be the right way to factor it. > Source/WebCore/rendering/InlineTextBox.cpp:543 > + GraphicsContext*& context = args.m_context; > + const bool& paintSelectedTextOnly = args.m_paintSelectedTextOnly; > + const bool& paintSelectedTextSeparately = args.m_paintSelectedTextSeparately; > + const Font& font = args.m_font; > + const int& sPos = args.m_sPos; > + const int& ePos = args.m_ePos; > + const int& length = args.m_length; > + const AtomicString& emphasisMark = args.m_emphasisMark; > + RenderCombineText*& combinedText = args.m_combinedText; > + TextRun& textRun = args.m_textRun; > + const FloatRect& boxRect = args.m_boxRect; > + const FloatPoint& boxOrigin = boxRect.location(); > + const FloatPoint& textOrigin = args.m_textOrigin; > + const ShadowData*& textShadow = args.m_textShadow; > + const int& emphasisMarkOffset = args.m_emphasisMarkOffset; > + const ShadowData*& selectionShadow = args.m_selectionShadow; > + const TextPaintStyle& textPaintStyle = args.m_textPaintStyle; > + const TextPaintStyle& selectionPaintStyle = args.m_selectionPaintStyle; This is messy and tells me we don’t have this quite right. It doesn’t make sense to move all this data en masse from the arguments to local variables better. We should use the arguments directly in the code or use a different pattern. It’s silly to use a const bool& or a const int& for this; it's a waste to follow a pointer each time to get back to the argument structure. So if we were doing something like this then we should use const& only where the object is more less expensive to copy than a pointer is. > Source/WebCore/rendering/InlineTextBox.cpp:1093 > +void InlineTextBox::paintDecoration(GraphicsContext* context, const FloatPoint& boxOrigin, TextDecoration deco, TextDecorationStyle decorationStyle, const ShadowData* shadow, PaintTextArguments& args) args is a confusing variable name here, since those are not all the args > Source/WebCore/rendering/InlineTextBox.cpp:1173 > + TextDecorationSkip textDecorationSkip = lineStyle.textDecorationSkip(); > + bool clipDecorationToMask = textDecorationSkip == TextDecorationSkipInk; There’s no need to put lineStyle.textDecorationSkip() into a local variable. > Source/WebCore/rendering/InlineTextBox.cpp:1207 > + args.m_textPaintStyle.strokeWidth += 1.0f; > + args.m_selectionPaintStyle.strokeWidth += 1.0f; Where does this constant of "1" come from? I think it should be a constant so we can make clearer what it means. I assume it’s an artistic choice about how far away from the text stroke the underlines should stop. > Source/WebCore/rendering/InlineTextBox.cpp:1209 > + args.m_textShadow = 0; > + args.m_selectionShadow = 0; nullptr instead of 0 > Source/WebCore/rendering/InlineTextBox.h:36 > struct CompositionUnderline; > class DocumentMarker; > +struct PaintTextArguments; > +class RenderCombineText; Our usual way to do this is to list all classes first in alphabetical order, then a blank line, then all structs in alphabetical order. > Source/WebCore/rendering/InlineTextBox.h:186 > + void paintText(PaintTextArguments& args); > + void paintDecoration(GraphicsContext*, const FloatPoint& boxOrigin, TextDecoration, TextDecorationStyle, const ShadowData*, PaintTextArguments& args); The name "args" should be omitted here. We only include argument names when the add clarity to the meaning of the arguments. > Source/WebCore/rendering/style/RenderStyle.h:589 > +#if ENABLE(CSS3_TEXT_DECORATION) > + TextDecorationSkip textDecorationSkip() const { return static_cast<TextDecorationSkip>(rareInheritedData->m_textDecorationSkip); } > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > Source/WebCore/rendering/style/RenderStyle.h:1174 > +#if ENABLE(CSS3_TEXT_DECORATION) > + void setTextDecorationSkip(TextDecorationSkip v) { SET_VAR(rareInheritedData, m_textDecorationSkip, v); } > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. I suggest the name "skip" instead of "v" for the argument. > Source/WebCore/rendering/style/RenderStyle.h:1698 > +#if ENABLE(CSS3_TEXT_DECORATION) > + static TextDecorationSkip initialTextDecorationSkip() { return TextDecorationSkipNone; } > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > Source/WebCore/rendering/style/RenderStyleConstants.h:384 > + TextDecorationSkipNone = 0x0, TextDecorationSkipInk = 0x4 Where does the "4" value come from? I think the value should be (1 << 0) to make it clear this is a bit mask. And to avoid creating the mystery of the two skipped bits! > Source/WebCore/rendering/style/RenderStyleConstants.h:387 > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > Source/WebCore/rendering/style/StyleRareInheritedData.cpp:120 > +#if ENABLE(CSS3_TEXT_DECORATION) > + , m_textDecorationSkip(RenderStyle::initialTextDecorationSkip()) > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > Source/WebCore/rendering/style/StyleRareInheritedData.cpp:329 > +#if ENABLE(CSS3_TEXT_DECORATION) > + && m_textDecorationSkip == o.m_textDecorationSkip > +#endif // CSS3_TEXT_DECORATION Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them.

Comment on attachment 214065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214065&action=review > Source/WebCore/rendering/InlineTextBox.cpp:1193 > + savedContext = args.m_context; > + savedTextStrokeWidth = args.m_textPaintStyle.strokeWidth; > + savedSelectionStrokeWidth = args.m_selectionPaintStyle.strokeWidth; > + savedTextShadow = args.m_textShadow; > + savedSelectionShadow = args.m_selectionShadow; This set of things that needs to be saved and restored seems like a good candidate to be in a separate struct. Then the would be a single line of code.

Generally it looks like this is going in a good direction. Keep it up!

Comment on attachment 214065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214065&action=review >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2371 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done. >> Source/WebCore/css/CSSParser.cpp:2377 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done. >> Source/WebCore/css/CSSParser.h:289 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done. >> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1291 >> + } > > Since the argument is never null, it should be a reference rather than a pointer. Since the function works only on a CSSPrimitiveValue, the typecast should be done at the call site, so the argument should be CSSPrimitiveValue&. > > Please don’t use abbreviations made out of letters instead of words, such as "pv", for variable names in WebKit code. The code would be easier to read if the variable was named primitiveValue. applyValue() has to match the signature of WebCore::PropertyHandler::ApplyFunction. I've changed valueToDecorationSkip. Done. >> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1300 >> + TextDecorationSkip t = RenderStyle::initialTextDecorationSkip(); > > The name "t" doesn’t make sense. We avoid single letter variable names except for certain traditional places. I suggest the name "skip" here, since that's the term we chose in CSS. Or something else made with words. Done. >> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2324 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done. >> Source/WebCore/css/StyleResolver.cpp:3051 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1093 >> +void InlineTextBox::paintDecoration(GraphicsContext* context, const FloatPoint& boxOrigin, TextDecoration deco, TextDecorationStyle decorationStyle, const ShadowData* shadow, PaintTextArguments& args) > > args is a confusing variable name here, since those are not all the args Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1173 >> + bool clipDecorationToMask = textDecorationSkip == TextDecorationSkipInk; > > There’s no need to put lineStyle.textDecorationSkip() into a local variable. I actually did this on purpose, since there is a logical difference between "use a mask" and "text-decoration-skip: ink is applied" Eventually boolean computation will be more complicated when other things require using a mask. >> Source/WebCore/rendering/InlineTextBox.cpp:1193 >> + savedSelectionShadow = args.m_selectionShadow; > > This set of things that needs to be saved and restored seems like a good candidate to be in a separate struct. Then the would be a single line of code. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1207 >> + args.m_selectionPaintStyle.strokeWidth += 1.0f; > > Where does this constant of "1" come from? I think it should be a constant so we can make clearer what it means. I assume it’s an artistic choice about how far away from the text stroke the underlines should stop. Yep. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1209 >> + args.m_selectionShadow = 0; > > nullptr instead of 0 Done. >> Source/WebCore/rendering/InlineTextBox.h:36 >> +class RenderCombineText; > > Our usual way to do this is to list all classes first in alphabetical order, then a blank line, then all structs in alphabetical order. Done. >> Source/WebCore/rendering/InlineTextBox.h:186 >> + void paintDecoration(GraphicsContext*, const FloatPoint& boxOrigin, TextDecoration, TextDecorationStyle, const ShadowData*, PaintTextArguments& args); > > The name "args" should be omitted here. We only include argument names when the add clarity to the meaning of the arguments. Done. >> Source/WebCore/rendering/style/RenderStyle.h:589 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done. >> Source/WebCore/rendering/style/RenderStyle.h:1174 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. > > I suggest the name "skip" instead of "v" for the argument. Done. Done. >> Source/WebCore/rendering/style/RenderStyle.h:1698 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done. >> Source/WebCore/rendering/style/RenderStyleConstants.h:384 >> + TextDecorationSkipNone = 0x0, TextDecorationSkipInk = 0x4 > > Where does the "4" value come from? I think the value should be (1 << 0) to make it clear this is a bit mask. And to avoid creating the mystery of the two skipped bits! It came from the spec. "ink" is the 3rd item in the list of available options, so i made it the 3rd bit. I'll change it. >> Source/WebCore/rendering/style/RenderStyleConstants.h:387 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done. >> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:120 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done. >> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:329 >> +#endif // CSS3_TEXT_DECORATION > > Comments on the #endif on these short #if sequences make the code harder to read rather than easier. Please omit them. Done.

Partial comment responses.

Comment on attachment 214065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214065&action=review >> Source/WebCore/css/DeprecatedStyleBuilder.cpp:1304 >> + t |= valueToDecorationSkip(item); > > The assertion should be done inside a casting function, toCSSPrimitiveValue, so we don’t have to write it out here. > > Is there a rule against listing the same thing over and over again? "ink ink ink ink ink"? If so, where is that enforced? If not, then we should have test cases for that. Done. The spec says "A double bar (||) separates two or more options: one or more of them must occur, in any order." I'd guess that this means there can be duplicates. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:521 >> +}; > > This is a poor pattern. If we are going to have a struct with lots of members like this, they should not have "m_" prefixes. Those are for members of classes, where there is competition between the function members and the data members. But here using "m_" just makes it harder to read the code using the class. > > GraphicsContext* should be a reference, since it’s never null. Maybe some of the others as well. > > But also, putting all this state into an "arguments to a function" struct tells us that perhaps we need a class. Or that these should be in more logical groupings in some smaller structs. This mechanical approach where the struct is literally just "all the arguments to the function" is unlikely to be the right way to factor it. I've made this a class. >> Source/WebCore/rendering/InlineTextBox.cpp:543 >> + const TextPaintStyle& selectionPaintStyle = args.m_selectionPaintStyle; > > This is messy and tells me we don’t have this quite right. It doesn’t make sense to move all this data en masse from the arguments to local variables better. We should use the arguments directly in the code or use a different pattern. > > It’s silly to use a const bool& or a const int& for this; it's a waste to follow a pointer each time to get back to the argument structure. So if we were doing something like this then we should use const& only where the object is more less expensive to copy than a pointer is. That's why I made them references, so there wasn't "args." littered everywhere. Done.

Created attachment 214319 [details] Patch

Please post a rebased patch; bots can't apply it. (In reply to comment #18) > Created an attachment (id=214319) [details] > Patch

Comment on attachment 214065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214065&action=review >>> Source/WebCore/rendering/InlineTextBox.cpp:543 >>> + const TextPaintStyle& selectionPaintStyle = args.m_selectionPaintStyle; >> >> This is messy and tells me we don’t have this quite right. It doesn’t make sense to move all this data en masse from the arguments to local variables better. We should use the arguments directly in the code or use a different pattern. >> >> It’s silly to use a const bool& or a const int& for this; it's a waste to follow a pointer each time to get back to the argument structure. So if we were doing something like this then we should use const& only where the object is more less expensive to copy than a pointer is. > > That's why I made them references, so there wasn't "args." littered everywhere. > > Done. Obsolete in the new patch, but I don’t understand the “why I made them references” comment. I believe that const bool& x = args.x will always perform more poorly than bool x = args.x and has no benefits unless we want to track changes to args.x. But perhaps I am mistaken.

Darin: I believe you're right. My comment was only about scoping.

Created attachment 214375 [details] Patch

Comment on attachment 214375 [details] Patch Attachment 214375 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/4110288

Comment on attachment 214375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214375&action=review Looks like you just missed the "AllInOne" file. r- to correct that. > Source/JavaScriptCore/ChangeLog:8 > + * Configurations/FeatureDefines.xcconfig: What's this all about? Looks like a leftover ChangeLog > Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:11634 > + <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='Production|x64'">true</ExcludedFromBuild> If you mark it as "don't build" here, you need to add it to "RenderingAllInOne.cpp"

Created attachment 214383 [details] Patch

Comment on attachment 214375 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214375&action=review > Source/WebCore/ChangeLog:27 > + This patch is large enough that everything below ought to have description of what's going on. The above overview is good, but we need more detail below. Also, the caveats you place as comments in the code ought to be in the Changelog instead, since for the most part it is obvious from the code that only "ink" is being handled. > Source/WebCore/css/CSSParser.h:286 > +#endif // CSS3_TEXT Comment unneeded. > LayoutTests/ChangeLog:11 > + * fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-roundtrip-multiple-expected.txt: Added. Needs description was the test is for. > LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-skip-ink-zoomed-expected.html:23 > + aliasing near the character The description should be clarified a bit, either here or in the Changelog, that because the character is being blown up, the underline and moat are scaled appropriately as well, thus proper skip:ink impl means we would see no underline. So the expected results would be the same as if it were not underlined at all.

Comment on attachment 214383 [details] Patch 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-decoration-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.

Comment on attachment 214383 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=214383&action=review >> Source/WebCore/rendering/InlineTextBox.cpp:1009 >> + context->save(); > > Look at GraphicsContextStateSaver. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1017 >> + OwnPtr<ImageBuffer> imageBuffer = context->createCompatibleBuffer(enclosingDeviceRect.size()); > > Does this do the right thing in rotated contexts? Yes, though we've discussed how to change this. >> 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? We discussed this offline. >> Source/WebCore/rendering/InlineTextBox.cpp:1031 >> + 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. We talked about this offline. >> Source/WebCore/rendering/TextPainter.cpp:58 >> + bool opaque = fillColor.alpha() == 255; > > color.hasAlpha() This is cut-and-paste, but I can change that. Done. >> Source/WebCore/rendering/TextPainter.cpp:73 >> + context->drawEmphasisMarks(font, textRun, emphasisMark, textOrigin + extraOffset + IntSize(0, emphasisMarkOffset), startOffset, endOffset); > > This pattern occurs three times. Move it into a function? This is cut-and-paste, but I can change that. Done. >> Source/WebCore/rendering/TextPainter.h:56 >> + const FloatRect& boxRect() { return m_boxRect; } > > Should be a const function. Good catch. Done. >> 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? The implementation paints into the given GraphicsContext. Done. >> Source/WebCore/rendering/TextPainter.h:65 >> + int m_ePos; > > Names are too mysterious. Done. >> Source/WebCore/rendering/TextPainter.h:72 >> + int m_emphasisMarkOffset; > > Why not a float? This was cut-and-paste. I didn't want to change any semantics. >> Source/WebCore/rendering/TextPainter.h:73 >> + bool m_horizontal; > > What is horizontal? The InlineTextBox (InlineTextBox::IsHorizontal). I'll change the variable name to be clearer. Done. >> LayoutTests/fast/css3-text/css3-text-decoration/text-decoration-skip/text-decoration-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". Done. >> 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. See the line above it

Patch doesn't redraw when the CSS property is changed or removed

Created attachment 215747 [details] Patch

Comment on attachment 215747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215747&action=review > Source/WebCore/rendering/InlineTextBox.cpp:783 > + return inlineTextBox->logicalHeight() + gap + max<float>(offset, 0); I think we explicitly do std:max nowadays.

Comment on attachment 215747 [details] Patch 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.

Comment on attachment 215747 [details] Patch 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. Done. >> Source/WebCore/ChangeLog:14 >> + 6. Un-apply the mask > > One does not really un-apply a mask. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:783 >> + return inlineTextBox->logicalHeight() + gap + max<float>(offset, 0); > > I think we explicitly do std:max nowadays. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:814 >> +static float getWavyStrokeStep(float strokeThickness) > > We only use getFoo for functions with out params. Done. >> 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. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:940 >> + 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. Done. >> 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. Done. >> 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. Done >> Source/WebCore/rendering/InlineTextBox.cpp:963 >> + 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. I believe in this function, unite is okay. >> Source/WebCore/rendering/InlineTextBox.cpp:1045 >> + TextDecorationSkip textDecorationSkip = lineStyle.textDecorationSkip(); > > I, like Darin before me, don't see the need for a separate variable. I misunderstood Darin's comment. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1051 >> + const float skipInkGapWidth = 1.f; > > No need for .f Done. >> 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. Done.

Created attachment 215761 [details] Patch

Comment on attachment 215761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215761&action=review > Source/WebCore/rendering/InlineTextBox.cpp:809 > +static void populateWavyStrokeParameters(float strokeThickness, float& controlPointDistance, float& step) It's OK to use get now because you have out params.

Created attachment 215763 [details] Patch

Comment on attachment 215763 [details] Patch 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.

Comment on attachment 215763 [details] Patch 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 Done. >> Source/WebCore/rendering/InlineTextBox.cpp:821 >> + step = 2 * max<float>(2, strokeThickness); > > std::max Done. >> Source/WebCore/rendering/InlineTextBox.cpp:923 >> + */ > > Should be a “//“ comment, not a “/* */“ comment. > > Isn’t it normally called strikethrough rather than strokethrough? Done. Typo. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:926 >> + 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). Done. >> 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. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:950 >> + 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. Done. >> 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. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:960 >> + const int underlineOffset = computeUnderlineOffset(underlinePosition, lineStyle.fontMetrics(), &inlineTextBox, textDecorationThickness); > > Remove the const here. Done. >> Source/WebCore/rendering/InlineTextBox.cpp:1063 >> + GraphicsContext* maskContext = imageBuffer->context(); > > Should use a reference instead of pointer here since this is never null. Done. >> 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. Done.

Created attachment 215771 [details] Patch

Comment on attachment 215771 [details] Patch Attachment 215771 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/19558107

Comment on attachment 215771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215771&action=review > Source/WebCore/rendering/InlineTextBox.cpp:1098 > context->drawLineForText(FloatPoint(localOrigin.x(), localOrigin.y() + baseline + 1), width, isPrinting); Missed the context-> here, otherwise looks ready to go.

%*@)*%#@ Xcode! It compiled, I swear! Patch incoming

Created attachment 215774 [details] Patch

Comment on attachment 215774 [details] Patch Clearing flags on attachment: 215774 Committed r158467: <http://trac.webkit.org/changeset/158467>

All reviewed patches have been landed. Closing bug.

Comment on attachment 215774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215774&action=review > Source/WebCore/rendering/InlineTextBox.cpp:926 > +static FloatRect boundingBoxForSingleDecoration(GraphicsContext& context, float textDecorationThickness, > + float width, FloatPoint localOrigin, TextDecorationStyle decorationStyle, > + bool isPrinting, float yOffset, float underlineOffset, float doubleOffset) > +{ It is bit sad that all this code is added to already InlineTextBox. The simple line path should be able to share the same text painting code. Maybe some of this could be factored out?