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
Summary: Integrate SVG Fonts within GlyphPage concept, removing the special SVG code p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on: 59087 59096 59101 60255 62441 62501 63048 63319
Blocks: 17608 25460 32227 34236 36840 41934 60850 62973 62974 63022
  Show dependency treegraph
 
Reported: 2011-04-21 03:04 PDT by Nikolas Zimmermann
Modified: 2011-06-27 07:08 PDT (History)
14 users (show)

See Also:


Attachments
Draft patch (160.51 KB, patch)
2011-05-05 01:23 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch (108.00 KB, patch)
2011-06-03 23:50 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (1.72 MB, text/plain)
2011-06-04 00:22 PDT, Nikolas Zimmermann
no flags Details
Patch v3 (1.69 MB, patch)
2011-06-12 12:45 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.53 MB, application/zip)
2011-06-12 13:17 PDT, WebKit Review Bot
no flags Details
Patch v4 (1.70 MB, patch)
2011-06-14 01:53 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.67 MB, application/zip)
2011-06-14 02:26 PDT, WebKit Review Bot
no flags Details
Patch v5 (1.70 MB, patch)
2011-06-15 04:44 PDT, Nikolas Zimmermann
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v6 (1.70 MB, patch)
2011-06-15 06:18 PDT, Nikolas Zimmermann
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v7 (1.70 MB, patch)
2011-06-15 08:05 PDT, Nikolas Zimmermann
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.42 MB, application/zip)
2011-06-15 09:30 PDT, WebKit Review Bot
no flags Details
Patch v8 (1.70 MB, patch)
2011-06-16 03:50 PDT, Nikolas Zimmermann
ossy: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.42 MB, application/zip)
2011-06-16 04:23 PDT, WebKit Review Bot
no flags Details
Qt build log for Patch v8 (5.84 KB, patch)
2011-06-16 05:27 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch v9 (1.70 MB, patch)
2011-06-16 08:14 PDT, Nikolas Zimmermann
ossy: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.35 MB, application/zip)
2011-06-16 08:51 PDT, WebKit Review Bot
no flags Details
Patch v10 (1.70 MB, patch)
2011-06-17 02:50 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.60 MB, application/zip)
2011-06-17 03:44 PDT, WebKit Review Bot
no flags Details
Patch v11 (1.70 MB, patch)
2011-06-17 04:47 PDT, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.31 MB, application/zip)
2011-06-17 05:35 PDT, WebKit Review Bot
no flags Details
Patch v12 (landed and rolled out) (1.70 MB, patch)
2011-06-17 06:42 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.51 MB, application/zip)
2011-06-17 07:35 PDT, WebKit Review Bot
no flags Details
Qt diff for Patch v12 - part I (1.81 MB, patch)
2011-06-19 08:25 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Qt diff for Patch v12 - part II (1.89 MB, patch)
2011-06-19 08:25 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Qt diff for Patch v12 - part III (1.89 MB, patch)
2011-06-19 08:26 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
The fix of the Acid3 regression (diff to previously submitted patch) (53.32 KB, patch)
2011-06-24 04:19 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v13 (deleted)
2011-06-24 13:29 PDT, Nikolas Zimmermann
rwlbuis: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.61 MB, application/zip)
2011-06-24 14:28 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2011-04-21 03:04:48 PDT
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.
Comment 1 Nikolas Zimmermann 2011-05-05 01:23:59 PDT
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.
Comment 2 Nikolas Zimmermann 2011-06-03 23:50:59 PDT
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.
Comment 3 Nikolas Zimmermann 2011-06-03 23:51:21 PDT
CC'ing Rob.
Comment 4 WebKit Review Bot 2011-06-03 23:53:35 PDT
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.
Comment 5 WebKit Review Bot 2011-06-04 00:06:19 PDT
Comment on attachment 96012 [details]
Patch

Attachment 96012 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8768010
Comment 6 Nikolas Zimmermann 2011-06-04 00:22:57 PDT
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.
Comment 7 WebKit Review Bot 2011-06-04 00:26:46 PDT
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.
Comment 8 Early Warning System Bot 2011-06-04 00:39:00 PDT
Comment on attachment 96014 [details]
Patch v2

Attachment 96014 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8765299
Comment 9 Dirk Schulze 2011-06-06 10:56:43 PDT
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.
Comment 10 Nikolas Zimmermann 2011-06-10 01:48:58 PDT
Comment on attachment 96014 [details]
Patch v2

