Bug 38217 - [chromium] Add WOFF support
: [chromium] Add WOFF support
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Text
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-27 13:58 PDT by Adam Langley
Modified: 2010-04-29 10:52 PDT (History)
7 users (show)

See Also:


Attachments
patch (4.31 KB, patch)
2010-04-27 14:05 PDT, Adam Langley
agl: commit‑queue-
Details | Formatted Diff | Diff
patch (4.40 KB, patch)
2010-04-27 14:15 PDT, Adam Langley
levin: review-
agl: commit‑queue-
Details | Formatted Diff | Diff
patch (8.37 KB, patch)
2010-04-28 07:45 PDT, Adam Langley
levin: review+
agl: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Langley 2010-04-27 13:58:52 PDT
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.)
Comment 1 Adam Langley 2010-04-27 14:05:42 PDT
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.)
Comment 2 Evan Martin 2010-04-27 14:11:05 PDT
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.
Comment 3 Adam Langley 2010-04-27 14:15:18 PDT
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.
Comment 4 David Levin 2010-04-27 17:14:01 PDT
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
> +2010-04-27  Adam Langley  <agl@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?)
Comment 5 David Levin 2010-04-27 22:35:06 PDT
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?
Comment 6 Adam Langley 2010-04-28 07:45:45 PDT
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.
Comment 7 David Levin 2010-04-28 08:13:40 PDT
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.
Comment 8 WebKit Review Bot 2010-04-29 08:07:23 PDT
http://trac.webkit.org/changeset/58517 might have broken Chromium Linux Release
Comment 9 Adam Langley 2010-04-29 08:15:21 PDT
(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)
Comment 10 Dimitri Glazkov (Google) 2010-04-29 08:20:47 PDT
(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?
Comment 11 Adam Langley 2010-04-29 08:29:48 PDT
> 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.)
Comment 12 Adam Langley 2010-04-29 10:52:03 PDT
r58517