[webkit-reviews] review denied: [Bug 38217] [chromium] Add WOFF support : [Attachment 54456] patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Adam Langley <agl at chromium.org>'s
request for review:
Bug 38217: [chromium] Add WOFF support
https://bugs.webkit.org/show_bug.cgi?id=38217

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

------- Additional Comments from David Levin <levin at chromium.org>

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


> diff --git a/LayoutTests/fast/css/resources/DroidSerif-Regular.woff
b/LayoutTests/fast/css/resources/DroidSerif-Regular.woff

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

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> +2010-04-27  Adam Langley  <agl at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   This patch adds support for WOFF in Chromium. Since Chromium
> +	   already transcodes all OpenType files for security reasons we
> +	   are adding WOFF support into the transcoder. The WebKit changes
> +	   which remain are:
> +	     - To recognise "woff" as a font-face format value (guarded by
> +	       PLATFORM(CHROMIUM) at this point)
> +	     - To change the transcoder glue file so that the transcoded
> +	       font can be larger than the original. (WOFF files are
> +	       compressed, so the transcoded TTF is typically larger.)

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


> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=38217
> +
> +	   * css/CSSFontFaceSrcValue.cpp:
> +	   (WebCore::CSSFontFaceSrcValue::isSupportedFormat):
> +	   * platform/graphics/opentype/OpenTypeSanitizer.cpp:
> +	   (WebCore::OpenTypeSanitizer::sanitize):
> +



> diff --git a/WebCore/platform/graphics/opentype/OpenTypeSanitizer.cpp
b/WebCore/platform/graphics/opentype/OpenTypeSanitizer.cpp
> +    return SharedBuffer::create(static_cast<unsigned
char*>(output.Release()), transcodeLen);

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?)