[Webkit-unassigned] [Bug 81326] Vertical flow support for OpenType fonts with the least platform dependencies

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from mitz at webkit.org  2012-03-17 15:14:39 PST ---
(From update of attachment 132470)
View in context: https://bugs.webkit.org/attachment.cgi?id=132470&action=review

> Source/WebCore/ChangeLog:17
> +        This patch is for any platforms that wants to parse OpenType tables

typo: wants

> Source/WebCore/platform/graphics/FontPlatformData.h:280
> +    void* openTypeTable(uint32_t table, size_t* = 0) const;

Do you foresee any callers who won’t be interested in the size? Are there going to be any callers besides instantiations of the below template? Maybe this can be private and the last parameter be changed into a size_t&.

> Source/WebCore/platform/graphics/FontPlatformData.h:281
> +    template <typename T> PassOwnPtr<T> openTypeTable(uint32_t table, size_t* size = 0) const

Same question about the size. I wonder if this function shouldn’t return a SharedBuffer instead of an OwnPtr and size. One reason is that I’m thinking that on some platforms may vend the table data as a refcounted object (e.g. a CFDataRef) so we wouldn’t want to create an extra copy.

> Source/WebCore/platform/graphics/FontPlatformData.h:288
> +            ASSERT_WITH_MESSAGE(false, "Broken font table ignored, size %d < min %d", *size, sizeof(T));

It’s wrong to make an assertion about something that’s outside the code’s control: a font file may contain malformed tables (and the platform API may not care). You can use LOG_ERROR here.

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:2
> + * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

Is the code in this new file copied form some other file with this copyright? Otherwise, why are you including it?

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:31
> +struct OTInt16 {

I wonder if it would be better to have these in an OpenType namespace (or, since they apply to all sfnt-type containers, a more generic name).

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:42
> +

This is the same as BigEndianUShort from OpenTypeUtilities.cpp (perhaps that’s why you included the Apple copyright?). Are you going to switch that file over to using these types?

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:59
> +#define OT_MAKE_TAG(ch1, ch2, ch3, ch4) ((((uint32_t)(ch4)) << 24) | (((uint32_t)(ch3)) << 16) | (((uint32_t)(ch2)) << 8) | ((uint32_t)(ch1)))

Is this necessary? OpenTypeUtilities.cpp makes us think that all relevant compilers support four-character literals.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:2
> + * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

Same question about copyright.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:131
> +    if (vhea) {

You can reverse the condition and write this as an early return.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:152
> +        if (cVmtxEntries) {

Ditto.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h:2
> + * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

Same question about copyright.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h:46
> +    void getVerticalTranslationsForGlyphs(const SimpleFontData*, const Glyph*, size_t, float* outXYArray) const;

Is there a reason why this is using bare pointers and a count instead of vectors?

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.h:51
> +private:

Extra private: label here.

> Source/WebCore/platform/graphics/win/FontPlatformDataWin.cpp:83
> +    DWORD cb = GetFontData(hdc, table, 0, 0, 0);

Please use a more descriptive name than “cb”.

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