Move VDMX parsing into the Chromium Linux port. VDMX tables are optional tables in TrueType fonts which contain the exact pixel height of a given font at a given pel size. In order to match Windows font metrics we have to use these numbers. Previously, the parsing was performed in Skia. As part of the merge with upstream Skia, an interface for getting table data from a font has been added to Skia and we're moving the parsing into WebKit.
Created attachment 29367 [details] patch
Comment on attachment 29367 [details] patch This code doesn't seem to follow the Webkit naming and style conventions. Functions should not have an initial uppercase letter and arguments should not be named with lower_case_and_underscores. The "Chromium" prefix on ChromiumVDMXParse is a bit strange.
Heck, sorry. I'm switching between so many coding styles that I forgot about WebKit's lowercase method names. I'll fix.
Created attachment 29376 [details] patch Changed method names to camelCase (my bad). Changed the argument names in the prototype to camalCase too (missed them before). Renamed ChromiumVDMXParse to trueTypeVDMXParse. (Just aiming for the donkey's behind here, I'll take any suggestions as to the correct name for this function.) cheers
Comment on attachment 29376 [details] patch I don't know much about TrueType fonts or VDMX tables. Is there someone on the Chrome team who can review this solution? Dean perhaps? Just nits: > +++ b/WebCore/platform/graphics/chromium/FontPlatformDataLinux.h > + // ------------------------------------------------------------------------- > + // Return Skia's unique id for this font. This encodes both the style and > + // the font's file name so refers to a single face. > + // ------------------------------------------------------------------------- nit: this block comment style is not used very uniformly in our code... it seems a bit jarring to me, but i guess that's just personal preference. consistency across our code would be nice. > +++ b/WebCore/platform/graphics/chromium/FontTrueTypeLinux.cpp nit: We should be using the suffix ChromiumLinux.cpp for linux specific files in the chromium/ directory. We have not been very consistent with this in the Linux specific files, which is unfortunate. Maybe you can at least fix this one that you are adding. > +class Buffer > +{ > + public: nit: please use webkit style, which is: class Buffer { public: Buffer(... > + Buffer(const uint8_t* buffer, size_t length) > + : m_buffer(buffer) > + , m_length(length) > + , m_offset(0) > + { } nit: webkit style would probably either be: Buffer(const uint8_t* buffer, size_t length) : m_buffer(buffer) , m_length(length) , m_offset(0) { } or: Buffer(const uint8_t* buffer, size_t length) : m_buffer(buffer) , m_length(length) , m_offset(0) { } > + bool readU8(uint8_t* value) > + { > + if (m_offset + 1 > m_length) > + return false; > + *value = m_buffer[m_offset]; > + m_offset++; > + return true; > + } > + > + bool readU16(uint16_t* value) > + { > + if (m_offset + 2 > m_length) > + return false; > + memcpy(value, m_buffer + m_offset, sizeof(uint16_t)); > + *value = ntohs(*value); > + m_offset += 2; > + return true; > + } how about using sizeof(*value) or sizeof(uint16_t) and sizeof(uint8_t), respectively, instead of the magic 1 and 2 constants? > + private: > + const uint8_t *const m_buffer; nit: "private:" should not be indented. nit: "const uint8_t* const m_buffer;" > +// Freetype does not parse these tables so we do so here. In the future we > +// might support loading of arbitary fonts. This is not something that one > +// would wish to do, dangerous as it is, so we are careful where we tread. we have code to load arbitrary truetype fonts from the web. maybe we need to worry sooner than later? > +bool trueTypeVDMXParse(int* ymax, int* ymin, > + const uint8_t* vdmx, const size_t vdmxLength, > + const unsigned targetPelSize) nit: yMax and yMin to conform to webkit variable naming conventions nit: please use Pixel instead of Pel since Pel is not that common. > + if (!buf.skip(4) || > + !buf.readU16(&numRatios)) nit: webkit style is to put the "||" on the next line or to just avoid the line break, which is what i would do here. > + if (!buf.skip(1) || > + !buf.readU8(&xRatio) || > + !buf.readU8(&yRatio1) || > + !buf.readU8(&yRatio2)) > + return false; same nit here and below > + > + // This either covers 1:1, or this is the default entry (0, 0, 0) > + if ((xRatio == 1 && yRatio1 <= 1 && yRatio2 >= 1) || > + (xRatio == 0 && yRatio1 == 0 && yRatio2 == 0)) { > + desiredRatio = i; > + break; > + } as ugly as it may be, you are supposed to do: if (foo || bar) { } and perhaps that is why most people just do: if (foo || bar) { despite how long it can make some lines :( > + if (desiredRatio == 0xffffffff) // no ratio found > + return false; uber nits: one space instead of two before "//" > + if (!buf.readU16(&numRecords) || > + !buf.skip(sizeof(uint16_t))) > + return false; ding > + if (!buf.readS16(&tempYMax) || > + !buf.readS16(&tempYMin)) nit: || is wrongly placed > + return true; > + } else { nit: no need for else after return > + if (!buf.skip(2 * sizeof(int16_t))) nit: should be 4 white spaces > +++ b/WebCore/platform/graphics/chromium/FontTrueTypeLinux.h same nit about file naming maybe these files should be named VDMXParser.{h,cpp}? it doesn't seem very linux specific (though you are only using it in the chromium-linux port). > +namespace WebCore { > + bool trueTypeVDMXParse(int* ymax, int* ymin, > + const uint8_t* vdmx, const size_t vdmxLength, > + const unsigned targetPelSize); parseVDMX might be a nicer name? seems like it would be more conventional to list the out parameters last. nit: "const size_t vdmxLength" should just be "size_t vdmxLength". putting const in front of a parameter type that is copy-by-value is extraneous. same goes for targetPelSize. > +} // namespace WebCore nit: one space before "//" > +++ b/WebCore/platform/graphics/chromium/SimpleFontDataLinux.cpp > + uint8_t *vdmxTable = (uint8_t *) malloc(vdmxSize); > + if (vdmxTable > + && SkFontHost::GetTableData(fontID, vdmxTag, 0, vdmxSize, vdmxTable) == vdmxSize > + && trueTypeVDMXParse(&vdmxAscent, &vdmxDescent, vdmxTable, vdmxSize, pelSize)) > + isVDMXValid = true; > + free(vdmxTable); nit: please use fastMalloc and fastFree, which are what webkit uses instead of malloc and free.
Created attachment 29393 [details] patch Have fixed all the comments. Clearly I need to write a cpplint.py for WebKit style! The code is previously from Skia and deanm reviewed it when it went in there. It has only changed in terms of style, not function, so should still be good.
> Have fixed all the comments. Clearly I need to write a cpplint.py for WebKit > style! Yeah, it is painful to switch between coding styles :( > The code is previously from Skia and deanm reviewed it when it went in there. > It has only changed in terms of style, not function, so should still be good. In the future, please note this and include a link to where the code review occurred. Thanks!
Landed as: http://trac.webkit.org/changeset/42548