Bug 128936 – text-decoration-skip: ink does not skip over SVG fonts

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

text-decoration-skip: ink does not skip over SVG fonts

[Myles C. Maxfield]

Created attachment 224435 [details]
Patch

[Myles C. Maxfield]

Note that this patch introduces a malloc() that did not exist before.

[Myles C. Maxfield]

Created attachment 224437 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/16047679>

[Myles C. Maxfield]

Comment on attachment 224437 [details]
Patch

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

> Source/WebCore/platform/graphics/mac/FontMac.mm:500
> +    const SimpleFontData* fontData = glyphBuffer.fontDataAt(0);

Can possibly use Font::primaryFont()

[Dean Jackson]

Comment on attachment 224437 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Tests: fast/css3-text/css3-text-decoration/text-decoration-skip/helvetica.svg

We definitely can't upload a copyrighted font as a test. You'll have to hand author something that has descenders. On the plus side, you could author something that makes the ref test really easy.

[Myles C. Maxfield]

Created attachment 225331 [details]
Patch

[Darin Adler]

Comment on attachment 225331 [details]
Patch

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(textOrigin.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.

[Myles C. Maxfield]

Comment on attachment 225331 [details]
Patch

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

>> 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().

Done.

>> 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*.

Done.

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

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:445
>> +        fontData = glyphBuffer.fontDataAt(m_index);
> 
> I suggest initialization syntax for this instead of assignment.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:447
>> +        m_translation = CGAffineTransformScale(m_translation, 1, -1);
> 
> I suggest initialization syntax for this instead of assignment.
> 
>     , m_translation(CGAffineTransformScale(CGAffineTransformMakeTranslation(textOrigin.x(), textOrigin.y()), 1, -1))
> 
> Although it might be nice to have some helper functions to make this a little less wordy.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:459
>> +    }
> 
> 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.

Done.

I've changed this to a RefPtr, and a CG-specific constructor.

Done.

>> 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.

I can't use a reference in this case because of line 475. I have to walk along the glyphBuffer and pull out the relevant font data

>> 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.

Done.

>> 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.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:512
>> +    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.

Done.

>> Source/WebCore/platform/graphics/mac/FontMac.mm:515
>> +            || (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.

Done.

>> 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.

Done.

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

Done.

>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:119
>> +        m_glyphOrigin.setY(svgFontData.horizontalOriginY() * scale);
> 
> Why not use construction syntax for these instead of assignment syntax?

Done.

>> 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.

Done.

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

Done.

>> 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.

Done.

>> 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.

Done.

>> 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.

Done.

>> 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.

Done.

>> 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.

Done.

>> 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.

Done.

[Myles C. Maxfield]

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