Bug 151962 – Refactor TextPainter::paintText() into sub methods.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[alan]

TextPainter::paintText is way more complicated than it should be.

[alan]

Created attachment 266807 [details]
Patch

[alan]

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);
}

[Myles C. Maxfield]

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

[alan]

Created attachment 266810 [details]
Patch

[alan]

(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.

[WebKit Commit Bot]

Comment on attachment 266810 [details]
Patch

Clearing flags on attachment: 266810

Committed r193656: <http://trac.webkit.org/changeset/193656>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[alan]

(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