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

[bugzilla-daemon at webkit.org]

Darin Adler <darin 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 219352: Patch
https://bugs.webkit.org/attachment.cgi?id=219352&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?