[Qt] Enable SVG Fonts by default
Created attachment 141920 [details] Patch
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.
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.
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?
(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 ! :)
(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.
Created attachment 142028 [details] Patch
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.
Created attachment 142197 [details] Patch
(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.
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?
(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.
(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 :-)
Comment on attachment 142197 [details] Patch r=me.
Committed r117259: <http://trac.webkit.org/changeset/117259>