Bug 15741

Summary: REGRESSION: svg/W3C-SVG-1.1/fonts-elem-03-b.svg shows worse behavior on TOT
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, zimmermann
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
SVG Fonts support oliver: review+

Description Eric Seidel (no email) 2007-10-28 18:40:18 PDT
svg/W3C-SVG-1.1/fonts-elem-03-b.svg
has never been correct, and wont' be until we implement SVG fonts, but at least the font size used to be correct before. Now it just spews about how it couldn't create a font.  We should recognize .svg urls and not try to load them using the standard font-face stuff.

ERROR: unable to initialize with font (null) at not known
(/Stuff/Projects/WebKit/WebCore/platform/mac/FontDataMac.mm:147 void WebCore::FontData::platformInit())
ERROR: Corrupt font detected, using (null) in place of (null) located at "not known".
(/Stuff/Projects/WebKit/WebCore/platform/mac/FontDataMac.mm:154 void WebCore::FontData::platformInit())
ERROR: failed to set up font, using system font ?;}?
(/Stuff/Projects/WebKit/WebCore/platform/mac/FontDataMac.mm:161 void WebCore::FontData::platformInit())
ERROR: unable to initialize with font (null) at not known
(/Stuff/Projects/WebKit/WebCore/platform/mac/FontDataMac.mm:147 void WebCore::FontData::platformInit())
ERROR: Corrupt font detected, using (null) in place of (null) located at "not known".
(/Stuff/Projects/WebKit/WebCore/platform/mac/FontDataMac.mm:154 void WebCore::FontData::platformInit())
ERROR: failed to set up font, using system font ?;}??i??
(/Stuff/Projects/WebKit/WebCore/platform/mac/FontDataMac.mm:161 void WebCore::FontData::platformInit())
ERROR: unable to initialize with font (null) at not known
(/Stuff/Projects/WebKit/WebCore/platform/mac/FontDataMac.mm:147 void WebCore::FontData::platformInit())
ERROR: Corrupt font detected, using (null) in place of (null) located at "not known".
(/Stuff/Projects/WebKit/WebCore/platform/mac/FontDataMac.mm:154 void WebCore::FontData::platformInit())
ERROR: failed to set up font, using system font ?;}??i??
(/Stuff/Projects/WebKit/WebCore/platform/mac/FontDataMac.mm:161 void WebCore::FontData::platformInit())
Comment 1 Eric Seidel (no email) 2007-10-28 18:58:55 PDT
This seems to be a very recent regression... maybe it came from my <font-face> support.
Comment 2 Nikolas Zimmermann 2008-01-06 15:00:25 PST
Created attachment 18305 [details]
SVG Fonts support

Uploading SVG Fonts support follow-up patch here, as it fixes this problem.
Comment 3 Oliver Hunt 2008-01-06 15:03:19 PST
Comment on attachment 18305 [details]
SVG Fonts support

Leave the ENABLE(SVG_FONTS) checks in, and just change the build-webkit script to have it on by default
Comment 4 Oliver Hunt 2008-01-06 15:04:15 PST
Hmm, that may only effect a few header include barriers
Comment 5 Oliver Hunt 2008-01-06 15:07:56 PST
in FontData* CSSFontSelector::getFontData there are a set of 3 "if (!face && !...)" checks -- i can't see how these are being made to only effect svg fonts...
Comment 6 Oliver Hunt 2008-01-06 15:18:18 PST
Whyfore the CString.h include in SVGfonts?

Comment 7 Oliver Hunt 2008-01-06 15:30:03 PST
Comment on attachment 18305 [details]
SVG Fonts support

Other than my prior comments this looks good to me.  However I would very much prefer it if you could get Dan to look over the arabic form-fu

Am going to do a code style check now.
Comment 8 Alexey Proskuryakov 2008-01-06 15:37:34 PST
+        virtual ~SVGGlyphElement() { }

Constructors and destructors of polymorphic classes should not be inline, as they cause significant binary size bloat otherwise (for the same reason, they should be explicitly defined).
Comment 9 Nikolas Zimmermann 2008-01-06 15:39:23 PST
(In reply to comment #7)
> (From update of attachment 18305 [details] [edit])
> Other than my prior comments this looks good to me.  However I would very much
> prefer it if you could get Dan to look over the arabic form-fu
> 
> Am going to do a code style check now.
Dan, already okayed the arabic-fu :-)

CString.h include removed.

CSSFontSelector: Indeed need to find a way to only do this for SVG fonts. hmmm

Greetings,
Niko
Comment 10 Nikolas Zimmermann 2008-01-06 15:40:21 PST
(In reply to comment #3)
> (From update of attachment 18305 [details] [edit])
> Leave the ENABLE(SVG_FONTS) checks in, and just change the build-webkit script
> to have it on by default
Yeah, I did that.
Comment 11 Nikolas Zimmermann 2008-01-06 15:40:43 PST
(In reply to comment #8)
> +        virtual ~SVGGlyphElement() { }
> 
> Constructors and destructors of polymorphic classes should not be inline, as
> they cause significant binary size bloat otherwise (for the same reason, they
> should be explicitly defined).
Interessting, didn't know that. Fixed.

Comment 12 Oliver Hunt 2008-01-06 16:16:09 PST
Comment on attachment 18305 [details]
SVG Fonts support

"CSSFontSelector: Indeed need to find a way to only do this for SVG fonts."
It causes no regressions despite this?
Comment 13 Nikolas Zimmermann 2008-01-07 09:42:36 PST
(In reply to comment #12)
> (From update of attachment 18305 [details] [edit])
> "CSSFontSelector: Indeed need to find a way to only do this for SVG fonts."
> It causes no regressions despite this?
> 
Right, no regressions.
Comment 14 Oliver Hunt 2008-01-07 13:42:51 PST
Comment on attachment 18305 [details]
SVG Fonts support

Based on comments from hyatt and others, r=me
Comment 15 Nikolas Zimmermann 2008-01-07 15:17:40 PST
Landed in r29246.