[webkit-reviews] review granted: [Bug 136971] Implement 'vhea', 'vmtx', and 'kern' tables in SVG -> OTF converter : [Attachment 238404] Patch

Sun Sep 21 10:49:14 PDT 2014

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 136971: Implement 'vhea', 'vmtx', and 'kern' tables in SVG -> OTF converter
https://bugs.webkit.org/show_bug.cgi?id=136971

Attachment 238404: Patch
https://bugs.webkit.org/attachment.cgi?id=238404&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238404&action=review

I’m not sure that all this clamping is right.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:101
> +	       return (static_cast<uint32_t>(glyph1) << 16 | glyph2) <
(static_cast<uint32_t>(other.glyph1) << 16 | other.glyph2);

The static_cast here are not needed unless we are compiling for a platform
where “int” and “unsigned” are 16-bit, and that won’t be happening. But also,
there’s no need to combine these glyphs that way. Lets just write this:

    return glyph1 < other.glyph1 || (glyph1 == other.glyph1 && glyph2 <
other.glyph2);

Also, it would be cleaner to have this be a separate compareGlyphs function
rather than having KerningData implement a < operator that doesn’t consider all
of the data.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:142
> +    HashMap<Codepoint, Glyph> m_codepointToIndexMap; // FIXME: There might
be many glyphs that map to a single codepoint

Missing period.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:338
> +	   auto& missingGlyphAdvanceAttribute =
m_missingGlyphElement->fastGetAttribute(SVGNames::horiz_adv_xAttr);
> +	   int value = missingGlyphAdvanceAttribute.toInt(&ok);

No need for the local variable here. Just write this in one line.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:340
> +	       averageAdvance = clampTo<int16_t, int>(value);

Should just be clampTo<int16_t>, it’s not helpful to specify the second time.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:547
> +    int16_t defaultVerticalOriginY =
clampTo<int16_t>(m_fontElement.fastGetAttribute(SVGNames::vert_origin_yAttr).to
Int(&ok));
> +    if (!ok && m_missingGlyphElement)

Do we really need to check "ok"? Is zero a valid value?

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:548
> +	   defaultVerticalOriginY =
clampTo<int16_t>(m_missingGlyphElement->fastGetAttribute(SVGNames::vert_origin_
yAttr).toInt(&ok));

No reason to pass &ok here since we never look at it afterward.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:575
> +    write16(result, clampTo<int16_t>(-static_cast<long>(m_unitsPerEm / 2)));
// Vertical typographic descender

The choice of the type "long" here is unusual. I could see using int or
int32_t, but long doesn’t seem better than either of those two types.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:580
> +    write16(result, clampTo<int16_t, float>(m_advanceHeightMax));
> +    write16(result, clampTo<int16_t, float>(m_unitsPerEm -
m_boundingBox.maxY())); // Minimum top side bearing
> +    write16(result, clampTo<int16_t, float>(m_boundingBox.y())); // Minimum
bottom side bearing
> +    write16(result, clampTo<int16_t, float>(m_unitsPerEm -
m_boundingBox.y())); // Y maximum extent

No need to specify ", float" on all of these clampTo function calls.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:593
> +    for (const auto& glyph : m_glyphs) {

Just auto& is fine.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:595
> +	   write16(result, clampTo<uint16_t, float>(glyph.verticalAdvance));
> +	   write16(result, clampTo<int16_t, float>(m_unitsPerEm -
glyph.boundingBox.maxY())); // top side bearing

No need to specify ", float" on all of these clampTo function calls.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:601
> +    uint16_t roundedNumKerningPairs =
roundDownToPowerOfTwo(kerningData.size());

I really don’t understand why rounding *down* is OK.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:603
> +    uint16_t searchRange = roundedNumKerningPairs * 6;
> +    uint16_t rangeShift = (kerningData.size() - roundedNumKerningPairs) * 6;

No idea where this magic number 6 comes from.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:606
> +    uint16_t entrySelector = 0;
> +    while (roundedNumKerningPairs >>= 1)
> +	   ++entrySelector;

I think it would be nicer to have a helper function for this rather than
writing out the loop in this fashion.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:616
> +    for (auto unicodeRange : unicodeRanges) {

Should be auto&, not auto.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:619
> +	       if (codepoint > std::numeric_limits<Codepoint>::max())
> +		   continue;

Why is this check needed? Is there some danger that the find call below will
find something when passed in a huge number? It’s probably not a good idea to
truncate a code point here, but I don’t understand how such bad values could
get into UnicodeRanges in the first place. Also, 0x0000 and 0xFFFF will both
cause trouble below in the has table code but this doesn’t disallow either of
those.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:620
> +	       auto indexIter = m_codepointToIndexMap.find(codepoint);

I think the word “iterator” would be better than word and a half “index iter”.

What prevents us from passing 0 or the deleted value (usually -1) here?

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:622
> +		   glyphSet.add(indexIter->value);

This is a really inefficient data structure for this kind of thing.
Representing every single value in a range as a separate entry in a HashSet is
super-expensive for large ranges.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:630
> +	   // FIXME: Only support BMP for now

Why? The code to handle any UTF-16 sequence is simple.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:631
> +	   if (codepoint.length() == 1 && codepoint.length() == 1 &&
codepoint.at(0) <= std::numeric_limits<Codepoint>::max()) {

This says codepoint.length() twice.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:632
> +	       auto indexIter = m_codepointToIndexMap.find(codepoint.at(0));

What prevents us from trying to use 0x0000 or 0xFFFF here?

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:639
> +void SVGToOTFFontConverter::addGlyphNames(const HashSet<String>& glyphNames,
HashSet<uint16_t>& glyphSet) const

I’m not sure why we keep the glyph names in a set and then later put them into
a map. It seems wasteful to use indexed data types for both. I suggest using
just a Vector for the names in the kerningPair object.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:641
> +    for (auto glyphName : glyphNames) {

This should be auto&.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:668
> +	       for (auto glyph1 : glyphSet1) {
> +		   for (auto glyph2 : glyphSet2)

Should be auto& instead of auto.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:690
> +	   size_t sizeOfKerningDataTable = 14 + 6 * kerningData.size();

A little strange to put this into a size_t and then write it out as a 16-bit
value without any range checking. I think the code overall is not consistent
about things like clamping and range checking.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:693
> +	       sizeOfKerningDataTable = 0;

Seems like this should be 14.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:714
> +    // Table 2

This copy and paste code isn’t good. There should be a helper function rather
than code that we just repeat twice.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:740
> +    // There are two competing specs for the 'kern' table: one that only
Apple supports and one that everyone supports. Apple's font
> +    // parser claims to support both; however, it requires some padding
bytes at the end if you use the second format. <rdar://problem/18401901>

I don’t think this should have all this commentary. Comment should say “Work
around a bug in Apple's font parser by adding some padding bytes.” The
commentary about two specs for 'kern' table doesn’t seem relevant here.

> Source/WebCore/svg/SVGToOTFFontConversion.cpp:964
> +    for (const auto& glyphElement :
childrenOfType<SVGGlyphElement>(m_fontElement)) {

Should just be auto&, not const auto&.