[Webkit-unassigned] [Bug 190255] [GTK][WPE] VDMX vertical font metrics support
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Oct 3 14:32:39 PDT 2018
https://bugs.webkit.org/show_bug.cgi?id=190255
--- Comment #3 from Myles C. Maxfield <mmaxfield at apple.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
Please try to make a test font so you can make a test.
> Source/WebCore/ChangeLog:10
> + No new tests (OOPS!).
👎
> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:142
> + ascent = vdmxAscent;
> + descent = -vdmxDescent;
Presumably these values should only be used in vertical text?
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:2
> + * Copyright (c) 2008, 2009, Google Inc. All rights reserved.
?
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:36
> +// For htons/ntohs
> +#include <arpa/inet.h>
Surely there's a better way to flip endianness rather than including a header for network protocols
> 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
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:75
> + bool skip(size_t numBytes)
> + {
> + if (m_offset + numBytes > m_length)
> + return false;
> + m_offset += numBytes;
> + return true;
> + }
> +
> + bool readU8(uint8_t* value)
> + {
> + if (m_offset + sizeof(uint8_t) > m_length)
> + return false;
> + *value = m_buffer[m_offset];
> + m_offset += sizeof(uint8_t);
> + return true;
> + }
> +
> + bool readU16(uint16_t* value)
> + {
> + if (m_offset + sizeof(uint16_t) > m_length)
> + return false;
> + memcpy(value, m_buffer + m_offset, sizeof(uint16_t));
> + *value = ntohs(*value);
> + m_offset += sizeof(uint16_t);
> + return true;
> + }
Returning optional<>s would be more elegant.
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:106
> +namespace WebCore {
Why is Buffer in the global namespace? That is almost surely wrong.
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:121
> +bool parseVDMX(int* yMax, int* yMin,
> + const uint8_t* vdmx, size_t vdmxLength,
> + unsigned targetPixelSize)
Instead of out params, I prefer returning structs. That way you can return an optional of the struct.
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:131
> + // Now we have two tables. Firstly we have @numRatios Ratio records, then a
stray @
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:132
> + // matching array of @numRatios offsets. We save the offset of the beginning
stray @
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:139
> + unsigned desiredRatio = 0xffffffff;
Optional<unsigned>
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:147
> + || !buf.readU8(&yRatio1)
The spec names this yStartRatio, I suggest matching it.
> Source/WebCore/platform/graphics/freetype/VDMXParser.cpp:148
> + || !buf.readU8(&yRatio2))
The spec names this yEndRatio, I suggest matching it.
> 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 ==
--
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/20181003/0d4cffc2/attachment.html>
More information about the webkit-unassigned
mailing list