Bug 38217 – [chromium] Add WOFF support

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Adam Langley]

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

[Adam Langley]

Created attachment 54451 [details]
patch

(This patch doesn't build against the Chromium tree until the OTS change in http://codereview.appspot.com/946044 has landed.)

[Evan Martin]

Consider making the test case use a sans-serif as a fallback in the test so it's more obvious when the font fails to load.

[Adam Langley]

Created attachment 54456 [details]
patch

(In reply to comment #2)
> Consider making the test case use a sans-serif as a fallback in the test so
> it's more obvious when the font fails to load.

Good point.

[David Levin]

Comment on attachment 54456 [details]
patch


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
> [phone]  Adam Langley  <[e-mail]>
> +
> +        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?)

[David Levin]

one more issue with the patch:

Should 

#if PLATFORM(CHROMIUM)
            || equalIgnoringCase(m_format, "woff")
#endif

really be

#if ENABLE(OPENTYPE_SANITIZER)
            || equalIgnoringCase(m_format, "woff")
#endif

?

Since if chromium turned off  "OPENTYPE_SANITIZER", then chromium wouldn't have woff support and any port that does enable it would have woff support?

[Adam Langley]

Created attachment 54563 [details]
patch

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.

[David Levin]

Comment on attachment 54563 [details]
patch

Regarding the Skipped files, I'm pretty sure that "LayoutTests/platform/mac/Skipped" is used for all Mac OSX platforms, so there is no need to additionally put it in mac-tiger, etc. Ditto for qt.

[WebKit Review Bot]

http://trac.webkit.org/changeset/58517 might have broken Chromium Linux Release

[Adam Langley]

(In reply to comment #8)
> http://trac.webkit.org/changeset/58517 might have broken Chromium Linux Release

It made all the bots red, but that's because they're hugely behind. (They're building against Chromium r45696. But ToT is 45933 and LKGR is 45931)

[Dimitri Glazkov (Google)]

(In reply to comment #9)
> (In reply to comment #8)
> > http://trac.webkit.org/changeset/58517 might have broken Chromium Linux Release
> 
> It made all the bots red, but that's because they're hugely behind. (They're
> building against Chromium r45696. But ToT is 45933 and LKGR is 45931)

So maybe need to change WebKit/chromium/DEPS to rectify that?

[Adam Langley]

> So maybe need to change WebKit/chromium/DEPS to rectify that?

dglazkov set me straight. Have rolled WebKit/chromium/DEPS to the correct version now. (Sorry guys, new WebKit things that I didn't know about.)

[Adam Langley]

r58517