Bug 86463 – [Qt] Enable SVG Fonts by default

• Component: New Bugs
• Status: RESOLVED FIXED

[Pierre Rossi]

[Qt] Enable SVG Fonts by default

[Pierre Rossi]

Created attachment 141920 [details]
Patch

[Pierre Rossi]

Comment on attachment 141920 [details]
Patch

Since the tests have been skipped for a while, they will most likely need to be rebaselined. I'm just not sure I can trus what I get locally once again.

[Csaba Osztrogonác]

Created attachment 141943 [details]
new results with the patch

Here is the new results with your patch.
Feel free to land with the original patch.

[Simon Hausmann]

Comment on attachment 141920 [details]
Patch

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

> Source/WebCore/platform/graphics/qt/FontPlatformData.h:59
> +// Necessary for SVG Fonts, which are only supported when using QRawFont.

I suppose the comment should be indented and maybe also explain why it is necessary - like you do in the ChangeLog :)

> Source/WebCore/svg/SVGFontElement.cpp:137
> -                ligatures.append(unicode);
> +                ligatures.append(unicode.string());

Why is this needed?

[Pierre Rossi]

(In reply to comment #3)
> Created an attachment (id=141943) [details]
> new results with the patch
> 
> Here is the new results with your patch.
> Feel free to land with the original patch.

Thanks Ossy ! :)

[Pierre Rossi]

(In reply to comment #4)
> (From update of attachment 141920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141920&action=review
> 
> > Source/WebCore/platform/graphics/qt/FontPlatformData.h:59
> > +// Necessary for SVG Fonts, which are only supported when using QRawFont.
> 
> I suppose the comment should be indented and maybe also explain why it is necessary - like you do in the ChangeLog :)
> 
> > Source/WebCore/svg/SVGFontElement.cpp:137
> > -                ligatures.append(unicode);
> > +                ligatures.append(unicode.string());
> 
> Why is this needed?

I had an ambiguity warning with String operator I think. that seemed legit but maybe it's just hiding another problem, I'll look again.

[Pierre Rossi]

Created attachment 142028 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 142028 [details]
Patch

There are more things to do here. Grep for PLATFORM(QT) in svg/ and rendering/svg, you'll find some places related to SVG Fonts support, where we currently have special cases for Qt, as it's the only code path yet not using the simple code path & GlyphPage concept.

[Pierre Rossi]

Created attachment 142197 [details]
Patch

[Pierre Rossi]

(In reply to comment #8)
> (From update of attachment 142028 [details])
> There are more things to do here. Grep for PLATFORM(QT) in svg/ and rendering/svg, you'll find some places related to SVG Fonts support, where we currently have special cases for Qt, as it's the only code path yet not using the simple code path & GlyphPage concept.

Thanks for taking a look at this.
Are those the ones you meant ? There are also two similar cases I noticed in CanvasRenderingContext2D.cpp, but I don't think it belongs with that patch really.

[Nikolas Zimmermann]

Comment on attachment 142197 [details]
Patch

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

> Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:51
> +#if PLATFORM(QT) && !HAVE(QRAWFONT)

Exactly, without these changes you shouldn't see SVG Fonts, even when QRawFont is enabled.
Did you verify they work now with Qt5+QRawFont?

[Pierre Rossi]

(In reply to comment #11)
> (From update of attachment 142197 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142197&action=review
> 
> > Source/WebCore/rendering/svg/SVGTextMetricsBuilder.cpp:51
> > +#if PLATFORM(QT) && !HAVE(QRAWFONT)
> 
> Exactly, without these changes you shouldn't see SVG Fonts, even when QRawFont is enabled.
> Did you verify they work now with Qt5+QRawFont?

Actually I'm not sure to follow you: AFAICT these changes only ensure we use the simple path over the complex one with respect to metrics. Isn't it like for plain font stuff: we can go through the complex path for anything, it just might not be the best thing to do. 
The testing I had done, albeit in release, showed things worked pretty well even without them. I take it some metrics calculations were done using the complex code path, but the integration with GlyphPageTree and the like was still used afterwards.

[Nikolas Zimmermann]

(In reply to comment #12)
> Actually I'm not sure to follow you: AFAICT these changes only ensure we use the simple path over the complex one with respect to metrics. Isn't it like for plain font stuff: we can go through the complex path for anything, it just might not be the best thing to do. 
We've discussed this on IRC. Kerning pairs in SVG Fonts will now be properly respected, by the change, so its good nonetheless :-)

[Nikolas Zimmermann]

Comment on attachment 142197 [details]
Patch

r=me.

[Pierre Rossi]

Committed r117259: <http://trac.webkit.org/changeset/117259>