[Webkit-unassigned] [Bug 81389] Vertical alternate glyph (GSUB) support for OpenTypeVerticalData

[bugzilla-daemon at webkit.org]

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


Tony Chang <tony at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #138058|review?                     |review-
               Flag|                            |




--- Comment #16 from Tony Chang <tony at chromium.org>  2012-07-09 15:35:13 PST ---
(From update of attachment 138058)
View in context: https://bugs.webkit.org/attachment.cgi?id=138058&action=review

Is it possible to write a GTest style unittest for this code to verify that different malformed inputs are rejected?  For example, for a chromium build, you could add a test to Source/WebKit/chromium/tests/.

> Source/WebCore/ChangeLog:9
> +        This patch adds support for reading 'GSUB' OpenType table to get
> +        vertical alternate glyphs.

Can you include a link to the OpenType spec about this table?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:51
> +enum {
> +    DefaultScriptTag = OT_MAKE_TAG('D', 'F', 'L', 'T'),
> +};

This doesn't need to be an enum does it?  It could just be a uint32_t.  I'm not sure why the tags above are in an enum either.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:55
> +enum {
> +    VertFeatureTag = OT_MAKE_TAG('v', 'e', 'r', 't'),
> +};

Also doesn't need to be an enum.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:59
> +    static bool isValidEnd(const SharedBuffer& buffer, const void* p)

Please use a longer variable name than |p|.  position would be ok.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:61
> +        size_t offset = reinterpret_cast<const char*>(p) - buffer.data();

Just to be safe should we verify that p >= buffer.data()?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:65
> +    template <typename T> static const T* validatePtr(const SharedBuffer& buffer, const void* p)

Please use a longer variable name than |p|.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:75
> +        return validatePtr<T>(buffer, reinterpret_cast<const int8_t*>(this) + offset);

Is it possible for this + offset to overflow?  I guess if we do the extra check mentioned above for line 61 it would be OK.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:185
> +        switch (lookupType) {
> +        case 1: // Single Substitution Subtable

Do you need a default: for this switch statement?  I think some compilers will complain if it's missing.  If we only care about case 1, maybe we should early return if lookupType != 1?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:194
> +                switch (substitution->substFormat) {
> +                case 2: { // Single Substitution Format 2

Maybe use an if statement here too?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:211
> +                        break; }

Nit: Closing } goes on a new line and lines up with the left side of 'case'.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:219
> +                        for (uint16_t i = 0, indexTo = 0; i < countRange; i++) {

Nit: ++i

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:222
> +                            if (indexTo +  (fromEnd - from) > countTo)

Nit: accidental double space?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:261
> +        for (uint16_t i = 0; i < count; i++) {

Nit: ++i

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:342
> +        for (uint16_t i = 0; i < count; i++) {

Nit: ++i

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:363
> +        const ScriptTable* s = defaultScript(buffer);
> +        if (!s)

Nit: Rename s to scriptTable

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:400
> +        const FeatureTable* vert = feature(OpenType::VertFeatureTag, buffer);
> +        const LookupList* lookups = lookupList(buffer);
> +        return vert && lookups && vert->getGlyphSubstitutions(lookups, map, buffer);

Nit: If vert is null, I would early return before calling lookupList.  Also, vert should be verticalFeatureTable or something rather than an abbreviation.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.