Bug 59085 – Integrate SVG Fonts within GlyphPage concept, removing the special SVG code paths from Font, making it possible to reuse the simple text code path for SVG Fonts

• Component: SVG
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

Master bug: Integrate SVG Fonts within GlyphPage concept.
The goal is to remove all width/drawTextUsingSVGFont/.. special code paths for SVG Fonts from Font.h
The FontFastPath code for drawing/measuring simple text should just work for SVG Fonts as well, that makes it necessary to make GlyphBuffer/WidthIterator/GlyphPage aware of SVG Fonts, allowing us to reuse GlyphBuffer and the simple text code paths!

I've mostly finished the patch and will break it down into multiple smaller pieces first and do some cleanup work, in how we currently construct TextRuns.

[Nikolas Zimmermann]

Created attachment 92387 [details]
Draft patch

Uploading my all-in-one patch rewriting SVG Fonts. I'm splitting this up, and refactoring several parts.
This is not intended to be landed, but for Leo to have a look.

[Nikolas Zimmermann]

Created attachment 96012 [details]
Patch

Adding the 2nd iteration of the patch. It's complete now, has 0 pixel test regression and fixes several bugs.
I'm adding this w/o a proper ChangeLog, and LayoutTests/ changes to get some early EWS results, that's why it's marked r?.
I'll need to split up this patch into smaller pieces.

[Nikolas Zimmermann]

CC'ing Rob.

[WebKit Review Bot]

Attachment 96012 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/Font.h:142:  The parameter name "c" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/SimpleFontData.h:83:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 96012 [details]
Patch

Attachment 96012 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8768010

[Nikolas Zimmermann]

Created attachment 96014 [details]
Patch v2

Complete patch including all ChangeLogs and LayoutTest results. Marking again as r? to see whether cr-linux build is fixed.
If all build problems are gone, I'm going to split up.

[WebKit Review Bot]

Attachment 96014 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 96014 [details]
Patch v2

Attachment 96014 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8765299

[Dirk Schulze]

Comment on attachment 96014 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=96014&action=review

Well. It is a very very big patch and it is really hard to overlook everything. I have a lot more questions but need to read the patch a second and maybe a third time. Like the relation between Glyph and SVGGlyph, GlyphPage and so on. Someone with a lot more knowledge in Glyphs has to look at the patch as well.

> Source/WebCore/platform/graphics/FontFastPath.cpp:382
> +    TextRun::RenderingContext* renderingContext = run.renderingContext();

This is unused if SVGFonts are disabled. Would it make sense to surround it by an if SVG_FONTS as well

> Source/WebCore/platform/graphics/WidthIterator.h:25
> +#include "SVGGlyph.h"

Hm, isn't that SVG that needed to be surrounded by if SVG_FONTS?

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:101
> +    RenderObject* parentRenderObject = firstParentRendererForNonTextNode(renderObject);

Why first parent? Can it have more than once? :-P

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:120
> +    float scale = calculateEmUnitToPixelScaleFactor(fontData->platformData().size(), fontFaceElement->unitsPerEm());

What about the other units?

> Source/WebCore/svg/SVGFontData.cpp:101
> +    Glyph spaceGlyph = glyphPageZero->glyphDataForCharacter(' ').glyph;

Is it really just this whitespace character?

> LayoutTests/ChangeLog:61
> +        * platform/mac/svg/dynamic-updates/SVGFEConvolveMatrixElement-dom-preserveAlpha-attr-expected.png:
> +        * platform/mac/svg/dynamic-updates/SVGFEConvolveMatrixElement-svgdom-preserveAlpha-prop-expected.png:

I thought we already have image results for this test. I fear that we won't see problems with this test if you update the image results. And IIRC these tests very failing in trunk.

[Nikolas Zimmermann]

Comment on attachment 96014 [details]
Patch v2

Clear flags, I'm now splitting up this patch into smaller pieces that can be landed individually.

[Nikolas Zimmermann]

Created attachment 96889 [details]
Patch v3

[WebKit Review Bot]

Attachment 96889 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/svg/SVGFontData.cpp:111:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 51 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 96889 [details]
Patch v3

Attachment 96889 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8832220

[WebKit Review Bot]

Comment on attachment 96889 [details]
Patch v3

Attachment 96889 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8831556

New failing tests:
svg/batik/text/xmlSpace.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/batik/text/textEffect3.svg
svg/custom/svg-fonts-fallback.xhtml
svg/text/text-text-06-t.svg
svg/text/text-text-05-t.svg
svg/W3C-SVG-1.1/text-altglyph-01-b.svg
svg/custom/svg-fonts-segmented.xhtml
svg/text/text-overflow-ellipsis-svgfont.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/text/text-altglyph-01-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/svg-fonts-word-spacing.html
svg/batik/text/textEffect.svg
svg/text/text-text-04-t.svg

[WebKit Review Bot]

Created attachment 96890 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Schulze]

