| Summary: | Vertical flow support for OpenType fonts with the least platform dependencies | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Koji Ishii <kojii> |
| Component: | Text | Assignee: | Nobody <webkit-unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | haraken, hyatt, kojii, mitz, webkit.review.bot |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | PC | ||
| OS: | Windows 7 | ||
| Bug Depends on: | |||
| Bug Blocks: | 81332, 81389 | ||
| Attachments: | |||
|
Description
Koji Ishii
2012-03-16 02:49:27 PDT
Created attachment 132247 [details]
OpenTypeVerticalData class and its related changes
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 Created attachment 132451 [details] OpenTypeVerticalData class and its related changes Self-review; name cleanup and moved more code to bug 81389 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”. (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! (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? 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.
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.
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. 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.
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" 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.
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.
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> |