Bug 16980 – Support external SVG Fonts

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

SVG allows to reference external fonts using the CSS @font-face rule and using svg style with the <font-face> element. WebKit doesn't support it yet.

[Nikolas Zimmermann]

Created attachment 18631 [details]
Initial patch

No regressions. Remarkable small patch for such a nice feature :-)

[Eric Seidel (no email)]

Comment on attachment 18631 [details]
Initial patch

Ok a few issues with this first patch:

Is it possible for m_fontElement not to be reset if a SVGFontFace element was moved out of a <font> element between rebuildFontFace() calls?  I know that's an edge case, but I wouldn't want it to not get reset and us to crash or some other strangeness.

You probably should use KURL for this url parsing:
+                // URL scheme example: "file:///Users/nikoz/WebKit/LayoutTests/svg/W3C-SVG-1.1/resources/ext-TestComic.svg#Font"
+                int nameStart = fontName.find('#');
+                if (nameStart != -1)
+                    fontName = fontName.substring(nameStart + 1, fontName.length() - (nameStart + 1));

Are there obscure cases where we would need to decode the anchor before performing lookup in the remote document?

Blindly adopting nodes from some external document into this one seems like a bad idea.  What if those nodes were <script> nodes?  I'm not sure if scripts in shadow trees run... i would hope not.  It's not entirely clear to me why you'd want to adopt the font element into the document anyway (instead of just holding it in the cache).  What happens when an SVG font gets reused from the cache?  Does it work on reuse?

[Eric Seidel (no email)]

Comment on attachment 18631 [details]
Initial patch

Ok a few issues with this first patch:

Is it possible for m_fontElement not to be reset if a SVGFontFace element was moved out of a <font> element between rebuildFontFace() calls?  I know that's an edge case, but I wouldn't want it to not get reset and us to crash or some other strangeness.

You probably should use KURL for this url parsing:
+                // URL scheme example: "file:///Users/nikoz/WebKit/LayoutTests/svg/W3C-SVG-1.1/resources/ext-TestComic.svg#Font"
+                int nameStart = fontName.find('#');
+                if (nameStart != -1)
+                    fontName = fontName.substring(nameStart + 1, fontName.length() - (nameStart + 1));

Are there obscure cases where we would need to decode the anchor before performing lookup in the remote document?

Blindly adopting nodes from some external document into this one seems like a bad idea.  What if those nodes were <script> nodes?  I'm not sure if scripts in shadow trees run... i would hope not.  It's not entirely clear to me why you'd want to adopt the font element into the document anyway (instead of just holding it in the cache).  What happens when an SVG font gets reused from the cache?  Does it work on reuse?

[Nikolas Zimmermann]

Hey Eric,

thanks for a first review.
 
> Is it possible for m_fontElement not to be reset if a SVGFontFace element was
> moved out of a <font> element between rebuildFontFace() calls?  I know that's
> an edge case, but I wouldn't want it to not get reset and us to crash or some
> other strangeness.
Of course this is a demoniac case, but I agree it should work. The easiest way to fix is to reset m_fontElement to 0, if parentNode()->hasTagName(fontTag) doesn't hold true anymore. I'll fix that.

> You probably should use KURL for this url parsing:
> +                // URL scheme example:
> "file:///Users/nikoz/WebKit/LayoutTests/svg/W3C-SVG-1.1/resources/ext-TestComic.svg#Font"
> +                int nameStart = fontName.find('#');
> +                if (nameStart != -1)
> +                    fontName = fontName.substring(nameStart + 1,
> fontName.length() - (nameStart + 1));
> 
> Are there obscure cases where we would need to decode the anchor before
> performing lookup in the remote document?

a) KURL sucks. It's all DeprecatdString based.
b) KURL does the same.
c) I'll use "String SVGURIReference::getTarget(const String& url)", it does what I want.


> Blindly adopting nodes from some external document into this one seems like a
> bad idea.  What if those nodes were <script> nodes?  I'm not sure if scripts in
> shadow trees run... i would hope not.  It's not entirely clear to me why you'd
> want to adopt the font element into the document anyway (instead of just
> holding it in the cache).  What happens when an SVG font gets reused from the
> cache?  Does it work on reuse?
erm, I'm adopting a <font> element, not more. <scripts> are never run, this is a shadowed tree. You're wondering why I'm releasing the "m_externalSVGDocument" and instead adopt the nodes into our document, and add it there.... that's because CachedFont is resused. I'm just noticing that for custom fonts, the parsed data is never released. So indeed I'm doing too much work. Will fix.

[Nikolas Zimmermann]

Created attachment 18648 [details]
Updated patch

[Oliver Hunt]

Comment on attachment 18648 [details]
Updated patch

