[webkit-reviews] review denied: [Bug 31302] Add WOFF support for @font-face : [Attachment 62916] Add woffToSfnt() and use it on Mac and Windows

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied mitz at webkit.org's request for review:
Bug 31302: Add WOFF support for @font-face
https://bugs.webkit.org/show_bug.cgi?id=31302

Attachment 62916: Add woffToSfnt() and use it on Mac and Windows
https://bugs.webkit.org/attachment.cgi?id=62916&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    while (searchRange < numTables) {
> +	   entrySelector++;
> +	   searchRange <<= 1;
> +    }

If numTables is a the maximum value, this will be an infinite loop.

> +    searchRange <<= 4;

If numTables is a large value, this could overflow.

> +    uint16_t rangeShift = (numTables << 4) - searchRange;

If numTables is a large value, this could overflow.

> +    if ((totalSfntSize - sfnt.size()) / (4 * sizeof(uint32_t)) < numTables)
> +	   return false;

What guarantees that totalSfntSize is >= sfnt.size()? Since numTables is
restricted to 16 bits, you could do this math with multiplication instead of
divison.

> +	   uint32_t tableCompLength;
> +	   if (!readUInt32(woff, offset, tableCompLength))
> +	       return false;

How about "compressed" instead of "comp"?

> +	   uint32_t tableOrigLength;
> +	   if (!readUInt32(woff, offset, tableOrigLength) || tableCompLength >
tableOrigLength)
> +	       return false;

How about "original" instead of "orig"?

> +// Returns false if woff is not a valid WOFF font. Otherwise returns true
and writes
> +// the sfnt payload into sfnt.
> +bool woffToSfnt(SharedBuffer* woff, Vector<uint8_t>& sfnt);

How about a verb? Maybe convertWOFFToSfnt?

> +#if OS(LINUX)
> +class String;
> +#endif

I think you can make the forward declaration unconditional.

> +static void freeSfntData(void*, const void*data, size_t)
> +{
> +    fastFree(const_cast<void*>(data));
> +}

Missing space after the "*" before data.

> +    Vector<uint8_t> sfnt;
> +    RefPtr<SharedBuffer> sfntBuffer;
> +    if (woffToSfnt(buffer, sfnt)) {
> +	   sfntBuffer =
SharedBuffer::adoptVector(*reinterpret_cast<Vector<char>*>(&sfnt));
> +	   buffer = sfntBuffer.get();
> +    }

The reinterpret_cast to a Vector<char>* seems really ugly and dangerous. Is
there any way around this? Perhaps we could just write into a Vector<char>.
When reading, the fact that we don't know if it's signed or unsigned could be
an issue. But when writing, it should not.

review- because you should defend against crazy values for numTables and
totalSfntSize.