[webkit-reviews] review denied: [Bug 125718] Faster implementation of text-decoration-skip: ink : [Attachment 219212] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 125718: Faster implementation of text-decoration-skip: ink
https://bugs.webkit.org/show_bug.cgi?id=125718

Attachment 219212: Patch
https://bugs.webkit.org/attachment.cgi?id=219212&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.