Bug 81326 – Vertical flow support for OpenType fonts with the least platform dependencies

• Component: Text
• Status: RESOLVED FIXED

[Koji Ishii]

Splitting from bug 48459; I looked for how to split a bug without much luck, apologize in advance if I were mistaken.

There are several bug for vertical flow, one for each platform for how a platform gets vertical alternate glyphs and vertical font metrics. Each platform can use its own APIs, but some platform like Windows lacks support for such APIs and therefore it needs to read OpenType tables directly. Doing so gives more controls on its behavior like text-orientation support, and gives consistent behavior among WebKit platforms.

It was discussed briefly at webkit-dev:
https://lists.webkit.org/pipermail/webkit-dev/2012-February/019562.html
https://lists.webkit.org/pipermail/webkit-dev/2012-February/019565.html
https://lists.webkit.org/pipermail/webkit-dev/2012-February/019647.html
and it looks like our preference is to read OpenType tables directly rather than using platform-dependent APIs.

This bug is to track platform-independent part of the fix. I'm thinking to propose splitting into 3 patches:
1. A class that reads all the necessary information from OpenType tables. This includes Windows version of the platfrom support for the class (this bug)
2. Change to WebCore to support the class in platform-independent way.
3. Change to WebCore to use the class for CGWin (bug 48459)

The 2nd bug is not entered yet, I'll do so soon after this bug. I'm also interested in doing platform-dependent part (the last bug) for Chromium-Windows port too once the 1st and the 2nd bugs are set.

[Koji Ishii]

Created attachment 132247 [details]
OpenTypeVerticalData class and its related changes

[Koji Ishii]

Created attachment 132350 [details]
OpenTypeVerticalData class and its related changes

Since the OpenTypeVerticalData class is too large for a patch, I'm splitting GSUB part to bug 81389

[Koji Ishii]

Created attachment 132451 [details]
OpenTypeVerticalData class and its related changes

Self-review; name cleanup and moved more code to bug 81389

[Koji Ishii]

Created attachment 132470 [details]
OpenTypeVerticalData class and its related changes

More cleanup and removed things that are not absolutely necessary to reduce patch size

Comment on attachment 132470 [details]
OpenTypeVerticalData class and its related changes

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”.

[Koji Ishii]

