[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 #6 from Koji Ishii <kojiishi at gmail.com>  2012-03-17 15:55:39 PST ---
(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!

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