[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