Bug 127082 – Draw all underline segments in a particular run in the same call

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Draw all underline segments in a particular run in the same call

[Myles C. Maxfield]

Created attachment 221330 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 221330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221330&action=review

> Source/WebCore/platform/graphics/GraphicsContext.h:374
> +        void drawLinesForText(const FloatPoint&, DashArray widths, bool printing);

Should DashArray be passed by value like this?

[Myles C. Maxfield]

Comment on attachment 221330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221330&action=review

>> Source/WebCore/platform/graphics/GraphicsContext.h:374
>> +        void drawLinesForText(const FloatPoint&, DashArray widths, bool printing);
> 
> Should DashArray be passed by value like this?

Good point. Fixed.

[Myles C. Maxfield]

Created attachment 221399 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 221399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221399&action=review

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1496
> +    Vector<CGRect> dashBounds;

Inline capacity, and set capacity up front?

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1507
> +    if (fillColor() != strokeColor())

You could avoid doing this comparison twice.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1518
> +    CGContextRef context = platformContext();
> +    CGContextSaveGState(context);

Odd that this is so different. Why save/restore here and not above?

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1525
> +    Vector<CGRect> dashBounds;

Ditto.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1530
> +    if (m_state.shouldUseContextColors)

Is m_state.shouldUseContextColors iOS-specific? What does it mean?

[Myles C. Maxfield]

Comment on attachment 221399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221399&action=review

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1496
>> +    Vector<CGRect> dashBounds;
> 
> Inline capacity, and set capacity up front?

Done.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1507
>> +    if (fillColor() != strokeColor())
> 
> You could avoid doing this comparison twice.

Done.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1518
>> +    CGContextSaveGState(context);
> 
> Odd that this is so different. Why save/restore here and not above?

https://bugs.webkit.org/show_bug.cgi?id=127135

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1525
>> +    Vector<CGRect> dashBounds;
> 
> Ditto.

Done.

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1530
>> +    if (m_state.shouldUseContextColors)
> 
> Is m_state.shouldUseContextColors iOS-specific? What does it mean?

Yes, iOS-specific. It appears to be a flag that, when unset, fills the underline with the fillColor() instead of the strokeColor().

[Andy Estes]

Comment on attachment 221399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221399&action=review

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1523
> +    FloatRect rect = computeLineBoundsAndAntialiasingModeForText(*this, point, width, printing, shouldAntialiasLine, color);

This line breaks the iOS build. Should width be widths.last() instead?

[Andy Estes]

Assuming the answer to my question is yes, I fixed this in http://trac.webkit.org/changeset/162164

[Myles C. Maxfield]

Yes, it should be. Thanks for fixing this for me.