Bug 127082 - Draw all underline segments in a particular run in the same call
Summary: Draw all underline segments in a particular run in the same call
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-01-15 21:18 PST by Myles C. Maxfield
Modified: 2014-01-16 16:23 PST (History)
8 users (show)

See Also:

Attachments
Patch (7.41 KB, patch)
2014-01-15 21:24 PST, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (7.44 KB, patch)
2014-01-16 12:15 PST, Myles C. Maxfield 		simon.fraser: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2014-01-15 21:18:38 PST 
Draw all underline segments in a particular run in the same call
Comment 1 Myles C. Maxfield 2014-01-15 21:24:55 PST 
Created attachment 221330 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-01-15 22:18:56 PST 
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?
Comment 3 Myles C. Maxfield 2014-01-16 11:50:14 PST 
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.
Comment 4 Myles C. Maxfield 2014-01-16 12:15:22 PST 
Created attachment 221399 [details]
Patch
Comment 5 Simon Fraser (smfr) 2014-01-16 12:20:33 PST 
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?
Comment 6 Myles C. Maxfield 2014-01-16 12:47:43 PST 
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().
Comment 7 Andy Estes 2014-01-16 16:00:20 PST 
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?
Comment 8 Andy Estes 2014-01-16 16:10:03 PST 
Assuming the answer to my question is yes, I fixed this in http://trac.webkit.org/changeset/162164
Comment 9 Myles C. Maxfield 2014-01-16 16:23:41 PST 
Yes, it should be. Thanks for fixing this for me.