Bug 10652 – Need SVGFontFace*Element(s) DOM objects

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 16547 [details]
more complete fix (still not quite functional)

[Eric Seidel (no email)]

Created attachment 16548 [details]
complete fix

I'll add some tests after discussion with hyatt.

[Eric Seidel (no email)]

Created attachment 16549 [details]
test case showing @font-face and <font-face>

[Darin Adler]

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
+{
+public:
+    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
+#if ENABLE(SVG)

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);
+#endif

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?

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

Created attachment 16759 [details]
test case showing font-face-src fallback (blocked by 15588)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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).

[Dave Hyatt]

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).

r=me

[Eric Seidel (no email)]

Created attachment 16812 [details]
possible cascade test (after hyatt's correction)

[Eric Seidel (no email)]

Created attachment 16813 [details]
Fixed cascade test

[Eric Seidel (no email)]

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.