Bug 16880 – SVGCSSFontFace should die, instead integrate within the FontCache.

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

[Nikolas Zimmermann]

The summary says it all: SVGCSSFontFace is obsolete, and we should rather integrate within the existing CSSFontFaceSource/FontCache concept. Patch is in progress.

[Nikolas Zimmermann]

Created attachment 18567 [details]
Work in progress patch

Not ready for review yet, just for mitz to check :-)

[Nikolas Zimmermann]

Created attachment 18571 [details]
Initial patch

No regressions, fixes this and 16784. Adds missing-glyph element handling, as well as the framework for external SVG Fonts. Updated all build systems, and add code to support SVG Fonts on mac/gtk/win.

[Nikolas Zimmermann]

*** Bug 16784 has been marked as a duplicate of this bug. ***

Comment on attachment 18571 [details]
Initial patch

createSVGFontCustomPlatformData is not a good name for a function that, unlike createFontCustomPlatformData, does not return a pointer to an object that needs to be freed. It does not even return a FontCustomPlatformData, so I am not sure what "custom" is doing in the name and what the function is doing in that file.

I am also not sure it makes sense to have the derived class SVGFontData if SimpleFontData instances need to check m_isSVGFont.

[Nikolas Zimmermann]

(In reply to comment #4)
> (From update of attachment 18571 [details] [edit])
> createSVGFontCustomPlatformData is not a good name for a function that, unlike
> createFontCustomPlatformData, does not return a pointer to an object that needs
> to be freed. It does not even return a FontCustomPlatformData, so I am not sure
> what "custom" is doing in the name and what the function is doing in that file.
Okay, changed to 'fontPlatformDataForSVGFont'.
 
> I am also not sure it makes sense to have the derived class SVGFontData if
> SimpleFontData instances need to check m_isSVGFont.
Well if it's no problem to add some floats to SimpleFontData, then I'd
add the data there?

[Nikolas Zimmermann]

(In reply to comment #5)
> > I am also not sure it makes sense to have the derived class SVGFontData if
> > SimpleFontData instances need to check m_isSVGFont.
> Well if it's no problem to add some floats to SimpleFontData, then I'd
> add the data there?

Hm, got a better idea. Holding an OwnPtr<SVGFontData> in SimpleFontData, when
SVG Fonts are enabled.

(In reply to comment #6)
> Hm, got a better idea. Holding an OwnPtr<SVGFontData> in SimpleFontData, when
> SVG Fonts are enabled.

Sounds good.

[Nikolas Zimmermann]

Created attachment 18572 [details]
Updated patch

Incorporated comments.

[Eric Seidel (no email)]

Comment on attachment 18572 [details]
Updated patch

We talked about naming m_fontFaceElement m_svgFontFaceElement to match the accessor name.

Personally I'm more a fan of fontData = new than fontData.set(, but I I'm not gonna pick on you too much about it. :)

Maybe foundLocalSVGFont should be "foundInlineSVGFont" since local seems to mean "local to this computer".  I'm not sure "inline" is actually better, but it occurred to me when reading it that we use "local" to use two different things.

+	, m_isCustomFont(customFont)

uses a tab! :)

if (m_unitsPerEm)
scale /= m_unitsPerEm;

might be more clear.

It's kinda icky to have such a large if in a constructor.  But a shared init() routine might be more icky.

+    : m_syntheticBold(b), m_syntheticOblique(o), m_cgFont(0), m_atsuFontID(0), m_size(s), m_font(0)

should be one per line, as per the style docs
: bar(foo)
, bar2(foo2)
, etc.

Wrong spacing in:
+float SVGFontFaceElement::verticalAdvanceY() const
?

+        return descent < 0 ? -descent : descent;

That deserves a comment, explaining why we take the absolute value.  Why it's correct to do that, and not allow authors to intentionally specify a negative decent (And thus break some broken test cases).

There are still lots of unecessary hasAttribute/getAttribute pairs.

element->hasAttribute(vert_origin_yAttr)
, etc.


+    String language = getAttribute(XMLNames::langAttr);
+    if (language.isEmpty()) // SVG defines a non-xml prefixed "lang" propert for <glyph> it seems.
+        language = getAttribute("lang");

We need to land a test for this fallback, and make sure we agree with other browsers.  :lang is mentioned for <glyph>, I'm not sure xml:lang actually makes any sense for <glyph> even though test cases use it.  We should probably prefer lang over xml:lang in any case.

The patch is fine.  I'm tempted to r+ it, but I think we really should land a simple test case for lang vs. xml:lang and which wins when different.  That, combined with little has/get issues, I guess are enough to make this an r-.

[Nikolas Zimmermann]

(In reply to comment #9)
> (From update of attachment 18572 [details] [edit])
> We talked about naming m_fontFaceElement m_svgFontFaceElement to match the
> accessor name.
Fixed.
 
> Personally I'm more a fan of fontData = new than fontData.set(, but I I'm not
> gonna pick on you too much about it. :)
I love this style to differentiate between RefPtr/OwnPtr by just reading. Okay, it only works for code I wrote, as I'm probably the only one using that convention :-)
 
> Maybe foundLocalSVGFont should be "foundInlineSVGFont" since local seems to
> mean "local to this computer".  I'm not sure "inline" is actually better, but
> it occurred to me when reading it that we use "local" to use two different
> things.
How about 'foundInDocumentSVGFont' - more verbose, but clear. Going to fix.

> 
> +       , m_isCustomFont(customFont)
> 
> uses a tab! :)
Ough ouch :-)
 
> if (m_unitsPerEm)
> scale /= m_unitsPerEm;
> 
> might be more clear.
Fixed.
 
