[webkit-reviews] review granted: [Bug 128936] text-decoration-skip: ink does not skip over SVG fonts : [Attachment 225331] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 128936: text-decoration-skip: ink does not skip over SVG fonts
https://bugs.webkit.org/show_bug.cgi?id=128936

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=225331&action=review


I’m going to say review+, but I am not happy with the storage management here.
The use of CGPathCreateMutableCopy without adoptCF is quite non-obvious, and
not the style we want, and the use of raw pointers instead of std::unique_ptr
is something easy to fix that we should definitely address before landing.

> Source/WebCore/platform/graphics/Font.h:82
> +    virtual Path getNextPath() = 0;

In WebKit coding style we don’t name these kinds of functions with the word
“get”. So this would just be nextPath().

> Source/WebCore/platform/graphics/TextRun.h:192
> +	   virtual GlyphToPathTranslator* createGlyphToPathTranslator(const
SimpleFontData*, const GlyphBuffer&, int from, int numGlyphs, const
FloatPoint&) const = 0;

This should return std::unique_ptr, not a raw pointer. Also, I suggest taking a
SimpleFontData& instead of a SimpleFontData*.

> Source/WebCore/platform/graphics/mac/FontMac.mm:439
> +class MacGlyphToPathTranslator : public GlyphToPathTranslator {

Should mark this class final.

> Source/WebCore/platform/graphics/mac/FontMac.mm:445
> +	   fontData = glyphBuffer.fontDataAt(m_index);

I suggest initialization syntax for this instead of assignment.

> Source/WebCore/platform/graphics/mac/FontMac.mm:447
> +	   m_translation = CGAffineTransformMakeTranslation(textOrigin.x(),
textOrigin.y());
> +	   m_translation = CGAffineTransformScale(m_translation, 1, -1);

I suggest initialization syntax for this instead of assignment.

    ,
m_translation(CGAffineTransformScale(CGAffineTransformMakeTranslation(textOrigi
n.x(), textOrigin.y()), 1, -1))

Although it might be nice to have some helper functions to make this a little
less wordy.

> Source/WebCore/platform/graphics/mac/FontMac.mm:459
> +    virtual bool containsMorePaths() override
> +    {
> +	   return m_index != m_glyphBuffer.size();
> +    }
> +    virtual Path getNextPath() override
> +    {
> +	   RetainPtr<CGPathRef> result =
adoptCF(CTFontCreatePathForGlyph(fontData->platformData().ctFont(),
m_glyphBuffer.glyphAt(m_index), &m_translation));
> +	   incrementIndex();
> +	   return CGPathCreateMutableCopy(result.get());
> +    }

Normally it’s not great style to define functions inside the class definition,
since it implicitly marks them inline and makes the class harder to read.

The storage management of the return value CGPath here is not good. I’d like to
see an adoptCF here, but that would require that the Path constructor take a
RetainPtr rather than a raw pointer. It’s bad style to have the “adopt” an
implicit part of the Path constructor, because it makes it hard to read this
code and know whether there is a storage leak here or not.

Also, I would leave these virtual function overrides private. We only want to
call them through the base class, not directly on this class.

> Source/WebCore/platform/graphics/mac/FontMac.mm:479
> +    const SimpleFontData* fontData;

I suggest using a reference here instead of a pointer. This should be named
m_fontData or m_font rather than fontData.

> Source/WebCore/platform/graphics/mac/FontMac.mm:499
> +    // TODO: Handle SVG + non-SVG interleaved runs

We don’t use TODO in WebKit. Either leave it out entirely or use FIXME.

> Source/WebCore/platform/graphics/mac/FontMac.mm:505
> +	   translator.reset(new MacGlyphToPathTranslator(glyphBuffer, origin));


We discourage use of std::unique_ptr::reset. Instead this line of code should
use std::make_unique. Calls to new are deprecated in new WebKit code.

> Source/WebCore/platform/graphics/mac/FontMac.mm:512
> +    int index = 0;
> +    while (translator->containsMorePaths()) {

This would read nicely as a for loop.

    for (int index = 0; translator->containsMorePaths(); ++index)

No need to change it to a while loop.

> Source/WebCore/platform/graphics/mac/FontMac.mm:515
> +	   if ((glyphBuffer.fontDataAt(index)->isSVGFont() && !isSVG)
> +	       || (glyphBuffer.fontDataAt(index) != fontData && isSVG))

This would read better all on one line. But also, this will call fontDataAt
twice in some cases; I suggest checking isSVG first. Maybe there’s some better
way to write this for clarity too, like with a helper function.

> Source/WebCore/platform/graphics/mac/FontMac.mm:516
> +	       break; // The advances will get all messed up if we do anything
other than bail here

Since you capitalized this comment you should also use a period.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:104
> +class SVGGlyphToPathTranslator : public GlyphToPathTranslator {

Should mark this class final.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:119
> +	   m_glyph = glyphBuffer.glyphAt(m_index);
> +	   m_glyphOrigin.setX(svgFontData.horizontalOriginX() * scale);
> +	   m_glyphOrigin.setY(svgFontData.horizontalOriginY() * scale);

Why not use construction syntax for these instead of assignment syntax?

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:128
> +    virtual bool containsMorePaths() override

I would leave these virtual function overrides private. We only want to call
them through the base class, not directly on this class.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:191
> +class DummyGlyphToPathTranslator : public GlyphToPathTranslator {

Should mark this class final.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:193
> +    virtual bool containsMorePaths() override

I would leave these virtual function overrides private. We only want to call
them through the base class, not directly on this class.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:203
> +GlyphToPathTranslator*
SVGTextRunRenderingContext::createGlyphToPathTranslator(const SimpleFontData*
fontData, const GlyphBuffer& glyphBuffer, int from, int numGlyphs, const
FloatPoint& point) const

This should return a std::unique_ptr.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:210
> +	   return new DummyGlyphToPathTranslator();

This should use std::make_unique. Calls to new are deprecated in new WebKit
code.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:218
> +    return new SVGGlyphToPathTranslator(glyphBuffer, point, *svgFontData,
*fontElement, from, numGlyphs, scale, isVerticalText);

This should use std::make_unique. Calls to new are deprecated in new WebKit
code.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:237
> +    std::unique_ptr<GlyphToPathTranslator>
translator(createGlyphToPathTranslator(fontData, glyphBuffer, from, numGlyphs,
point));

I’d suggest using auto here instead of explicit std::unique_ptr.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.h:41
> +    virtual GlyphToPathTranslator* createGlyphToPathTranslator(const
SimpleFontData*, const GlyphBuffer&, int from, int numGlyphs, const
FloatPoint&) const override;

I suggest making this override private instead of public.