Comment on attachment 96889 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=96889&action=review

Giving you the chance to comment on my review before setting the flag.

> Source/WebCore/platform/graphics/FontFastPath.cpp:387
> +    TextRun::RenderingContext* renderingContext = run.renderingContext();

This is not used it SVG was disabled, rigth? Can this cause warnings during the compilation with --disable-svg? unused variable or something like that?

> Source/WebCore/platform/graphics/SimpleFontData.h:329
> +    if (m_fontData)
> +        width = m_fontData->widthForSVGGlyph(glyph, m_platformData.size());

Know that m_fontData was used before this patch. But it looks like it is only used for SVG. Can you rename it do m_SVGFontData please? just s/m_fontData/m_SVGFontData/ should be easy to make. (or m_svgFontData)

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:86
> +    return unitsPerEm ? fontSize / unitsPerEm : 1;

the original code:
if (!unitsPerEm)
    return 0.0f;

you return 1, why?

> Source/WebCore/svg/SVGAltGlyphElement.cpp:106
> +    // FIXME: No support for altGlyphDef/glyphRef.

Can you point to bug https://bugs.webkit.org/show_bug.cgi?id=60850 please?

> Source/WebCore/svg/SVGFontData.cpp:51
> +    return unitsPerEm ? fontSize / unitsPerEm : 1;

same question again. Also can it be shared with the other calculateEmUnitToPixelScaleFactor? both are static.

> LayoutTests/ChangeLog:30
> +        Update SVG pixel test baseline.

It's sad that I don't see the old pixel result next to the new one. Should be the case. How do you create your patches? :P Some changes in DRT look quite big (up to 10 px IIRC) most have a difference of at least 1px. Shouldn't this be noticeable on the pixel tests? How bi is the difference in percentage between old and new pixel results? Can the change to emToPxScale be responsible for some changes as well?

> LayoutTests/ChangeLog:41
> +        * platform/mac/svg/W3C-SVG-1.1/text-altglyph-01-b-expected.png:
> +        * platform/mac/svg/W3C-SVG-1.1/text-altglyph-01-b-expected.txt:

Why do we see the P and A in Happy now? And why are the chars stroked?

[Nikolas Zimmermann]

