[webkit-reviews] review denied: [Bug 16880] SVGCSSFontFace should die, instead integrate within the FontCache. : [Attachment 18572] Updated patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 16880: SVGCSSFontFace should die, instead integrate within the FontCache.
http://bugs.webkit.org/show_bug.cgi?id=16880

Attachment 18572: Updated patch
http://bugs.webkit.org/attachment.cgi?id=18572&action=edit

------- Additional Comments from Eric Seidel <eric at webkit.org>
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-.