Bug 125718 – Faster implementation of text-decoration-skip: ink

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

[Myles C. Maxfield]

Faster implementation of text-decoration-skip: ink

[Myles C. Maxfield]

Created attachment 219212 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 219212 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Faster implementation of text-decoration-skip: ink

Is it really faster?

[EFL EWS Bot]

Comment on attachment 219212 [details]
Patch

Attachment 219212 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/47508015

[EFL EWS Bot]

Comment on attachment 219212 [details]
Patch

Attachment 219212 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/47478031

[Simon Fraser (smfr)]

Comment on attachment 219212 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.h:104
> +    DashArray getIntersectionPoints(const TextRun&, const FloatPoint& textOrigin, int from, int to, const FloatRect& lineExtents) const;

What are 'from' and 'to'? Offsets, pixels?

> Source/WebCore/platform/graphics/mac/FontMac.mm:333
> +} LMGlyphPathInfo;

Give this a better name.

> Source/WebCore/platform/graphics/mac/FontMac.mm:354
> +    if (e->type == kCGPathElementMoveToPoint) {
> +        info->startingPoint = e->points[0];
> +        info->currentPoint = e->points[0];
> +    } else if (e->type == kCGPathElementAddLineToPoint) {
> +        doIntersection = true;
> +        point = e->points[0];
> +    } else if (e->type == kCGPathElementAddQuadCurveToPoint) {
> +        doIntersection = true;
> +        point = e->points[1];
> +    } else if (e->type == kCGPathElementAddCurveToPoint) {
> +        doIntersection = true;
> +        point = e->points[2];
> +    } else if (e->type == kCGPathElementCloseSubpath) {
> +        doIntersection = true;
> +        point = info->startingPoint;

Can this be a switch?

> Source/WebCore/platform/graphics/mac/FontMac.mm:356
> +    if (doIntersection) {

Early return.

> Source/WebCore/platform/graphics/mac/FontMac.mm:360
> +            if (x < info->minX) info->minX = x;
> +            if (x > info->maxX) info->maxX = x;

Wrap.

> Source/WebCore/platform/graphics/mac/FontMac.mm:365
> +            if (x < info->minX) info->minX = x;
> +            if (x > info->maxX) info->maxX = x;

Wrap.

> Source/WebCore/platform/graphics/mac/FontMac.mm:376
> +    CGAffineTransform translation;

Declare at first use.

> Source/WebCore/platform/graphics/mac/FontMac.mm:384
> +        LMGlyphPathInfo info = {{0.0, 0.0}, {0.0, 0.0}, lineExtents.y(), lineExtents.y() + lineExtents.height(), 1e7, -1e7};

Why not give it a constructor?

> Source/WebCore/platform/graphics/mac/FontMac.mm:385
> +        CGPathRef path = CTFontCreatePathForGlyph(glyphBuffer.fontDataAt(i)->platformData().ctFont(), glyphBuffer.glyphAt(i), &translation);

RetainPtr?

> Source/WebCore/platform/graphics/mac/FontMac.mm:395
> +    return output;  

We normally call this 'result'.

> Source/WebCore/rendering/InlineTextBox.cpp:73
> +static DashArray formatTextIntersectionsForSkipInk(const DashArray& intersections, float dilationAmount, float totalWidth)

format -> adjust?

> Source/WebCore/rendering/InlineTextBox.cpp:80
> +        tuples.append(std::make_pair(*i - dilationAmount, *(i+1) + dilationAmount));

Spaces around +

> Source/WebCore/rendering/InlineTextBox.cpp:91
> +            float& secondStart = i->first;
> +            float& secondEnd = i->second;

Why references?

> Source/WebCore/rendering/InlineTextBox.cpp:106
> +    DashArray output;

result.

[Myles C. Maxfield]

Comment on attachment 219212 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.h:104
>> +    DashArray getIntersectionPoints(const TextRun&, const FloatPoint& textOrigin, int from, int to, const FloatRect& lineExtents) const;
> 
> What are 'from' and 'to'? Offsets, pixels?

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:333
>> +} LMGlyphPathInfo;
> 
> Give this a better name.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:354
>> +        point = info->startingPoint;
> 
> Can this be a switch?

Yep!

>> Source/WebCore/platform/graphics/mac/FontMac.mm:356
>> +    if (doIntersection) {
> 
> Early return.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:360
>> +            if (x > info->maxX) info->maxX = x;
> 
> Wrap.

Done. I think. If I understand you correctly.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:365
>> +            if (x > info->maxX) info->maxX = x;
> 
> Wrap.

Done. I think. If I understand you correctly.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:376
>> +    CGAffineTransform translation;
> 
> Declare at first use.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:384
>> +        LMGlyphPathInfo info = {{0.0, 0.0}, {0.0, 0.0}, lineExtents.y(), lineExtents.y() + lineExtents.height(), 1e7, -1e7};
> 
> Why not give it a constructor?

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:385
>> +        CGPathRef path = CTFontCreatePathForGlyph(glyphBuffer.fontDataAt(i)->platformData().ctFont(), glyphBuffer.glyphAt(i), &translation);
> 
> RetainPtr?

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:395
>> +    return output;  
> 
> We normally call this 'result'.

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:73
>> +static DashArray formatTextIntersectionsForSkipInk(const DashArray& intersections, float dilationAmount, float totalWidth)
> 
> format -> adjust?

I actually don't think that's quite right. The input is the coordinates of the underline intersections, the output is the coordinates of where skip:ink should draw underlines. I've renamed it to something that's hopefully clearer.

>> Source/WebCore/rendering/InlineTextBox.cpp:80
>> +        tuples.append(std::make_pair(*i - dilationAmount, *(i+1) + dilationAmount));
> 
> Spaces around +

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:91
>> +            float& secondEnd = i->second;
> 
> Why references?

firstEnd needs to be, because line 95 writes to it. However, the other two don't need to be. Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:106
>> +    DashArray output;
> 
> result.

Done.

[Myles C. Maxfield]

Created attachment 219233 [details]
Patch

[EFL EWS Bot]

Comment on attachment 219233 [details]
Patch

Attachment 219233 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/49038039

[Sam Weinig]

What is the benchmark being used to determine relative performance? How much faster is this implementation?

[EFL EWS Bot]

Comment on attachment 219233 [details]
Patch

Attachment 219233 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/47178263

[kov's GTK+ EWS bot]

Comment on attachment 219233 [details]
Patch

Attachment 219233 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/49038046

[Myles C. Maxfield]

Created attachment 219245 [details]
Patch

[EFL EWS Bot]

Comment on attachment 219245 [details]
Patch

Attachment 219245 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/47178297

[Myles C. Maxfield]

Looks like someone killed the efl-wk2 bot as it was compiling, causing it to show up as red. I'll resubmit the same patch again to re-trigger that bot.

[Myles C. Maxfield]

Created attachment 219352 [details]
Patch

[Darin Adler]

Comment on attachment 219352 [details]
Patch

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

review- because of the storage leak from forgetting to use adoptCF.

> Source/WebCore/platform/graphics/Font.cpp:317
> +#if !PLATFORM(MAC)
> +DashArray Font::getIntersectionPoints(const TextRun& run, const FloatPoint& textOrigin, int textRunStartIndex, int textRunEndIndex, const FloatRect& lineExtents) const

We shouldn’t do this. This function should just be left out and the call site should also be left out. The only reason this exists is because of the redirection in TextPainter, and if we ifdef that then we’ll be better off.

> Source/WebCore/platform/graphics/Font.cpp:324
> +    UNUSED_PARAM(run);
> +    UNUSED_PARAM(textOrigin);
> +    UNUSED_PARAM(textRunStartIndex);
> +    UNUSED_PARAM(textRunEndIndex);
> +    UNUSED_PARAM(lineExtents);
> +    return DashArray();

I suggest just omitting all the argument names instead of all those UNUSED_PARAM here. But also, don’t we want a compile time error for these other platforms?

> Source/WebCore/platform/graphics/mac/FontMac.mm:326
> +typedef struct _GlyphIterationState {

This should just be:

    struct GlyphIterationState

All that stuff with the leading underline and typedef is C-specific stuff that’s not needed in C++.

> Source/WebCore/platform/graphics/mac/FontMac.mm:327
> +    _GlyphIterationState(CGPoint _startingPoint, CGPoint _currentPoint, CGFloat _y1, CGFloat _y2, CGFloat _minX, CGFloat _maxX)

No need for all these underscores. Everything can just be named normally; no problem having the arguments named the same thing as the members they initialize.

> Source/WebCore/platform/graphics/mac/FontMac.mm:352
> +    GlyphIterationState *info = (GlyphIterationState *)vinfo;

Idiom for our WebKit C++ code would be:

     auto& state = *static_cast<GlyphIterationState*>(stateAsVoidPointer);

Use a reference, not a pointer. Use static_cast, not a C-style cast. Don’t repeat the type name twice. If the type is named state, then the variable should be named “state”, not “info”.

> Source/WebCore/platform/graphics/mac/FontMac.mm:391
> +DashArray Font::getIntersectionPoints(const TextRun& run, const FloatPoint& textOrigin, int textRunStartIndex, int textRunEndIndex, const FloatRect& lineExtents) const

This should be named intersectionPoints. We use "get" in function names only when they use out arguments. Functions with return values use noun phrases that describe the return value.

> Source/WebCore/platform/graphics/mac/FontMac.mm:402
> +    for (int i = 0; i < glyphBuffer.size(); i++) {

Is int the right type here? Also, normally we do ++i.

> Source/WebCore/platform/graphics/mac/FontMac.mm:404
> +        RetainPtr<CGPathRef> path = CTFontCreatePathForGlyph(glyphBuffer.fontDataAt(i)->platformData().ctFont(), glyphBuffer.glyphAt(i), &translation);

This leaks the path. Need to use adoptCF to avoid leaking.

> Source/WebCore/platform/graphics/mac/FontMac.mm:405
> +        CGPathApply(path.get(), (void *)&info, &findPathIntersections);

There should not be a typecast to (void *) needed here.

> Source/WebCore/rendering/TextPainter.h:71
> +    DashArray getIntersectionPoints(const FloatRect& lineExtents, int start, int end);

Since this is currently Mac-only, I suggest putting this inside some kind of #if. Ideally a feature #if for text-decoration-skip support. Or at least PLATFORM(MAC).

Or maybe we could just provide a getter that returns the Font, and sidestep all the rest of this?

[Myles C. Maxfield]

I'm gathering numbers regarding timing drawing code today. Will be done by COB.
(In reply to comment #9)
> What is the benchmark being used to determine relative performance? How much faster is this implementation?

[Myles C. Maxfield]

(In reply to comment #17)
> I'm gathering numbers regarding timing drawing code today. Will be done by COB.
> (In reply to comment #9)
> > What is the benchmark being used to determine relative performance? How much faster is this implementation?

To test this stuff I timed how long InlineTextBox::paint() takes on average while drawing a full page of text with skip:ink underlines. Numbers normalized to the no-skipping case:

No skipping: 1x (baseline)
This patch: 2.71x
IOSurface-based masking: 48.32x

If you like I can also run PLT, though I'm not sure if that's quite as relevant as the above test.

[Myles C. Maxfield]

Comment on attachment 219352 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.cpp:317
>> +DashArray Font::getIntersectionPoints(const TextRun& run, const FloatPoint& textOrigin, int textRunStartIndex, int textRunEndIndex, const FloatRect& lineExtents) const
> 
> We shouldn’t do this. This function should just be left out and the call site should also be left out. The only reason this exists is because of the redirection in TextPainter, and if we ifdef that then we’ll be better off.

Done.

>> Source/WebCore/platform/graphics/Font.cpp:324
>> +    return DashArray();
> 
> I suggest just omitting all the argument names instead of all those UNUSED_PARAM here. But also, don’t we want a compile time error for these other platforms?

Irrelevant due to above comment

>> Source/WebCore/platform/graphics/mac/FontMac.mm:326
>> +typedef struct _GlyphIterationState {
> 
> This should just be:
> 
>     struct GlyphIterationState
> 
> All that stuff with the leading underline and typedef is C-specific stuff that’s not needed in C++.

Wow. Yeah. Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:327
>> +    _GlyphIterationState(CGPoint _startingPoint, CGPoint _currentPoint, CGFloat _y1, CGFloat _y2, CGFloat _minX, CGFloat _maxX)
> 
> No need for all these underscores. Everything can just be named normally; no problem having the arguments named the same thing as the members they initialize.

Cool; I didn't know that. Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:352
>> +    GlyphIterationState *info = (GlyphIterationState *)vinfo;
> 
> Idiom for our WebKit C++ code would be:
> 
>      auto& state = *static_cast<GlyphIterationState*>(stateAsVoidPointer);
> 
> Use a reference, not a pointer. Use static_cast, not a C-style cast. Don’t repeat the type name twice. If the type is named state, then the variable should be named “state”, not “info”.

Cool. Thanks for these tips. Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:391
>> +DashArray Font::getIntersectionPoints(const TextRun& run, const FloatPoint& textOrigin, int textRunStartIndex, int textRunEndIndex, const FloatRect& lineExtents) const
> 
> This should be named intersectionPoints. We use "get" in function names only when they use out arguments. Functions with return values use noun phrases that describe the return value.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:402
>> +    for (int i = 0; i < glyphBuffer.size(); i++) {
> 
> Is int the right type here? Also, normally we do ++i.

GlyphBuffer::size() returns an int. GlyphBuffer::fontDataAt() takes an int. GlyphBuffer::advanceAt() takes an int as well.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:404
>> +        RetainPtr<CGPathRef> path = CTFontCreatePathForGlyph(glyphBuffer.fontDataAt(i)->platformData().ctFont(), glyphBuffer.glyphAt(i), &translation);
> 
> This leaks the path. Need to use adoptCF to avoid leaking.

Done. This is embarrassing.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:405
>> +        CGPathApply(path.get(), (void *)&info, &findPathIntersections);
> 
> There should not be a typecast to (void *) needed here.

Done.

>> Source/WebCore/rendering/TextPainter.h:71
>> +    DashArray getIntersectionPoints(const FloatRect& lineExtents, int start, int end);
> 
> Since this is currently Mac-only, I suggest putting this inside some kind of #if. Ideally a feature #if for text-decoration-skip support. Or at least PLATFORM(MAC).
> 
> Or maybe we could just provide a getter that returns the Font, and sidestep all the rest of this?

Done.

I'm not quite sure what you mean by "a getter that returns the Font." What do you mean?

[Myles C. Maxfield]

Created attachment 219378 [details]
Patch

[Myles C. Maxfield]

(In reply to comment #18)
> (In reply to comment #17)
> > I'm gathering numbers regarding timing drawing code today. Will be done by COB.
> > (In reply to comment #9)
> > > What is the benchmark being used to determine relative performance? How much faster is this implementation?
> 
> To test this stuff I timed how long InlineTextBox::paint() takes on average while drawing a full page of text with skip:ink underlines. Numbers normalized to the no-skipping case:
> 
> No skipping: 1x (baseline)
> This patch: 2.71x
> IOSurface-based masking: 48.32x
> 
> If you like I can also run PLT, though I'm not sure if that's quite as relevant as the above test.

BTW: 20 page reloads in each of the 3 cases (to make the averages more accurate)

[Sam Weinig]

(In reply to comment #21)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > I'm gathering numbers regarding timing drawing code today. Will be done by COB.
> > > (In reply to comment #9)
> > > > What is the benchmark being used to determine relative performance? How much faster is this implementation?
> > 
> > To test this stuff I timed how long InlineTextBox::paint() takes on average while drawing a full page of text with skip:ink underlines. Numbers normalized to the no-skipping case:
> > 
> > No skipping: 1x (baseline)
> > This patch: 2.71x
> > IOSurface-based masking: 48.32x
> > 
> > If you like I can also run PLT, though I'm not sure if that's quite as relevant as the above test.
> 
> BTW: 20 page reloads in each of the 3 cases (to make the averages more accurate)

That certainly is quite the speedup over the masking approach.  I would like to see PLT numbers, especially if we are considering turning this on by default.

[EFL EWS Bot]

Comment on attachment 219378 [details]
Patch

Attachment 219378 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/46338205

[Myles C. Maxfield]

Updated metrics for synchronous drawing (so I'm not just measuring how long it takes to put commands into a queue)

No skipping: 1x (baseline)
This patch: 2.19x
IOSurface-based masking: 21.38x

[Myles C. Maxfield]

For completeness: Using unaccelerated IOSurfaces (bitmaps in main memory): 8.78x

[Tim Horton]

(In reply to comment #25)
> For completeness: Using unaccelerated IOSurfaces (bitmaps in main memory): 8.78x

s/IOSurfaces/ImageBuffers/, I hope? There are no unaccelerated IOSurfaces.

[Myles C. Maxfield]

Oops. Yes, Tim. Unaccelerated ImageBuffers.

PLT has the same performance with this patch (and with skip:ink applied to all underlines) as without this patch (the times are within the error bound). The IOSurface-based approach has a PLT slowdown of 4%.

[Darin Adler]

Comment on attachment 219378 [details]
Patch

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

> Source/WebCore/platform/graphics/Font.h:28
> +#include "DashArray.h"

We don’t need to include the DashArray header just to declare a function with a DashArray return type. That can and should be done by just adding a forward declaration to this header.

> Source/WebCore/platform/graphics/Font.h:106
> +#if PLATFORM(MAC)
> +    DashArray intersectionPoints(const TextRun&, const FloatPoint& textOrigin, int from, int to, const FloatRect& lineExtents) const;
> +#endif

I’m not sure everyone else agrees with me, but I prefer to have functions like this be unconditional in the header even when the implementation is not always there. I think it’s tidier to not pollute the header with the conditionals; it does no harm to compile declarations of functions that aren’t defined on a particular platform as long as we don’t try to call the function on platforms where it’s not implemented, the interface itself is platform-independent and we don’t bother compiling an empty function body somewhere.

> Source/WebCore/rendering/InlineTextBox.cpp:132
> +    DashArray intersections;
> +#if PLATFORM(MAC)
> +    intersections = textPainter.intersectionPoints(underlineBoundingBox, m_start, m_start + m_len);
> +#endif
> +    DashArray a = translateIntersectionPointsToSkipInkBoundaries(intersections, underlineBoundingBox.height(), width);

I don’t understand this conditional. Is there a point in compiling the rest of this function if we can’t compute intersection points? Does this function do any good without the intersectionPoints function? If not, then I suggest we put this feature under a more appropriate #if rather than just ENABLE(CSS3_TEXT_DECORATION).

The change log sheds no light on this point.

Generally it’s mixed-up to have the functions needed for “skip ink” to be partly under PLATFORM(MAC) #if and partly under ENABLE(CSS3_TEXT_DECORATION) #if. We should think this through and do something as logical as possible, and as little PLATFORM(MAC)-ish as possible. For example, maybe this should be under a ENABLE(CSS3_TEXT_DECORATION_SKIP_INK) conditional. I’m not saying that conditional needs to be separately configurable on the command line, but rather that we find one place to set it, and then use it elsewhere rather than literally writing PLATFORM(MAC) at each conditional site.

> Source/WebCore/rendering/TextPainter.h:27
> +#include "DashArray.h"

Same thing about not needing an include just to declare a function with a return value.

> Source/WebCore/rendering/TextPainter.h:71
> +#if PLATFORM(MAC)

Same thing about not needing a conditional where a function is declared.

[Myles C. Maxfield]

Comment on attachment 219378 [details]
Patch

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

>> Source/WebCore/platform/graphics/Font.h:28
>> +#include "DashArray.h"
> 
> We don’t need to include the DashArray header just to declare a function with a DashArray return type. That can and should be done by just adding a forward declaration to this header.

DashArray is a typedef, so it can't be forward declared.

>> Source/WebCore/platform/graphics/Font.h:106
>> +#endif
> 
> I’m not sure everyone else agrees with me, but I prefer to have functions like this be unconditional in the header even when the implementation is not always there. I think it’s tidier to not pollute the header with the conditionals; it does no harm to compile declarations of functions that aren’t defined on a particular platform as long as we don’t try to call the function on platforms where it’s not implemented, the interface itself is platform-independent and we don’t bother compiling an empty function body somewhere.

Done.

>> Source/WebCore/rendering/InlineTextBox.cpp:132
>> +    DashArray a = translateIntersectionPointsToSkipInkBoundaries(intersections, underlineBoundingBox.height(), width);
> 
> I don’t understand this conditional. Is there a point in compiling the rest of this function if we can’t compute intersection points? Does this function do any good without the intersectionPoints function? If not, then I suggest we put this feature under a more appropriate #if rather than just ENABLE(CSS3_TEXT_DECORATION).
> 
> The change log sheds no light on this point.
> 
> Generally it’s mixed-up to have the functions needed for “skip ink” to be partly under PLATFORM(MAC) #if and partly under ENABLE(CSS3_TEXT_DECORATION) #if. We should think this through and do something as logical as possible, and as little PLATFORM(MAC)-ish as possible. For example, maybe this should be under a ENABLE(CSS3_TEXT_DECORATION_SKIP_INK) conditional. I’m not saying that conditional needs to be separately configurable on the command line, but rather that we find one place to set it, and then use it elsewhere rather than literally writing PLATFORM(MAC) at each conditional site.

Good idea. Done.

>> Source/WebCore/rendering/TextPainter.h:27
>> +#include "DashArray.h"
> 
> Same thing about not needing an include just to declare a function with a return value.

DashArray is a typedef, so it can't be forward declared.

>> Source/WebCore/rendering/TextPainter.h:71
>> +#if PLATFORM(MAC)
> 
> Same thing about not needing a conditional where a function is declared.

Done.

[Myles C. Maxfield]

Created attachment 219450 [details]
Patch

[Myles C. Maxfield]

Darin already r+'ed this, and I have addressed his comments. Is it possible to get a cq+?

[Simon Fraser (smfr)]

Comment on attachment 219450 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontMac.mm:350
> +static bool findIntersectionPoint(float y, CGPoint p1, CGPoint p2, CGFloat& x)
> +{
> +    x = p1.x + (y - p1.y) * (p2.x - p1.x) / (p2.y - p1.y);
> +    return (p1.y < y && p2.y > y) || (p1.y > y && p2.y < y);
> +}

I wish we had a GeometryUtilities.h :\

> Source/WebCore/platform/graphics/mac/FontMac.mm:352
> +static void findPathIntersections(void* stateAsVoidPointer, const CGPathElement* e)

I would like to see a comment saying what this function does. It's very hard to figure out.

> Source/WebCore/platform/graphics/mac/FontMac.mm:393
> +DashArray Font::intersectionPoints(const TextRun& run, const FloatPoint& textOrigin, int textRunStartIndex, int textRunEndIndex, const FloatRect& lineExtents) const

"intersectionPoints": intersection with what?

> Source/WebCore/platform/graphics/mac/FontMac.mm:405
> +        GlyphIterationState info = GlyphIterationState(CGPointMake(0, 0), CGPointMake(0, 0), lineExtents.y(), lineExtents.y() + lineExtents.height(), 1e7, -1e7);

1e7, -1e7? Don't we have FLOAT_MAX or CGFLOATMAX you can use?

[Tim Horton]

Comment on attachment 219450 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/FontMac.mm:350
>> +}
> 
> I wish we had a GeometryUtilities.h :\

There's a lot of code like this in FloatPoint.cpp. Though more verbose and readable.

[Myles C. Maxfield]

Comment on attachment 219450 [details]
Patch

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

>> Source/WebCore/platform/graphics/mac/FontMac.mm:352
>> +static void findPathIntersections(void* stateAsVoidPointer, const CGPathElement* e)
> 
> I would like to see a comment saying what this function does. It's very hard to figure out.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:393
>> +DashArray Font::intersectionPoints(const TextRun& run, const FloatPoint& textOrigin, int textRunStartIndex, int textRunEndIndex, const FloatRect& lineExtents) const
> 
> "intersectionPoints": intersection with what?

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:405
>> +        GlyphIterationState info = GlyphIterationState(CGPointMake(0, 0), CGPointMake(0, 0), lineExtents.y(), lineExtents.y() + lineExtents.height(), 1e7, -1e7);
> 
> 1e7, -1e7? Don't we have FLOAT_MAX or CGFLOATMAX you can use?

Done.

[Myles C. Maxfield]

http://trac.webkit.org/changeset/160951

[Myles C. Maxfield]

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