(In reply to comment #16)
> (From update of attachment 96889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96889&action=review
> 
> Giving you the chance to comment on my review before setting the flag.
> 
> > Source/WebCore/platform/graphics/FontFastPath.cpp:387
> > +    TextRun::RenderingContext* renderingContext = run.renderingContext();
> 
> This is not used it SVG was disabled, rigth? Can this cause warnings during the compilation with --disable-svg? unused variable or something like that?
Nope, this is not SVGTextRunRenderingContext, it's the abstract TextRun::RenderingContext, it's always available, even with svg disabled.

> 
> > Source/WebCore/platform/graphics/SimpleFontData.h:329
> > +    if (m_fontData)
> > +        width = m_fontData->widthForSVGGlyph(glyph, m_platformData.size());
> 
> Know that m_fontData was used before this patch. But it looks like it is only used for SVG. Can you rename it do m_SVGFontData please? just s/m_fontData/m_SVGFontData/ should be easy to make. (or m_svgFontData)
Nope, this is wrong. m_fontData is the SimpleFontData::AdditionalFontData object, which is always available, even if SVG is disabled.
Did you miss the last set of patches, where we introduced these _abstract_ interfaces? The goal is to use them instead of anything SVG specific.

> 
> > Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:86
> > +    return unitsPerEm ? fontSize / unitsPerEm : 1;
> 
> the original code:
> if (!unitsPerEm)
>     return 0.0f;
> 
> you return 1, why?
I can return 0 again, it doesn't matter at all - as unitsPerEm is always defined.


> 
> > Source/WebCore/svg/SVGAltGlyphElement.cpp:106
> > +    // FIXME: No support for altGlyphDef/glyphRef.
> 
> Can you point to bug https://bugs.webkit.org/show_bug.cgi?id=60850 please?
Good idea.

> 
> > Source/WebCore/svg/SVGFontData.cpp:51
> > +    return unitsPerEm ? fontSize / unitsPerEm : 1;
> 
> same question again. Also can it be shared with the other calculateEmUnitToPixelScaleFactor? both are static.
Do you know a good place where we can put it, so both SVGFontData and SVGTextRunRenderingContext can use it? I'd like to add it as free-funciton somewhere, if possible. Font.h? SimpleFontData.h? I'm not sure...

> > LayoutTests/ChangeLog:30
> > +        Update SVG pixel test baseline.
> 
> It's sad that I don't see the old pixel result next to the new one. Should be the case. How do you create your patches? :P Some changes in DRT look quite big (up to 10 px IIRC) most have a difference of at least 1px. Shouldn't this be noticeable on the pixel tests? How bi is the difference in percentage between old and new pixel results? Can the change to emToPxScale be responsible for some changes as well?
Nope emToPx... is not responsible.
Good catch, I have _no_ clue at all, why my diffs are not showing up.
I used "svn-create-patch > foobarpatch" from my root webkit checkout to create this.
Hrm, I could try webkit-patch create, but I'm not sure how that shold make any difference.


> 
> > LayoutTests/ChangeLog:41
> > +        * platform/mac/svg/W3C-SVG-1.1/text-altglyph-01-b-expected.png:
> > +        * platform/mac/svg/W3C-SVG-1.1/text-altglyph-01-b-expected.txt:
> 
> Why do we see the P and A in Happy now? And why are the chars stroked?
This is a progression, as we support named glyphs now. So we're matching the correct characters now, _but_ it still looks "broken" as we ignore the altGlyph around the P & A. This is fixed by Leos patch.

I'll work on a new patch today.

[Dirk Schulze]

Comment on attachment 96889 [details]
Patch v3

View in context: https://bugs.webkit.org/attachment.cgi?id=96889&action=review

>>> Source/WebCore/platform/graphics/SimpleFontData.h:329
>>> +        width = m_fontData->widthForSVGGlyph(glyph, m_platformData.size());
>> 
>> Know that m_fontData was used before this patch. But it looks like it is only used for SVG. Can you rename it do m_SVGFontData please? just s/m_fontData/m_SVGFontData/ should be easy to make. (or m_svgFontData)
> 
> Nope, this is wrong. m_fontData is the SimpleFontData::AdditionalFontData object, which is always available, even if SVG is disabled.
> Did you miss the last set of patches, where we introduced these _abstract_ interfaces? The goal is to use them instead of anything SVG specific.

Ok, but why is it used to detect SVGFonts?

bool isSVGFont() const { return m_fontData; }

will this change later?

>>> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:86
>>> +    return unitsPerEm ? fontSize / unitsPerEm : 1;
>> 
>> the original code:
>> if (!unitsPerEm)
>>     return 0.0f;
>> 
>> you return 1, why?
> 
> I can return 0 again, it doesn't matter at all - as unitsPerEm is always defined.

if 1 is never reached, can you assert unitsPerEm and just return fontSize / unitsPerEm?

>>> Source/WebCore/svg/SVGFontData.cpp:51
>>> +    return unitsPerEm ? fontSize / unitsPerEm : 1;
>> 
>> same question again. Also can it be shared with the other calculateEmUnitToPixelScaleFactor? both are static.
> 
> Do you know a good place where we can put it, so both SVGFontData and SVGTextRunRenderingContext can use it? I'd like to add it as free-funciton somewhere, if possible. Font.h? SimpleFontData.h? I'm not sure...

Great. Font.h looks reasonable.

>>> LayoutTests/ChangeLog:30
>>> +        Update SVG pixel test baseline.
>> 
>> It's sad that I don't see the old pixel result next to the new one. Should be the case. How do you create your patches? :P Some changes in DRT look quite big (up to 10 px IIRC) most have a difference of at least 1px. Shouldn't this be noticeable on the pixel tests? How bi is the difference in percentage between old and new pixel results? Can the change to emToPxScale be responsible for some changes as well?
> 
> Nope emToPx... is not responsible.
> Good catch, I have _no_ clue at all, why my diffs are not showing up.
> I used "svn-create-patch > foobarpatch" from my root webkit checkout to create this.
> Hrm, I could try webkit-patch create, but I'm not sure how that shold make any difference.

I just use 'webkit-patch upload'. This recognize the bug report by parsing the changelog. So you don't have to do or add something yourself. Nevertheless, it should do the same like svn-create-patch :-/

>>> LayoutTests/ChangeLog:41
>>> +        * platform/mac/svg/W3C-SVG-1.1/text-altglyph-01-b-expected.txt:
>> 
>> Why do we see the P and A in Happy now? And why are the chars stroked?
> 
> This is a progression, as we support named glyphs now. So we're matching the correct characters now, _but_ it still looks "broken" as we ignore the altGlyph around the P & A. This is fixed by Leos patch.
> 
> I'll work on a new patch today.

Great!

[Nikolas Zimmermann]

Created attachment 97086 [details]
Patch v4

[WebKit Review Bot]

Comment on attachment 97086 [details]
Patch v4

Attachment 97086 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8843121

New failing tests:
svg/batik/text/xmlSpace.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/batik/text/textEffect3.svg
svg/custom/svg-fonts-fallback.xhtml
svg/text/text-text-06-t.svg
svg/text/text-text-05-t.svg
svg/W3C-SVG-1.1/text-altglyph-01-b.svg
svg/custom/svg-fonts-segmented.xhtml
svg/text/text-overflow-ellipsis-svgfont.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/text/text-altglyph-01-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/svg-fonts-word-spacing.html
svg/batik/text/textEffect.svg
svg/text/text-text-04-t.svg

[WebKit Review Bot]

Created attachment 97090 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Dirk Schulze]

It still does not seem to build on win.

[Nikolas Zimmermann]

(In reply to comment #22)
> It still does not seem to build on win.

Yes, and I have no clue how to fix it. CC'ing Adam Roben.

[Dirk Schulze]

Comment on attachment 97086 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=97086&action=review

I'm still interested in why m_fontData is used to detect SVGFonts, but is not only used by SVGFonts. You said it makes no sense to rename it to something with SVG.

> Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:86
> +    return unitsPerEm ? fontSize / unitsPerEm : 0;

You did not answer the question, if you say we never reach 0, why can't you assert here? Also couldn't you find a way to share this line?

[Nikolas Zimmermann]

(In reply to comment #24)
> (From update of attachment 97086 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97086&action=review
> 
> I'm still interested in why m_fontData is used to detect SVGFonts, but is not only used by SVGFonts. You said it makes no sense to rename it to something with SVG.
> 
> > Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:86
> > +    return unitsPerEm ? fontSize / unitsPerEm : 0;
> 
> You did not answer the question, if you say we never reach 0, why can't you assert here? Also couldn't you find a way to share this line?


Well in theory, it's easy to reach unitsPerEm=0, if any platforms SimpleFontData does m_fontMetrics.setUnitsPerEm(0) during initializiation. Hm, So I'll add an assert - I'll also find a way to share this.

[Nikolas Zimmermann]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 97086 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=97086&action=review
> > 
> > I'm still interested in why m_fontData is used to detect SVGFonts, but is not only used by SVGFonts. You said it makes no sense to rename it to something with SVG.
> > 
> > > Source/WebCore/rendering/svg/SVGTextRunRenderingContext.cpp:86
> > > +    return unitsPerEm ? fontSize / unitsPerEm : 0;
> > 
> > You did not answer the question, if you say we never reach 0, why can't you assert here? Also couldn't you find a way to share this line?
> 
> 
> Well in theory, it's easy to reach unitsPerEm=0, if any platforms SimpleFontData does m_fontMetrics.setUnitsPerEm(0) during initializiation. Hm, So I'll add an assert - I'll also find a way to share this.

It was a good idea to revisit this. It turns out the unitsPerEm factor is directly computed from an attribute of the SVG <font-face> "unitsPerEmAttr". So it might be null.

I've checked the existing SimpleFontDataMac/CGWin code that deals with scaling to an em unit space:
static inline float scaleEmToUnits(float x, unsigned unitsPerEm) { return unitsPerEm ? x / static_cast<float>(unitsPerEm) : x; }

This does the right job, and I'm going to turn it into a free-function in FontMetrics.h so SimpleFontDataMac/SimpleFontDataCGWin/SVGFontData and SVGTextRunRenderingContext share the same code.

[Nikolas Zimmermann]

(In reply to comment #18)
> 
> Ok, but why is it used to detect SVGFonts?
> 
> bool isSVGFont() const { return m_fontData; }
> 
> will this change later?

For now only SVG Fonts set the SimpleFontData::AdditionalFontData object.
If this ever changes, we have to use "return m_fontData ? m_fontData->isSVGData() : false" there.
For now we save the virtual function call to a hypothetic isSVGData() method, as it's guaranteed if m_fontData is set we deal with a SVG Font.

(In reply to comment #24)
> (From update of attachment 97086 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97086&action=review
> 
> I'm still interested in why m_fontData is used to detect SVGFonts, but is not only used by SVGFonts. You said it makes no sense to rename it to something with SVG.

Does that answer your question?

[Nikolas Zimmermann]

Created attachment 97275 [details]
Patch v5

Fix Dirks comments. Hopefully fixes the win build...

[Early Warning System Bot]

Comment on attachment 97275 [details]
Patch v5

Attachment 97275 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8835988

[Nikolas Zimmermann]

Created attachment 97283 [details]
Patch v6

More work on Qt/win build fixes. It turns out SVGTextRunRenderingContexts base class has pure virtual functions and a virtual dtor. SVGTextRunRenderingContext itself has virtual functions and no dtor at all, hopefully explicitly specifying a virtual dtor fixes the msvc linkage issue.

[Early Warning System Bot]

Comment on attachment 97283 [details]
Patch v6

Attachment 97283 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8843674

[Nikolas Zimmermann]

Created attachment 97296 [details]
Patch v7

Trying to disable SVG Fonts completly for Qt, if QRawFont (aka. qt >´4.8) is not available or turned off.
The win linkage problem is still mysterious :-(

[Early Warning System Bot]

Comment on attachment 97296 [details]
Patch v7

Attachment 97296 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8867073

[WebKit Review Bot]

Comment on attachment 97296 [details]
Patch v7

Attachment 97296 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8850450

New failing tests:
svg/batik/text/xmlSpace.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/batik/text/textEffect3.svg
svg/custom/svg-fonts-fallback.xhtml
svg/text/text-text-06-t.svg
svg/text/text-text-05-t.svg
svg/W3C-SVG-1.1/text-altglyph-01-b.svg
svg/custom/svg-fonts-segmented.xhtml
svg/text/text-overflow-ellipsis-svgfont.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/text/text-altglyph-01-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/svg-fonts-word-spacing.html
svg/batik/text/textEffect.svg
svg/text/text-text-04-t.svg

[WebKit Review Bot]

Created attachment 97310 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Andreas Kling]

Comment on attachment 97296 [details]
Patch v7

View in context: https://bugs.webkit.org/attachment.cgi?id=97296&action=review

> Source/WebCore/WebCore.pro:3149
> +    # We have to disable SVG Fonts, which rely on the fast path.
> +    DEFINES -= ENABLE_SVG_FONTS=1

I believe you need to do this here:
DEFINES -= ENABLE_SVG_FONTS=1
DEFINES += ENABLE_SVG_FONTS=0

[Nikolas Zimmermann]

Created attachment 97432 [details]
Patch v8

Trying Andreas suggestion..

[WebKit Review Bot]

Comment on attachment 97432 [details]
Patch v8

Attachment 97432 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8880163

New failing tests:
svg/batik/text/xmlSpace.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/batik/text/textEffect3.svg
svg/custom/svg-fonts-fallback.xhtml
svg/text/text-text-06-t.svg
svg/text/text-text-05-t.svg
svg/W3C-SVG-1.1/text-altglyph-01-b.svg
svg/custom/svg-fonts-segmented.xhtml
svg/text/text-overflow-ellipsis-svgfont.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/text/text-altglyph-01-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/svg-fonts-word-spacing.html
svg/batik/text/textEffect.svg
svg/text/text-text-04-t.svg

[WebKit Review Bot]

Created attachment 97434 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Csaba Osztrogonác]

(In reply to comment #37)
> Created an attachment (id=97432) [details]
> Patch v8
> 
> Trying Andreas suggestion..

Unfortunately Qt build system can't handle this modification with incremental build, and it killed the EWS. :S See https://bugs.webkit.org/show_bug.cgi?id=38054 for details.

[Csaba Osztrogonác]

Comment on attachment 97432 [details]
Patch v8

Sorry, but I try to r- to make Qt EWS happier. This r- isn't for the 
patch. I'll try to build this locally. And then please r? again. I hope 
Qt EWS won't try to build it again and again.

[Csaba Osztrogonác]

Created attachment 97439 [details]
Qt build log for Patch v8

Unfortunately it still fails.

[Nikolas Zimmermann]

Created attachment 97444 [details]
Patch v9

Credits to ossy for finding the culprit of the Qt build issue.
Move the SVG Fonts disabling hack from WebCore.pro to features.pri and surround "vkern" in svgtags.in with SVG_FONTS guards, otherwhise building w/o SVG Fonts will fail.

Trying another hack to fix the msvc link problem...

[WebKit Review Bot]

Comment on attachment 97444 [details]
Patch v9

Attachment 97444 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8880217

New failing tests:
svg/batik/text/xmlSpace.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/batik/text/textEffect3.svg
svg/custom/svg-fonts-fallback.xhtml
svg/text/text-text-06-t.svg
svg/text/text-text-05-t.svg
svg/W3C-SVG-1.1/text-altglyph-01-b.svg
svg/custom/svg-fonts-segmented.xhtml
svg/text/text-overflow-ellipsis-svgfont.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/text/text-altglyph-01-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/svg-fonts-word-spacing.html
svg/batik/text/textEffect.svg
svg/text/text-text-04-t.svg

[WebKit Review Bot]

Created attachment 97448 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Csaba Osztrogonác]

Comment on attachment 97444 [details]
Patch v9

r- now, because of win build error, and make Qt EWS happier.

Could you touch platform.h with the next patch not to break Qt EWS again and again, please?

[Nikolas Zimmermann]

(In reply to comment #46)
> (From update of attachment 97444 [details])
> r- now, because of win build error, and make Qt EWS happier.
> 
> Could you touch platform.h with the next patch not to break Qt EWS again and again, please?
Ouch, sorry ossy! I thought touching features.pri would trigger a global rebuild as well - touching platform.h is unfortunate, but if we have no other choice, I'll do that.

[Nikolas Zimmermann]

Created attachment 97567 [details]
Patch v10

New day, new patch. Revert all win build hacks, none of them worked.
I just discovered that I forward declare WidthIterator/GlyphData using a class-forward, but these are actually structs, maybe that confuses the win linker.

This time I'm touching wtf/Platform.h, in order to not break Qt EWS again, I _hope_ this time it will work.

[Nikolas Zimmermann]

Hm, win still fails:  "public: virtual struct WebCore::GlyphData __thiscall WebCore::SVGTextRunRenderingContext::glyphDataForCharacter(class WebCore::Font const &,class WebCore::TextRun const &,class WebCore::WidthIterator &,int,bool,int,unsigned int &)" (?"

It still says "class WebCore::WidthIterator", I found out there's another case of "class WidthIterator" in SimpleFontData.h - I'll change that to "struct WidthIterator" in a follow-up patch ... what a journey.

[WebKit Review Bot]

Comment on attachment 97567 [details]
Patch v10

Attachment 97567 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8881493

New failing tests:
svg/batik/text/xmlSpace.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/batik/text/textEffect3.svg
svg/custom/svg-fonts-fallback.xhtml
svg/text/text-text-06-t.svg
svg/text/text-text-05-t.svg
svg/W3C-SVG-1.1/text-altglyph-01-b.svg
svg/custom/svg-fonts-segmented.xhtml
svg/text/text-overflow-ellipsis-svgfont.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/text/text-altglyph-01-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/svg-fonts-word-spacing.html
svg/batik/text/textEffect.svg
svg/text/text-text-04-t.svg

[WebKit Review Bot]

Created attachment 97574 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Created attachment 97581 [details]
Patch v11

[Nikolas Zimmermann]

/me dances around. Win build is fixed, it was really the struct vs. class-forward that made linking fail! Patch is now ready for review.

[WebKit Review Bot]

Comment on attachment 97581 [details]
Patch v11

Attachment 97581 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8879545

New failing tests:
svg/batik/text/xmlSpace.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/batik/text/textEffect3.svg
svg/custom/svg-fonts-fallback.xhtml
svg/text/text-text-06-t.svg
svg/text/text-text-05-t.svg
svg/W3C-SVG-1.1/text-altglyph-01-b.svg
svg/custom/svg-fonts-segmented.xhtml
svg/text/text-overflow-ellipsis-svgfont.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/text/text-altglyph-01-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/svg-fonts-word-spacing.html
svg/batik/text/textEffect.svg
svg/text/text-text-04-t.svg

[WebKit Review Bot]

Created attachment 97588 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Csaba Osztrogonác]

(In reply to comment #52)
> Created an attachment (id=97581) [details]
> Patch v11

I don't know why Qt EWS didn't apply this patch. I tried to build locally, but WebKitBuild/Release/WebCore/generated/JSDOMWindow.cpp wasn't regenerated, but it should. Could you touch WebCore/page/DOMWindow.idl too, because it depends on ENABLE_SVG_FONTS define, please?

[Nikolas Zimmermann]

Created attachment 97591 [details]
Patch v12 (landed and rolled out)

The patch builds fine for Qt, but not in the EWS which does incremental building, as several things are not regenerated.
Touching DOMWindow.idl upon Ossys suggestion.

[WebKit Review Bot]

Comment on attachment 97591 [details]
Patch v12 (landed and rolled out)

Attachment 97591 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8881550

New failing tests:
svg/batik/text/xmlSpace.svg
svg/W3C-SVG-1.1-SE/coords-units-03-b.svg
svg/batik/text/textEffect3.svg
svg/custom/svg-fonts-fallback.xhtml
svg/text/text-text-06-t.svg
svg/text/text-text-05-t.svg
svg/W3C-SVG-1.1/text-altglyph-01-b.svg
svg/custom/svg-fonts-segmented.xhtml
svg/text/text-overflow-ellipsis-svgfont.html
svg/W3C-SVG-1.1/fonts-glyph-03-t.svg
svg/text/text-altglyph-01-b.svg
svg/wicd/test-rightsizing-b.xhtml
svg/custom/svg-fonts-word-spacing.html
svg/batik/text/textEffect.svg
svg/text/text-text-04-t.svg

[WebKit Review Bot]

Created attachment 97598 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Rob Buis]

Comment on attachment 97591 [details]
Patch v12 (landed and rolled out)

View in context: https://bugs.webkit.org/attachment.cgi?id=97591&action=review

Looks great, just check whether you have all test results included and of course that there are no regressions. r=me

> Source/WebCore/ChangeLog:38
> +        * features.pri: Temporarily turn of SVG Fonts for Qt, as long as QRawFont support is not available and the fast path is disabled.

Turn off

> Source/WebCore/platform/graphics/TextRun.h:121
>          virtual ~RenderingContext() { }

It is a bit strange that this class name is generic but has SVG specific methods. But not needed for this patch to fix that.

> LayoutTests/ChangeLog:79
> +        * svg/custom/svg-fonts-fallback.xhtml: Added.

Are some test results like for this one missing?

[Nikolas Zimmermann]

I'm going to land this patch somewhen around 18:00 - 19:00 CEST. I'll need to coordinate that with Ossy, as it will cause a massive changeset for Qt, as SVG Fonts are temporarily turned off for them.

[Csaba Osztrogonác]

I ran a test with "97591: Patch v12" on Qt, but ~80 tests failed. 
I checked them manually, but some of them are absolutely differs 
from Mac png results. I don't know why. And I won't spend my 
weekend to find what cause these differences ...

Feel free to check the results yourself or find any Qt volunteer, 
who is interested in these failing tests, or simple add failing
tests to the Skipped list of Qt as we usually do, and somebody
will fix them or not. :)

Additionally old-run-webkit-tests is crazy, because it modified some
platform independent results, but it shouldn't do it. I usally use
"-p --platform qt --reset-results --add-platform-exceptions" options,
but I have no idea why ORWT touch platform independent results. I will
file a bug on it on monday morning, and will check what happened.

List of touched platform independent results:

#       modified:   LayoutTests/http/tests/misc/SVGFont-delayed-load-expected.txt
#       modified:   LayoutTests/svg/custom/acid3-test-77-expected.txt
#       modified:   LayoutTests/svg/custom/global-constructors-expected.txt
#       modified:   LayoutTests/svg/custom/glyph-setting-d-attribute-expected.txt
#       modified:   LayoutTests/svg/custom/glyph-transformation-with-hkern-expected.txt
#       modified:   LayoutTests/svg/custom/insertItemBefore-from-non-list-origin-expected.txt
#       modified:   LayoutTests/svg/custom/svg-features-expected.txt
#       modified:   LayoutTests/svg/dom/baseVal-animVal-crash-expected.txt
#       modified:   LayoutTests/svg/dom/font-face-elements-expected.txt



:-S
Crazy bugzilla ... I can't upload the 5Mb sized diff. :(((

[Csaba Osztrogonác]

Created attachment 97722 [details]
Qt diff for Patch v12 - part I

[Csaba Osztrogonác]

Created attachment 97723 [details]
Qt diff for Patch v12 - part II

[Csaba Osztrogonác]

Created attachment 97724 [details]
Qt diff for Patch v12 - part III

[Csaba Osztrogonác]

Andreas, any opinion about these failing tests?

[Nikolas Zimmermann]

(In reply to comment #62)
> I ran a test with "97591: Patch v12" on Qt, but ~80 tests failed. 
> I checked them manually, but some of them are absolutely differs 
> from Mac png results. I don't know why. And I won't spend my 
> weekend to find what cause these differences ...
Heh, Of course lots of test will fail now that SVG Fonts are _disabled_ for Qt.
SVG Fonts are used in lots of tests.

> 
> Feel free to check the results yourself or find any Qt volunteer, 
> who is interested in these failing tests, or simple add failing
> tests to the Skipped list of Qt as we usually do, and somebody
> will fix them or not. :)
SVG Fonts will magically work again, once QRawFont is turned on.

> 
> Additionally old-run-webkit-tests is crazy, because it modified some
> platform independent results, but it shouldn't do it. I usally use
> "-p --platform qt --reset-results --add-platform-exceptions" options,
> but I have no idea why ORWT touch platform independent results. I will
> file a bug on it on monday morning, and will check what happened.
Thanks!

> List of touched platform independent results:
> 
> #       modified:   LayoutTests/http/tests/misc/SVGFont-delayed-load-expected.txt
> #       modified:   LayoutTests/svg/custom/acid3-test-77-expected.txt
> #       modified:   LayoutTests/svg/custom/global-constructors-expected.txt
> #       modified:   LayoutTests/svg/custom/glyph-setting-d-attribute-expected.txt
> #       modified:   LayoutTests/svg/custom/glyph-transformation-with-hkern-expected.txt
> #       modified:   LayoutTests/svg/custom/insertItemBefore-from-non-list-origin-expected.txt
> #       modified:   LayoutTests/svg/custom/svg-features-expected.txt
> #       modified:   LayoutTests/svg/dom/baseVal-animVal-crash-expected.txt
> #       modified:   LayoutTests/svg/dom/font-face-elements-expected.txt
> 
> 
> 
> :-S
> Crazy bugzilla ... I can't upload the 5Mb sized diff. :(((
Thanks a lot for testing ossy!

[Nikolas Zimmermann]

Committed r89233: <http://trac.webkit.org/changeset/89233>

[Nikolas Zimmermann]

Oops, webkit-patch already closed this bug, I'm reopening until all Qt results are fixed, and eventually other non-mac platforms.

[Nikolas Zimmermann]

There's a regression tracked by https://bugs.webkit.org/show_bug.cgi?id=6297 with the Acid3 test.

[Kenneth Russell]

I'm sorry, but I rolled out this patch in http://trac.webkit.org/changeset/89311 after discussion on #webkit due to crashes in http/tests/misc/acid3.html which manifested themselves as timeouts of that test on multiple platforms.

When re-submitting it, please pick up the build fix in http://trac.webkit.org/changeset/89235 .

[Csaba Osztrogonác]

I rolled out Qt follow-up patches: http://trac.webkit.org/changeset/89341

[Ryosuke Niwa]

Hi Nikolas,

When you land your patch next time, could you add back test expectations I removed in http://trac.webkit.org/changeset/89395 (the ones marked as BUGWK62974)?  It'll help us keeping the Chromium bots green between the time your patch is landed and we/you rebaseline them.

[Nikolas Zimmermann]

(In reply to comment #71)
> I'm sorry, but I rolled out this patch in http://trac.webkit.org/changeset/89311 after discussion on #webkit due to crashes in http/tests/misc/acid3.html which manifested themselves as timeouts of that test on multiple platforms.
> 
> When re-submitting it, please pick up the build fix in http://trac.webkit.org/changeset/89235 .

Kenneth, thanks a lot for reverting. Unfortunately I had to leave unexpectedly on that day and couldn't do it on my own :( I spent the week tracking the problem down, and have a fix that I'm going to attach soon, so the next reviewer can easily see what was broken with the last patch that got landed in r89311 and how I fixed the Acid3 regression.

(In reply to comment #73)
> Hi Nikolas,
> 
> When you land your patch next time, could you add back test expectations I removed in http://trac.webkit.org/changeset/89395 (the ones marked as BUGWK62974)?  It'll help us keeping the Chromium bots green between the time your patch is landed and we/you rebaseline them.
Sure.

I'll also make sure that I include the Qt rebaselines next time I land this.

[Nikolas Zimmermann]

Created attachment 98480 [details]
The fix of the Acid3 regression (diff to previously submitted patch)

The fix of the Acid3 regression (diff to previously submitted patch): Mostly interessting for the next reviewer. I already splitted out the SurrogatePairAwareTextIterator patch out of it and submitted it as bug 63319. Once that landed in trunk I'm going to submit a new patch here that contains the SVG Fonts rewrite w/o the Acid3 regression and including the Qt/chromium rebaselines/expectation changes.

[Nikolas Zimmermann]

Created attachment 98527 [details]
Patch v13

This new patch has no regression, especially Acid3 works again perfectly. I hope it contains all Qt/Chromium rebaseline/expectation changes that have been landed so far. Let's see :-)
It's quite large as it also contains the Qt pixel test rebaselines..

[Rob Buis]

Comment on attachment 98527 [details]
Patch v13

View in context: https://bugs.webkit.org/attachment.cgi?id=98527&action=review

LGTM

> Source/JavaScriptCore/ChangeLog:8
> +        * wtf/Platform.h: Force Qt-EWS into a full rebuild, otherwhise this patch breaks the EWS.

Typo -> otherwise

[WebKit Review Bot]

Comment on attachment 98527 [details]
Patch v13

Attachment 98527 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8935508

New failing tests:
svg/custom/svg-fonts-word-spacing.html
svg/custom/svg-fonts-segmented.xhtml
svg/text/text-overflow-ellipsis-svgfont.html
svg/custom/svg-fonts-fallback.xhtml

[WebKit Review Bot]

Created attachment 98542 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nikolas Zimmermann]

Committed r89732: <http://trac.webkit.org/changeset/89732>

[Nikolas Zimmermann]

Landed a mac-leopard specific new result for a svg/foreignObject/ test in r89735 - the only one that failed :-)

[Nikolas Zimmermann]

Skip svg/text/select-text-svgfont.html	on mac-wk2, just like all other svg/text tests in r89738.

Note: I was on a gardening mission and also turned Leopard green again -- I want to make sure none of the bots has any regressions from my patch. It's still going to take some time until I've crawled through all platform results.html...

[Nikolas Zimmermann]

Landed win rebaselines in r89739. Gtk is still TODO, the rest is fixed.

[Nikolas Zimmermann]

Landed gtk rebaselines in r89740.
The rebaseline marathon should be done (for anyone but chromium). The remaining errors aren't mine.

[Csaba Osztrogonác]

(In reply to comment #80)
> Committed r89732: <http://trac.webkit.org/changeset/89732>

OMG, you landed many unrelated and unnecessary changes to Qt Skipped list :-((

I'm going to fix it now.

[Csaba Osztrogonác]

(In reply to comment #85)
> (In reply to comment #80)
> > Committed r89732: <http://trac.webkit.org/changeset/89732>
> 
> OMG, you landed many unrelated and unnecessary changes to Qt Skipped list :-((
> 
> I'm going to fix it now.

Done: http://trac.webkit.org/changeset/89796

[Nikolas Zimmermann]

(In reply to comment #85)
> (In reply to comment #80)
> > Committed r89732: <http://trac.webkit.org/changeset/89732>
> 
> OMG, you landed many unrelated and unnecessary changes to Qt Skipped list :-((
> 
> I'm going to fix it now.

Bah, that was a bad merge problem, I'm sorry :( I tried hard to incorporate all follow-up fixes that were landed after the patch first went in. I forgot to fixup the Qt skipped list :(

[Csaba Osztrogonác]

(In reply to comment #87)

> Bah, that was a bad merge problem, I'm sorry :( I tried hard to incorporate all follow-up fixes that were landed after the patch first went in. I forgot to fixup the Qt skipped list :(

Not problem, I only surprised why these tests fail again. :)