Okay this looks fine to me, but i'd like mitz and/or hyatt to review given it touches font-fu.

I'd also like a layout test for svg font in html :D

Comment on attachment 18648 [details]
Updated patch

+    m_externalSVGDocument = 0; // Release document

You should not need to do that.

Why is
+bool CachedFont::ensureSVGFontData()
always returning true?

+RefPtr<SVGElement> CachedFont::extractFontFromSVGData(const String& fontName) const

Not sure why this is returning a RefPtr and using a RefPtr<Node> internally. Using raw pointers seems safe and more efficient. I also don't understand "extract" in the name. I think "getSVGFontByID" is more clear (and then you can omit the parameter name in the declaration).

+FontPlatformData CachedFont::platformDataFromSVGData(float size, bool bold, bool italic)

Is that function really necessary?

[Nikolas Zimmermann]

(In reply to comment #7)
> (From update of attachment 18648 [details] [edit])
> +    m_externalSVGDocument = 0; // Release document
> 
> You should not need to do that.
Right, fixed.

> 
> Why is
> +bool CachedFont::ensureSVGFontData()
> always returning true?
Mistake, fixed.
 
> +RefPtr<SVGElement> CachedFont::extractFontFromSVGData(const String& fontName)
> const
> 
> Not sure why this is returning a RefPtr and using a RefPtr<Node> internally.
> Using raw pointers seems safe and more efficient. I also don't understand
> "extract" in the name. I think "getSVGFontByID" is more clear (and then you can
> omit the parameter name in the declaration).
Two good ideas, fixed.
 
> +FontPlatformData CachedFont::platformDataFromSVGData(float size, bool bold,
> bool italic)
> 
> Is that function really necessary?
Nope, integrated within platformDataFromCustomData.
Uploading new patch.

[Nikolas Zimmermann]

Created attachment 18726 [details]
Updated patch

Incorporated Dan's comments.

Comment on attachment 18726 [details]
Updated patch

+        RefPtr<Node> node = list->item(i);

I still don't see why you need to ref and deref each node here.

[Nikolas Zimmermann]

(In reply to comment #10)
> (From update of attachment 18726 [details] [edit])
> +        RefPtr<Node> node = list->item(i);
> 
> I still don't see why you need to ref and deref each node here.
Leftover from times where I had to clone these nodes. Fixed.
Won't upload a patch though as it's just s/RefPtr<Node>/Node*/

Commitable now?

Greetings,
Niko

Comment on attachment 18726 [details]
Updated patch

(In reply to comment #11)
> Commitable now?

Two other issues that I have noticed:

+#include "CString.h"

Is that needed?

+    return m_fontData->fontPlatformData((int) ceilf(size), bold, italic);

Should be a static_cast.

Oliver or Eric should review the SVG code in the patch.

[Nikolas Zimmermann]

(In reply to comment #12)
> (From update of attachment 18726 [details] [edit])
> (In reply to comment #11)
> > Commitable now?
> 
> Two other issues that I have noticed:
> 
> +#include "CString.h"
> 
> Is that needed?
Leftover from debugging, already removed it locally.
 
> +    return m_fontData->fontPlatformData((int) ceilf(size), bold, italic);
> 
> Should be a static_cast.
Okay. Changing.

Do I need to upload a new final patch, or is it okay as is?

[Nikolas Zimmermann]

Created attachment 18742 [details]
Final patch

Decided to upload a final version.

[Eric Seidel (no email)]

Comment on attachment 18742 [details]
Final patch

Ok, this looks great.  We talked about making getSVGFontById actually return an SVGFontElement (I think you've already done that locally).

I also think that we need some http tests for external fonts.  We could just file another bug about those, but it would be nice to land one or two via this patch.  Things to test:

1.  When a font is sent with the wrong mime type, do we handle it? (we currently ignore the mimetype and parse it anyway)
2.  When the font has a 404 doe we use it anyway (currently we have this bug throughout other parts of WebKit, it's OK not to fix this, but we should at least file a bug about it).
3.  If the font has an XML parse error what happens?  If the error is encountered after the <font> declaration, do we still treat the font as valid?  (I believe we do with your current code).

Those 3 need at least bugs, and preferably test cases.

Otherwise this looks totally ready to go.

[Eric Seidel (no email)]

Case 3 is particularly interesting if the parse error happens after a few of the <glyphs> have been parsed, but not all of them. I bet browsers would disagree there.

Then again, I think Opera is the only other browser to support SVG Fonts, so none of these are real compat issues (yet).

[Nikolas Zimmermann]

Thanks for review. Landed in r29839.

Why is this bug still open?

[Oliver Hunt]

Because niko forgot to close it?

Niko fixed in r29839