Bug 16980 - Support external SVG Fonts
Summary: Support external SVG Fonts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-01-22 18:08 PST by Nikolas Zimmermann
Modified: 2013-06-24 01:11 PDT (History)
5 users (show)

See Also:


Attachments
Initial patch (27.84 KB, patch)
2008-01-23 17:39 PST, Nikolas Zimmermann
eric: review-
Details | Formatted Diff | Diff
Updated patch (26.33 KB, patch)
2008-01-24 16:01 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Updated patch (26.27 KB, patch)
2008-01-27 15:20 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Final patch (26.09 KB, patch)
2008-01-28 14:08 PST, Nikolas Zimmermann
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2008-01-22 18:08:56 PST
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.
Comment 1 Nikolas Zimmermann 2008-01-23 17:39:36 PST
Created attachment 18631 [details]
Initial patch

No regressions. Remarkable small patch for such a nice feature :-)
Comment 2 Eric Seidel (no email) 2008-01-24 00:32:59 PST
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?
Comment 3 Eric Seidel (no email) 2008-01-24 00:33:00 PST
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?
Comment 4 Nikolas Zimmermann 2008-01-24 05:44:01 PST
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.
Comment 5 Nikolas Zimmermann 2008-01-24 16:01:32 PST
Created attachment 18648 [details]
Updated patch
Comment 6 Oliver Hunt 2008-01-27 04:46:13 PST
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 7 mitz 2008-01-27 14:25:48 PST
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?
Comment 8 Nikolas Zimmermann 2008-01-27 15:15:38 PST
(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.
Comment 9 Nikolas Zimmermann 2008-01-27 15:20:00 PST
Created attachment 18726 [details]
Updated patch

Incorporated Dan's comments.
Comment 10 mitz 2008-01-28 06:50:13 PST
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.
Comment 11 Nikolas Zimmermann 2008-01-28 07:28:28 PST
(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 12 mitz 2008-01-28 13:54:54 PST
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.
Comment 13 mitz 2008-01-28 13:55:38 PST
Oliver or Eric should review the SVG code in the patch.
Comment 14 Nikolas Zimmermann 2008-01-28 14:00:09 PST
(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?
Comment 15 Nikolas Zimmermann 2008-01-28 14:08:21 PST
Created attachment 18742 [details]
Final patch

Decided to upload a final version.
Comment 16 Eric Seidel (no email) 2008-01-28 15:05:24 PST
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.
Comment 17 Eric Seidel (no email) 2008-01-28 15:07:08 PST
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).
Comment 18 Nikolas Zimmermann 2008-01-28 15:38:59 PST
Thanks for review. Landed in r29839.
Comment 19 mitz 2008-01-28 22:44:25 PST
Why is this bug still open?
Comment 20 Oliver Hunt 2008-01-28 22:46:02 PST
Because niko forgot to close it?

Niko fixed in r29839