(In reply to comment #5)

Thank you mitz for the prompt review!

> > 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&.

Agreed. I don't foresee any such callers.

> > 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?

I looked for webkit.org and it said I should use WebKit/LICENSE at:
http://www.webkit.org/coding/contributing.html
Should I just remove this line?

> > 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?

I once did that, but then removed to reduce patch size thinking the switch isn't absolutely necessary. Sorry I'm still novice at WebKit, I'll re-do the switch.

> > 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.

You're right. I'll double-check and then make the change.

> > 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?

Are you talking about Glyph or outXYArray? For Glyph, it'll come from GlyphBuffer, which doesn't expose Vector. For outXYArray, there maybe a platform that wants out data as FloatPoint or CGSize in CoreGraphics, so I thought float* is the most flexible for callers. I referred to code in FontMac.mm that calls to CTFontGetVerticalTranslationsForGlyphs(platformData.ctFont(), glyphs, translations.data(), count) and I followed that model. Did I miss something? Again, I'm very new to WebKit, your advice is greatly appreciated.

I'll reflect all other comments as you said in the next patch. Thank you so much again for the review!

[Koji Ishii]

(In reply to comment #5)
> > 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.

I remember this one; I did this because Win32 GetFontData API expects different order from four-character literals, and as you said OpenTypeUtilities and several CG code use the syntax but they're not adopted to many platforms in WebKit as far as I understand. Chrome for instance excludes OpenTypeUtilities, my understanding is -- correct me if I'm wrong -- only CGWin and WinCE use OpenTypeUtilities.

I googled and got an article saying that this is implementation-defined in C99:
http://stackoverflow.com/questions/3960954/c-multicharacter-literal
but the article also says it's used in several libraries including boost. Other articles mention the same for C++0x.

I personally prefer the safe way if the spec says it's undefined, but bootst using it seems to be it's pretty safe, so I'd like to follow what you'd say. I can put conversion before calling Win32 API if necessary.

Can you tell me which you prefer?

[Koji Ishii]

Created attachment 132501 [details]
OpenTypeVerticalData class and its related changes after mitz's review

Changes from the previous patch:
* Fixed typo in ChangeLog.
* Changed openTypeTable() method to return PassRefPtr<SharedBuffer>.
* Changed ASSERT to LOG_ERROR when fonts are broken.
* Removed Copyright...Apple from all new files.
* Moved all OpenType types/structs to OpenType namespace.
* Reverted big-endian struct names to be compatible with OpenTypeUtilities.
* Changed conditions to early returns.
* Removed extra private: label.
* Changed variables names like "cb" and "cSomethign" to be more descriptive.

Questions still remains:
* Is Copyright okay with this change?
* Struct names are now compatible with OpenTypeUtilities but I didn't touch OpenTypeUtilities to share the code yet. Should I do this? In a separate bug?
* Multi-character literals wasn't changed yet. See above comment, and I'll change if that's the preference.
* Haven't change getVerticalTranslationsForGlyphs to use vectors yet. See comment above, and I'll change after I got response.

[Koji Ishii]

Comment on attachment 132501 [details]
OpenTypeVerticalData class and its related changes after mitz's review

Oops, I found my knowledge of "for" loop scope is outdated and that causes a problem on VC 2010/Chromium. I'm fixing it now. Sorry about that.

[Koji Ishii]

Created attachment 133975 [details]
OpenTypeVerticalData class and its related changes after mitz's review

The previous patch had a problem with VC 2010 while porting to Chromium Windows, due to my outdated knowledge on the scope of "for" loop. In this patch, I also changed another ASSERT to LOG_ERROR according to the previous mitz's advice (should not assert due to contents in font files.)

Comment on attachment 133975 [details]
OpenTypeVerticalData class and its related changes after mitz's review

View in context: https://bugs.webkit.org/attachment.cgi?id=133975&action=review

Looks good overall. r- because of the potential out-of-bounds reads. I also suggest that you use a nested namespace.

> Source/WebCore/platform/graphics/opentype/OpenTypeTypes.h:28
> +namespace OpenType {

I’m sorry I wasn’t clear about this when suggesting the namespace. I meant it would be nested in the WebCore namespace. I don’t think we should add top-level namespaces in WebCore.

I also thought that if you introduced a namespace you could drop the OT prefix from most identifiers.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:37
> +using namespace WebCore;

This shouldn’t be necessary if the namespaces are nested.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:139
> +        m_advanceWidths[i] = hmtx->entries[i].advanceWidth;

You should check that hmtx is big enough before reading from it (you can’t rely on countHmtxEntries being correct).

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:163
> +                const OTVORGTable::VertOriginYMetrics& metrics = vorg->vertOriginYMetrics[i];

Same here, you can’t trust the table to have as many entries as promised.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:178
> +        m_advanceHeights[i] = vmtx->entries[i].advanceHeight;

Ditto.

> Source/WebCore/platform/graphics/opentype/OpenTypeVerticalData.cpp:194
> +        m_topSideBearings[i] = vmtx->entries[i].topSideBearing;

And again.

[Koji Ishii]

Created attachment 134390 [details]
OpenTypeVerticalData class and its related changes after mitz's 2nd review

Thank you so much for taking time for the review again.

I now understand what to do with the namespace, sorry for my misunderstanding. I've made OpenType as nested namesapce in WebCore, and also removed all OT prefixes except the macro.

Note that I use OpenType:: specifiers even when unnecessary because type names such as "Int16" looked too common to me. Please let me know if this is annoying.

Also thank you for finding the out-of-bounds reads. They were all fixed now. I added validatedPtr function to get a pointer only when size is valid. It can make sure that type names of reinterpret_cast and sizeof are the same.

I also re-reviewed by myself to see if any other oob reads are left but I didn't find any others.

Hope this be the last review. I'll r? once EWS passed just in case.

[Dave Hyatt]

Comment on attachment 134390 [details]
OpenTypeVerticalData class and its related changes after mitz's 2nd review

View in context: https://bugs.webkit.org/attachment.cgi?id=134390&action=review

Everything looks ok to me. I don't know if Dan has any last comments though.

> Source/WebCore/ChangeLog:12
> +        Currently, WebKit relies on platorm APIs to do the work. However,

Typo. "platorm" should be "platform"

[Dave Hyatt]

Comment on attachment 134390 [details]
OpenTypeVerticalData class and its related changes after mitz's 2nd review

r=me. Dan can chime in if he sees anything to warrant another minus.

[Koji Ishii]

Created attachment 134498 [details]
Fixed two typos in ChangeLog from the previous patch

Huge thank you Dave, I fixed two typos in ChangeLog, but I'm not very clear what to do with this after r+. I set r? and cq? on this patch, apologize in advance if this isn't a good manner.

I double-checked by diff-ing with the previous patch to make sure other changes were not get mixed.

[WebKit Review Bot]

Comment on attachment 134498 [details]
Fixed two typos in ChangeLog from the previous patch

Clearing flags on attachment: 134498

Committed r112642: <http://trac.webkit.org/changeset/112642>