TextPainter::paintText is way more complicated than it should be.
Created attachment 266807 [details] Patch
This is ::paintText after the change. void TextPainter::paintText() { if (!m_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. bool fullLengthPaint = !m_paintSelectedTextSeparately || m_endPositionInTextRun <= m_startPositionInTextRun; int startOffset = fullLengthPaint ? 0 : m_endPositionInTextRun; int endOffset = fullLengthPaint ? m_length : m_startPositionInTextRun; paintTextWithStyle(startOffset, endOffset, m_textPaintStyle, m_textShadow); } // paint only the text that is selected if ((m_paintSelectedTextOnly || m_paintSelectedTextSeparately) && m_startPositionInTextRun < m_endPositionInTextRun) paintTextWithStyle(m_startPositionInTextRun, m_endPositionInTextRun, m_selectionPaintStyle, m_selectionShadow); }
Comment on attachment 266807 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266807&action=review > Source/WebCore/rendering/TextPainter.cpp:153 > +void TextPainter::paintTextWithEmphasisIfNeeded(int startOffset, int endOffset, const TextPaintStyle& paintStyle, const ShadowData* shadow) I think you should name this "emphasis marks" not "text with emphasis" > Source/WebCore/rendering/TextPainter.cpp:160 > + DEPRECATED_DEFINE_STATIC_LOCAL(TextRun, objectReplacementCharacterTextRun, (StringView(&objectReplacementCharacter, 1))); static NeverDestroyed > Source/WebCore/rendering/TextPainter.cpp:167 > + paintTextWithShadows(m_context, m_combinedText ? m_combinedText->originalFont() : m_font, emphasisMarkTextRun, m_emphasisMark, m_emphasisMarkOffset, startOffset, endOffset, m_length, emphasisMarkTextOrigin, m_boxRect, shadow, paintStyle.strokeWidth > 0, m_textBoxIsHorizontal); Why don't we make paintTextWithShadows a member function so we don't have to pass in all these m_ variables
Created attachment 266810 [details] Patch
(In reply to comment #3) > Comment on attachment 266807 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266807&action=review > > > Source/WebCore/rendering/TextPainter.cpp:153 > > +void TextPainter::paintTextWithEmphasisIfNeeded(int startOffset, int endOffset, const TextPaintStyle& paintStyle, const ShadowData* shadow) > > I think you should name this "emphasis marks" not "text with emphasis" > > > Source/WebCore/rendering/TextPainter.cpp:160 > > + DEPRECATED_DEFINE_STATIC_LOCAL(TextRun, objectReplacementCharacterTextRun, (StringView(&objectReplacementCharacter, 1))); > > static NeverDestroyed > Done. > > Source/WebCore/rendering/TextPainter.cpp:167 > > + paintTextWithShadows(m_context, m_combinedText ? m_combinedText->originalFont() : m_font, emphasisMarkTextRun, m_emphasisMark, m_emphasisMarkOffset, startOffset, endOffset, m_length, emphasisMarkTextOrigin, m_boxRect, shadow, paintStyle.strokeWidth > 0, m_textBoxIsHorizontal); > > Why don't we make paintTextWithShadows a member function so we don't have to > pass in all these m_ variables Follow up patch is coming up.
Comment on attachment 266810 [details] Patch Clearing flags on attachment: 266810 Committed r193656: <http://trac.webkit.org/changeset/193656>
All reviewed patches have been landed. Closing bug.
Comment on attachment 266810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266810&action=review > Source/WebCore/rendering/TextPainter.h:60 > private: > + void paintTextWithStyle(int startOffset, int endOffset, const TextPaintStyle&, const ShadowData*); > + void paintEmphasisMarksIfNeeded(int startOffset, int endOffset, const TextPaintStyle&, const ShadowData*); > + > +private: I don’t think we need to say private: twice.
(In reply to comment #8) > Comment on attachment 266810 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266810&action=review > > > Source/WebCore/rendering/TextPainter.h:60 > > private: > > + void paintTextWithStyle(int startOffset, int endOffset, const TextPaintStyle&, const ShadowData*); > > + void paintEmphasisMarksIfNeeded(int startOffset, int endOffset, const TextPaintStyle&, const ShadowData*); > > + > > +private: > > I don’t think we need to say private: twice. Fixing it in bug 151994