Bug 38217 - [chromium] Add WOFF support
: [chromium] Add WOFF support
Status: RESOLVED FIXED
: WebKit
Text
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-04-27 13:58 PST by
Modified: 2010-04-29 10:52 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-27 13:58:52 PST
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 From 2010-04-27 14:05:42 PST -------
Created an attachment (id=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 From 2010-04-27 14:11:05 PST -------
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 From 2010-04-27 14:15:18 PST -------
Created an attachment (id=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 From 2010-04-27 17:14:01 PST -------
(From update of attachment 54456 [details])

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 From 2010-04-27 22:35:06 PST -------
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 From 2010-04-28 07:45:45 PST -------
Created an attachment (id=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 From 2010-04-28 08:13:40 PST -------
(From update of attachment 54563 [details])
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 From 2010-04-29 08:07:23 PST -------
http://trac.webkit.org/changeset/58517 might have broken Chromium Linux Release
------- Comment #9 From 2010-04-29 08:15:21 PST -------
(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 From 2010-04-29 08:20:47 PST -------
(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 From 2010-04-29 08:29:48 PST -------
> 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 From 2010-04-29 10:52:03 PST -------
r58517