Clear flags, I'm now splitting up this patch into smaller pieces that can be landed individually.
Comment 11 Nikolas Zimmermann 2011-06-12 12:45:39 PDT
Created attachment 96889 [details]
Patch v3
Comment 12 WebKit Review Bot 2011-06-12 12:49:40 PDT
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.
Comment 13 Early Warning System Bot 2011-06-12 13:14:33 PDT
Comment on attachment 96889 [details]
Patch v3

Attachment 96889 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8832220
Comment 14 WebKit Review Bot 2011-06-12 13:17:28 PDT
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
Comment 15 WebKit Review Bot 2011-06-12 13:17:33 PDT
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
Comment 16 Dirk Schulze 2011-06-12 23:10:29 PDT
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?
Comment 17 Nikolas Zimmermann 2011-06-13 02:47:13 PDT
(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.
Comment 18 Dirk Schulze 2011-06-13 03:02:17 PDT
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!
Comment 19 Nikolas Zimmermann 2011-06-14 01:53:18 PDT
Created attachment 97086 [details]
Patch v4
Comment 20 WebKit Review Bot 2011-06-14 02:26:32 PDT
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
Comment 21 WebKit Review Bot 2011-06-14 02:26:38 PDT
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
Comment 22 Dirk Schulze 2011-06-14 09:11:19 PDT
It still does not seem to build on win.
Comment 23 Nikolas Zimmermann 2011-06-14 10:00:07 PDT
(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.
Comment 24 Dirk Schulze 2011-06-14 10:12:06 PDT
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?
Comment 25 Nikolas Zimmermann 2011-06-14 10:34:53 PDT
(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.
Comment 26 Nikolas Zimmermann 2011-06-15 03:33:00 PDT
(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.
Comment 27 Nikolas Zimmermann 2011-06-15 04:21:48 PDT
(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?
Comment 28 Nikolas Zimmermann 2011-06-15 04:44:55 PDT
Created attachment 97275 [details]
Patch v5

Fix Dirks comments. Hopefully fixes the win build...
Comment 29 Early Warning System Bot 2011-06-15 05:04:52 PDT
Comment on attachment 97275 [details]
Patch v5

Attachment 97275 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8835988
Comment 30 Nikolas Zimmermann 2011-06-15 06:18:01 PDT
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.
Comment 31 Early Warning System Bot 2011-06-15 07:29:13 PDT
Comment on attachment 97283 [details]
Patch v6

Attachment 97283 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8843674
Comment 32 Nikolas Zimmermann 2011-06-15 08:05:49 PDT
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 :-(
Comment 33 Early Warning System Bot 2011-06-15 08:35:15 PDT
Comment on attachment 97296 [details]
Patch v7

Attachment 97296 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8867073
Comment 34 WebKit Review Bot 2011-06-15 09:30:41 PDT
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
Comment 35 WebKit Review Bot 2011-06-15 09:30:46 PDT
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
Comment 36 Andreas Kling 2011-06-16 02:54:40 PDT
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
Comment 37 Nikolas Zimmermann 2011-06-16 03:50:55 PDT
Created attachment 97432 [details]
Patch v8

Trying Andreas suggestion..
Comment 38 WebKit Review Bot 2011-06-16 04:23:00 PDT
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
Comment 39 WebKit Review Bot 2011-06-16 04:23:06 PDT
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
Comment 40 Csaba Osztrogonác 2011-06-16 05:09:23 PDT
(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.
Comment 41 Csaba Osztrogonác 2011-06-16 05:14:25 PDT
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.
Comment 42 Csaba Osztrogonác 2011-06-16 05:27:12 PDT
Created attachment 97439 [details]
Qt build log for Patch v8

Unfortunately it still fails.
Comment 43 Nikolas Zimmermann 2011-06-16 08:14:27 PDT
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...
Comment 44 WebKit Review Bot 2011-06-16 08:51:10 PDT
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
Comment 45 WebKit Review Bot 2011-06-16 08:51:17 PDT
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
Comment 46 Csaba Osztrogonác 2011-06-16 11:13:25 PDT
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?
Comment 47 Nikolas Zimmermann 2011-06-16 23:55:59 PDT
(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.
Comment 48 Nikolas Zimmermann 2011-06-17 02:50:39 PDT
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.
Comment 49 Nikolas Zimmermann 2011-06-17 03:30:32 PDT
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.
Comment 50 WebKit Review Bot 2011-06-17 03:44:18 PDT
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
Comment 51 WebKit Review Bot 2011-06-17 03:44:24 PDT
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
Comment 52 Nikolas Zimmermann 2011-06-17 04:47:57 PDT
Created attachment 97581 [details]
Patch v11
Comment 53 Nikolas Zimmermann 2011-06-17 05:17:27 PDT
/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.
Comment 54 WebKit Review Bot 2011-06-17 05:35:39 PDT
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
Comment 55 WebKit Review Bot 2011-06-17 05:35:45 PDT
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
Comment 56 Csaba Osztrogonác 2011-06-17 06:18:33 PDT
(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?
Comment 57 Nikolas Zimmermann 2011-06-17 06:42:54 PDT
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.
Comment 58 WebKit Review Bot 2011-06-17 07:35:36 PDT
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
Comment 59 WebKit Review Bot 2011-06-17 07:35:43 PDT
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
Comment 60 Rob Buis 2011-06-17 08:35:44 PDT
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?
Comment 61 Nikolas Zimmermann 2011-06-19 03:41:55 PDT
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.
Comment 62 Csaba Osztrogonác 2011-06-19 08:21:37 PDT
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. :(((
Comment 63 Csaba Osztrogonác 2011-06-19 08:25:11 PDT
Created attachment 97722 [details]
Qt diff for Patch v12 - part I
Comment 64 Csaba Osztrogonác 2011-06-19 08:25:40 PDT
Created attachment 97723 [details]
Qt diff for Patch v12 - part II
Comment 65 Csaba Osztrogonác 2011-06-19 08:26:14 PDT
Created attachment 97724 [details]
Qt diff for Patch v12 - part III
Comment 66 Csaba Osztrogonác 2011-06-19 08:30:05 PDT
Andreas, any opinion about these failing tests?
Comment 67 Nikolas Zimmermann 2011-06-19 12:58:52 PDT
(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!
Comment 68 Nikolas Zimmermann 2011-06-20 01:25:47 PDT
Committed r89233: <http://trac.webkit.org/changeset/89233>
Comment 69 Nikolas Zimmermann 2011-06-20 01:28:33 PDT
Oops, webkit-patch already closed this bug, I'm reopening until all Qt results are fixed, and eventually other non-mac platforms.
Comment 70 Nikolas Zimmermann 2011-06-20 13:44:48 PDT
There's a regression tracked by https://bugs.webkit.org/show_bug.cgi?id=6297 with the Acid3 test.
Comment 71 Kenneth Russell 2011-06-20 16:33:22 PDT
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 .
Comment 72 Csaba Osztrogonác 2011-06-21 00:12:08 PDT
I rolled out Qt follow-up patches: http://trac.webkit.org/changeset/89341
Comment 73 Ryosuke Niwa 2011-06-21 17:13:27 PDT
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.
Comment 74 Nikolas Zimmermann 2011-06-24 04:17:18 PDT
(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.
Comment 75 Nikolas Zimmermann 2011-06-24 04:19:34 PDT
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.
Comment 76 Nikolas Zimmermann 2011-06-24 13:29:26 PDT
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..
Comment 77 Rob Buis 2011-06-24 13:51:33 PDT
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
Comment 78 WebKit Review Bot 2011-06-24 14:28:21 PDT
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
Comment 79 WebKit Review Bot 2011-06-24 14:28:30 PDT
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
Comment 80 Nikolas Zimmermann 2011-06-24 23:29:31 PDT
Committed r89732: <http://trac.webkit.org/changeset/89732>
Comment 81 Nikolas Zimmermann 2011-06-25 00:31:35 PDT
Landed a mac-leopard specific new result for a svg/foreignObject/ test in r89735 - the only one that failed :-)
Comment 82 Nikolas Zimmermann 2011-06-25 00:45:14 PDT
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...
Comment 83 Nikolas Zimmermann 2011-06-25 01:17:53 PDT
Landed win rebaselines in r89739. Gtk is still TODO, the rest is fixed.
Comment 84 Nikolas Zimmermann 2011-06-25 01:44:41 PDT
Landed gtk rebaselines in r89740.
The rebaseline marathon should be done (for anyone but chromium). The remaining errors aren't mine.
Comment 85 Csaba Osztrogonác 2011-06-26 23:48:44 PDT
(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.
Comment 86 Csaba Osztrogonác 2011-06-27 01:57:56 PDT
(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
Comment 87 Nikolas Zimmermann 2011-06-27 07:05:51 PDT
(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 :(
Comment 88 Csaba Osztrogonác 2011-06-27 07:08:22 PDT
(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. :)