[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





--- Comment #8 from Kenichi Ishibashi <bashi at chromium.org>  2012-04-16 16:44:43 PST ---
(From update of attachment 134970)
View in context: https://bugs.webkit.org/attachment.cgi?id=134970&action=review

Drive-by comments. (I'm not a reviewer)

> Source/WebCore/ChangeLog:11
> +        Reviewed by NOBODY (OOPS!).

nit: We usually put "Reviewed by" line before the description.

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

A template function named validatedPtr is already defined in the same file. Using the same name might confuse (The order of the arguments are also inconsistent between them).

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:68
> +        if (!isValidEnd(&casted[1], buffer))

Just checking whether the |p| is in the range of |buffer.data()| to |buffer.data() + buffer.size()| looks optimistic to me. How do you guarantee the casted pointer is valid? Since an attacker can send malformed font as webfont, we should be careful here. (Chromium uses font sanitizing library for webfonts, but not all port use such library).

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:181
> +        uint16_t countSubTable = subTableCount;

Why do we need this substitution?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:186
> +            for (int i = 0; i < countSubTable; ++i) {

int -> uint16_t?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:187
> +                const SubstitutionSubTable* sub = validatedPtr<SubstitutionSubTable>(subTableOffsets[i], buffer);

Please don't use abbreviation variable names (The same applies hereafter).

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:190
> +                const CoverageTable* coverage = sub->coverage(buffer);

This line looks essentially the same semantics of other cast such as L187 and L195. Why only this cast is defined as a function?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:234
> +                    LOG_ERROR("SubstFormat %d not supported", sub->substFormat);

Is LOG_ERROR() appropriate here? Is it unlikely that most fonts have other formats(e.g. Single Substitution Format 1 and Multiple Substitution Format) here?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:240
> +            LOG_ERROR("LookupType %d not supported", lookupType);

Is it unlikely that "lookupType != 1"?

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