[Webkit-unassigned] [Bug 190255] [GTK][WPE] VDMX vertical font metrics support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=190255

--- Comment #8 from Olivier Blin <olivier.blin at softathome.com> ---
Comment on attachment 351536
  --> https://bugs.webkit.org/attachment.cgi?id=351536
Patch

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

>> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:142
>> +                    descent = -vdmxDescent;
> 
> Presumably these values should only be used in vertical text?

VDMX tables are not for vertical text, but to modify the vertical alignment offsets depending on the font size.

>> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:43
>> +class Buffer {
> 
> It's kind of unfortunate to introduce a new class for this. We have like a hundred different Buffer types already. Are you sure we can't reuse one of those? Or maybe this should be collapsed with the later function to create a "VDMXParsingContext" class

Thanks for challenging this.
I will rework it using OpenTypeTypes helpers, which are exactly made for this.
If this works, then no Google copyright anymore.

>> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:121
>> +               unsigned targetPixelSize)
> 
> Instead of out params, I prefer returning structs. That way you can return an optional of the struct.

I will do this, thanks for the suggestion.

>> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:147
>> +            || !buf.readU8(&yRatio1)
> 
> The spec names this yStartRatio, I suggest matching it.

Right

>> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:186
>> +        if (pixelSize == targetPixelSize) {
> 
> Font sizes should be floats (the fact that they aren't right now is a bug). Seems unfortunate to match based on ==

They are uint16 in the VDMX records, and used to be passed as int in this "old" Google code.
What would be a better way to match font sizes?

>> Source/cmake/OptionsGTK.cmake:87
>> +WEBKIT_OPTION_DEFINE(USE_VDMX_FONT_METRICS "Whether to enable support for VDMX font metrics." PRIVATE OFF)
> 
> I don't want to add a build flag that's off by default on every port.
> 
> I don't think there should be a build flag for this. I would use an experimental features runtime flag. And if the feature is working well already, then hopefully that flag can be removed sooner rather than later.

I agree with this, and realized it too when I thought about enabling tests for VDMX tables.
Thanks

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181004/fd9ed586/attachment.html>