[webkit-reviews] review denied: [Bug 81326] Vertical flow support for OpenType fonts with the least platform dependencies : [Attachment 133975] OpenTypeVerticalData class and its related changes after mitz's review

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Koji Ishii <kojiishi at gmail.com>'s request for
review:
Bug 81326: Vertical flow support for OpenType fonts with the least platform
dependencies
https://bugs.webkit.org/show_bug.cgi?id=81326

Attachment 133975: OpenTypeVerticalData class and its related changes after
mitz's review
https://bugs.webkit.org/attachment.cgi?id=133975&action=review

------- Additional Comments from mitz at webkit.org
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.