> It's kinda icky to have such a large if in a constructor.  But a shared init()
> routine might be more icky.
> 
> +    : m_syntheticBold(b), m_syntheticOblique(o), m_cgFont(0), m_atsuFontID(0),
> m_size(s), m_font(0)
> 
> should be one per line, as per the style docs
> : bar(foo)
> , bar2(foo2)
> , etc.
It looks icky to fix that single case, as the whole file uses wrong indention. Follow-up style cleanup patch from you, might fix it as well :-) Honestly, I know we shouldn't check in wrong indented, code - so I'm fixing my new code.

 
> Wrong spacing in:
> +float SVGFontFaceElement::verticalAdvanceY() const
> ?
> 
> +        return descent < 0 ? -descent : descent;
> 
> That deserves a comment, explaining why we take the absolute value.  Why it's
> correct to do that, and not allow authors to intentionally specify a negative
> decent (And thus break some broken test cases).
Actually I haven't investigated a lot so far in this issue, the only thing I know that some W3C testcases use negative descent, where decent - as a _length_ value - only has meaning behind it for positive values. I'd prefer to leave it as is, with a comment though.

> 
> There are still lots of unecessary hasAttribute/getAttribute pairs.
> 
> element->hasAttribute(vert_origin_yAttr)
> , etc.
Oh going to fix.
  
> +    String language = getAttribute(XMLNames::langAttr);
> +    if (language.isEmpty()) // SVG defines a non-xml prefixed "lang" propert
> for <glyph> it seems.
> +        language = getAttribute("lang");
> 
> We need to land a test for this fallback, and make sure we agree with other
> browsers.  :lang is mentioned for <glyph>, I'm not sure xml:lang actually makes
> any sense for <glyph> even though test cases use it.  We should probably prefer
> lang over xml:lang in any case.
I don't think any other browser implements 'lang' based glyph detection. Going to check now, and post a report.
 
> The patch is fine.  I'm tempted to r+ it, but I think we really should land a
> simple test case for lang vs. xml:lang and which wins when different.  That,
> combined with little has/get issues, I guess are enough to make this an r-.
Yeah, r- is good, going to fix all issues first.

[Eric Seidel (no email)]

Even if you decide that no other browser implements :lang or xml:lang for <glyph> we should still have a test to make sure it's clear what our behavior is, and clear were we ever to change it.  Such a test is also useful for other browser venders.  Honestly, I would have probably ignored xml:lang (since it makes no sense to me) but if the W3C tests depend on them, we should 1.  file a bug with the W3c, and 2.  probably support it for now.

lang = "%LanguageCodes;"
The attribute value is a comma-separated list of language names as defined in [RFC3066]. The glyph can be used if the xml:lang attribute exactly matches one of the languages given in the value of this parameter, or if the xml:lang attribute exactly equals a prefix of one of the languages given in the value of this parameter such that the first tag character following the prefix is "-".
Animatable: no.

xml:lang is a single value.  :lang can be multiple.  (If I'm reading this correct).  lang specifies what xml:lang values the glyph is used for, xml:lang on a glyph would make little sense, but I could see it meaning (at least for the w3c tests) "this is one language this glyph is valid for", and being respected *after* any :lang values.

[Eric Seidel (no email)]

I just grepped the w3c test suite, I don't see a single use of xml:lang with <glyph> so I think your code is just wrong.

fonts-glyph-03-t.svg:        <glyph unicode="a" glyph-name="upward-triangle" lang="en" d="M0 0L500 0L250 900Z"/>
fonts-glyph-03-t.svg:        <glyph unicode="a" glyph-name="square" lang="fr" d="M0 250L500 250L500 750L0 750Z"/>


were the only instances of lang and <glyph> I saw.  I don't think we should support xml:lang on a glyph element.

[Eric Seidel (no email)]

Also, I grepped for a negative descent, and yes several are used.  However I think that a negative descent can make sense in a font.  A negative descent would mean that the lowest point any glyph draws at is actually above the baseline.

http://www.w3.org/TR/REC-CSS2/fonts.html#descent

grep -r "descent=\"-" * 

will show you the examples in the w3c directory.  If you believe those examples should actually have used positive values, then we should file a bug with the w3c, but I think a negative value can make sense.

[Nikolas Zimmermann]

Created attachment 18587 [details]
Updated patch v2

Includes new layout test testing glyph-selection based on lang / xml:lang attributes.

[Nikolas Zimmermann]

(In reply to comment #13)
> Also, I grepped for a negative descent, and yes several are used.  However I
> think that a negative descent can make sense in a font.  A negative descent
> would mean that the lowest point any glyph draws at is actually above the
> baseline.
The real reason we need positive descent values, is that we're calculating the height of a glyph for system fonts using font.ascent() + font.descent(), negative values break that aspect.

Try running layout tests w/o that fix.

[Nikolas Zimmermann]

(In reply to comment #12)
> were the only instances of lang and <glyph> I saw.  I don't think we should
> support xml:lang on a glyph element.

Okay, I agree on that, as you see my new layout test doesn't test this behaviour.
I still allow xml:lang on <glyph> as I've seen some SVG Fonts which used that. ASV3 allows it :(

Greetings,
Niko 

[Nikolas Zimmermann]

Created attachment 18593 [details]
Updated patch v3

After discussion with Eric regarding xml:lang :-)

[Eric Seidel (no email)]

Comment on attachment 18593 [details]
Updated patch v3

We discussed further changes over IRC.  on the whole, this looks fine.

[Nikolas Zimmermann]

Landed in r29700.

[Nikolas Zimmermann]

*** Bug 16904 has been marked as a duplicate of this bug. ***