Bug 10652

Summary: Need SVGFontFace*Element(s) DOM objects
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Nobody <webkit-unassigned>
Severity: Normal    
Priority: P4    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.w3.org/TR/SVG/fonts.html#FontFaceElement
Bug Depends on: 15588    
Bug Blocks: 10649, 10650    
Description Flags
partial fix
more complete fix (still not quite functional)
complete fix
darin: review-
test case showing @font-face and <font-face>
test case showing font-face-src fallback (blocked by 15588)
improved complete fix (ignore the large project file changes)
improved complete fix (large file size is from project changes)
hyatt: review+
possible cascade test (after hyatt's correction)
Fixed cascade test none

Description Eric Seidel (no email) 2006-08-31 01:29:09 PDT
Need SVGFontFace*Element(s) DOM objects

To complete this bug would mean implementing one more more of the SVGFontFace*Element dom objects.  By this I mean simple parsing and DOM element creation.  Only once this is done can one actually implement a fontData() method for SVGFontElement bug 10650.

Each of these DOM elements should be quite easy to create.

This blocks bug 10649.
Comment 1 Eric Seidel (no email) 2007-10-04 23:01:21 PDT
Ok, now that hyatt has implemented @font-face, we can actually wire this up.  I've created a patch to do just that.  It parses and partially builds the font-face rule, but doesn't insert it yet.  Still need to chat with hyatt about how to best do that.
Comment 2 Eric Seidel (no email) 2007-10-04 23:02:53 PDT
Created attachment 16537 [details]
partial fix

a partial fix (mostly just does parsing).  Posting this so hyatt and others can more easily see what I've done.
Comment 3 Eric Seidel (no email) 2007-10-05 10:19:10 PDT
Created attachment 16547 [details]
more complete fix (still not quite functional)
Comment 4 Eric Seidel (no email) 2007-10-05 10:53:53 PDT
Created attachment 16548 [details]
complete fix

I'll add some tests after discussion with hyatt.
Comment 5 Eric Seidel (no email) 2007-10-05 10:54:55 PDT
Created attachment 16549 [details]
test case showing @font-face and <font-face>
Comment 6 Darin Adler 2007-10-05 14:38:41 PDT
Comment on attachment 16548 [details]
complete fix

+ This file is part of the WebKit project

We don't normally include that in headers. I'd prefer to just omit it.

+    class SVGDefinitionSrcElement : public SVGElement
+    SVGDefinitionSrcElement(const QualifiedName&, Document*);
+    virtual ~SVGDefinitionSrcElement();
+    virtual void childrenChanged();

This looks indented wrong. Brace goes on same line as function definition.

+namespace WebCore

Brace goes on same line as namespace.

+    virtual ~SVGDefinitionSrcElement();

There's no need to declare and define the destructor explicitly in a case like this where the destructor is already virtual, unless there's something about the constructor that you can't compile given just the header (like a data member that is a RefPtr for a type that's forward declared). For simple classes it's far better to remove it.

+#define SVGDefinitionSrcElement_h

Normally we include "Platform.h" before using macros like ENABLE.

+// Defined in CSSGrammar.y, but not in any header, so just declare it here for now.
+int getPropertyID(const char* str, int len);

Lets not replicate this hack. If we need to use this in more places, then lets fix the problem.

+#if 0
+    if (definitionSrc)
+        m_styleDeclaration->setProperty(CSS_PROP_DEFINITION_SRC, definitionSrc->getAttribute(XLinkNames::hrefAttr), false);

Please don't check in code with #if 0; if you must please include a comment.

I think we need some sort of deferral of the work in rebuildFontFace -- if you do a sequence of DOM modifications, the SVGFontFaceElement will keep rebuilding itself over and over again. Maybe we can make the style declaration get rebuild as part of recalcStyle?
Comment 7 Eric Seidel (no email) 2007-10-20 22:48:15 PDT
I discovered an ASSERT in @font-face code (bug 15588) which blocks landing of this, since that bug was only hidden by another bug in the @font-face { src:} parsing code (which my SVG font-face code does not share).
Comment 8 Eric Seidel (no email) 2007-10-20 22:49:31 PDT
Created attachment 16759 [details]
test case showing font-face-src fallback (blocked by 15588)
Comment 9 Eric Seidel (no email) 2007-10-21 00:46:20 PDT
Created attachment 16762 [details]
improved complete fix (ignore the large project file changes)

This answers all of darin's concerns, except for recalcing the style selector every time a <font-face> element is changed.  I agree, it's less that ideal, but I don't see a nice way to fix it short of registering each font-face element with the document and then marking the doc as "needing style selector recalc" -- a concept which doesn't exist yet.
Comment 10 Eric Seidel (no email) 2007-10-21 01:37:18 PDT
Created attachment 16763 [details]
improved complete fix (large file size is from project changes)

I added groupings to the JS generated bindings, similar to what the Obj-C generated bindings already had.  These groupings explain the large project file diff (that and that this patch adds a bunch of files to the project).
Comment 11 Dave Hyatt 2007-10-22 15:48:05 PDT
Comment on attachment 16763 [details]
improved complete fix (large file size is from project changes)

Move the addition of the mapped element sheet rules to before the walking of the stylesheet list instead of after (in CSSStyleSelector's constructor).

Comment 12 Eric Seidel (no email) 2007-10-22 16:53:02 PDT
Created attachment 16812 [details]
possible cascade test (after hyatt's correction)
Comment 13 Eric Seidel (no email) 2007-10-22 16:56:15 PDT
Created attachment 16813 [details]
Fixed cascade test
Comment 14 Eric Seidel (no email) 2007-10-22 17:50:13 PDT
Ah.  So wonderful to finally have this in.  Note that one test (svg/custom/font-face-fallback.svg) is disabled until bug 15588 is fixed.