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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 62922: Add ConvertWOFFToSfnt() and use it on Mac and Windows
https://bugs.webkit.org/attachment.cgi?id=62922&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    uint32_t totalSfntSize;
> +    if (!readUInt32(woff, offset, totalSfntSize))
> +	   return false;
> +
> +    if (!sfnt.tryReserveCapacity(totalSfntSize))
> +	   return false;

Reading a number from a file and then immediately trying to allocate a buffer
of that size seems too trusting, even if we do use the version of the memory
allocation function that fails if memory is exhausted. Are there any reality
checks we can do on this size that can eliminate pathological cases?

>  #if !defined(BUILDING_ON_TIGER) && !defined(BUILDING_ON_LEOPARD)
> -    RetainPtr<CFDataRef> bufferData(AdoptCF, buffer->createCFData());
> -    RetainPtr<CGDataProviderRef> dataProvider(AdoptCF,
CGDataProviderCreateWithCFData(bufferData.get()));
> +    RetainPtr<CGDataProviderRef> dataProvider;
> +#if !HAVE(WOFF)
> +    Vector<char> sfnt;
> +    if (convertWOFFToSfnt(buffer, sfnt)) {
> +	   size_t sfntSize = sfnt.size();
> +	   dataProvider.adoptCF(CGDataProviderCreateWithData(0,
sfnt.releaseBuffer(), sfntSize, freeSfntData));
> +    } else
> +#endif // !HAVE(WOFF)
> +    {
> +	   RetainPtr<CFDataRef> bufferData(AdoptCF, buffer->createCFData());
> +	  
dataProvider.adoptCF(CGDataProviderCreateWithCFData(bufferData.get()));
> +    }

I think it would be better encapsulation to put this code into a function. For
one thing, the WOFF part could be an early return rather than an else with
braces outside the #if, which would be less subtle. Sure, the lack of
PassRetainPtr gives you a way to blame this all on me, but I still think a
function would be great, since it's just SharedBuffer in and CFDataRef out.

This code treats a non-WOFF font and a malformed WOFF font, including cases of
failure due to memory exhaustion, the same. Is that really the intended
algorithm? If the WOFF fails for any reason, then try to treat it as another
file format? That's OK, I guess, if it's really what you intend.

> +bool convertWOFFToSfnt(SharedBuffer* woff, Vector<char>& sfnt);

Maybe I should have suggested adding a try prefix on this function name. Not
sure that would be an improvement, though. It would be nice some day to figure
out a good naming scheme for "return a boolean to indicate success" functions.

> +    Vector<uint8_t> sfnt;
> +    if (convertWOFFToSfnt(buffer, sfnt)) {

I don't think this will compile any more, because you changed the vector type
at my suggestion.

r=me, but consider improvements too