[webkit-reviews] review requested: [Bug 38217] [chromium] Add WOFF support : [Attachment 54563] patch

[bugzilla-daemon at webkit.org]

Adam Langley <agl at chromium.org> has asked  for review:
Bug 38217: [chromium] Add WOFF support
https://bugs.webkit.org/show_bug.cgi?id=38217

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

------- Additional Comments from Adam Langley <agl at chromium.org>
PTAL

> It seems like your test needs to added to "Skipped" files so that other
> platforms don't try to run it and fail.

Done. (I didn't know about the Skipped files before, thanks.)

> According to http://www.droidfonts.com/licensing/, the Droid font is Apache
> licensed so it isn't allowed to be checked into WebKit.

I figured that we owned the copyright so could do what we liked with them.
However, it appears that Ascender might have kept the copyrights so I encoded
Ahem as a WOFF and used that.

> These comments would ideally be next to the functions below where the changes

> occur.

Done.

> output.Release() looks like a memory leak here because Release typically
> implies that it lets go of ownership of a buffer but SharedBuffer::create
isn't
> taking ownership of it. Why isn't this a leak? (Should Release have a
different
> name?)

Thanks, that was a leak. I assume that SharedBuffer was taking ownership, but
it's just copying. That's a little unfortunate, but I can't see any way to give
a buffer to a SharedBuffer so I've kept the same design and switched to .get()
instead.

> Should
>   #if PLATFORM(CHROMIUM)
> really be
>   #if ENABLE(OPENTYPE_SANITIZER)

Sure thing. Changed.