Bug 47467

Summary: SVG <path> DRT dumps have rounding problems across platforms
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cmarrin, dglazkov, eric, gustavo, krit, mdelaney7, mrobinson, ossy, pnormand, webkit.review.bot, xan.lopez, zherczeg, zimmermann, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 72614, 72793    
Bug Blocks: 18994, 62204    
Attachments:
Description Flags
Patch
none
Patch v1
none
Patch v2
webkit.review.bot: commit-queue-
Patch v3
webkit.review.bot: commit-queue-
Patch v4
none
Patch v5
gustavo: commit-queue-
Patch v6
gyuyoung.kim: commit-queue-
Patch v7
none
Patch v8
none
Patch v9
none
Patch v10
gyuyoung.kim: commit-queue-
Patch v11
webkit.review.bot: commit-queue-
Updated patch - v1
zherczeg: review+, zherczeg: commit-queue-
Updated patch - v1 (Mac rebaseline)
webkit.review.bot: commit-queue-
Updated patch - v1 (Qt rebaseline)
none
failing tests on 64 bit QtWebKit after r101342 none

Description Nikolas Zimmermann 2010-10-10 02:56:05 PDT
As mentioned in a webkit-dev thread, we have rounding problems when dumping the <path> data with DRT.
The problem is that String::format is used, which doesn't round but truncate. Gtk bots have to skip lots of tests, because of 32bit vs. 64bit rounding differences.

I'm proposing to add a new SVGNumberFormatter class utiliing wtf/DecimalNumber, to avoid these issues.
There are still some String::format usages in WebCore/svg that don't affect DRT dumps, and will be fixed in a seperated patch.
Comment 1 Nikolas Zimmermann 2010-10-10 03:31:44 PDT
Created attachment 70393 [details]
Patch
Comment 2 WebKit Review Bot 2010-10-10 03:35:48 PDT
Attachment 70393 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/runtime/StringBuilder.h:27:  #ifndef header guard has wrong style, please use: StringBuilder_h  [build/header_guard] [5]
WebCore/svg/SVGPathStringBuilder.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Nikolas Zimmermann 2010-10-10 04:02:01 PDT
Okay, I made up my mind, it needs to be further generalized. TextStream<<(float / double) needs the same logic, in order to get consistent values across platforms / architectures. Insipred by the SVG test skip list on platform/win/Skipped - it lists several non-<path> debug output failures, which are due the TextStream::operator<< usage.
Comment 4 Dirk Schulze 2010-10-10 09:32:53 PDT
(In reply to comment #3)
> Okay, I made up my mind, it needs to be further generalized. TextStream<<(float / double) needs the same logic, in order to get consistent values across platforms / architectures. Insipred by the SVG test skip list on platform/win/Skipped - it lists several non-<path> debug output failures, which are due the TextStream::operator<< usage.

So the review flag can be cleared? What about 18994, cant this bug be marked as duplicate?
Comment 5 Dirk Schulze 2010-10-13 01:55:25 PDT
Comment on attachment 70393 [details]
Patch

Clearing the r-flag, since it looks like you're working on another solution and we can move this patch out of the pending review list. Reset it, if you think this should get reviewed.
Comment 6 Nikolas Zimmermann 2011-11-11 07:28:17 PST
*** Bug 23927 has been marked as a duplicate of this bug. ***
Comment 7 Nikolas Zimmermann 2011-11-11 07:31:29 PST
I've generalized this, will upload a work-of-concept patch, that unifies the way DRT prints floating point numbers.
Comment 8 Nikolas Zimmermann 2011-11-14 04:42:38 PST
Created attachment 114917 [details]
Patch v1

New attempt at fixing the rounding problems across the Gtk/Win bots.
It fixes all 0.0 vs -0.0 issues, several wrong sizes that are reported in the mac dumps 481x360 -> 480x360.
This includes a patch from Zoltan to SVGPreserveAspectRatio::getCTM(), to avoid loosing precision, by using double internally. The AffineTransform that getCTM() generates uses double values, that's why this matters!
Comment 9 WebKit Review Bot 2011-11-14 04:45:18 PST
Attachment 114917 [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/JavaScriptCore/wtf/dtoa/utils.h:202:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Philippe Normand 2011-11-14 05:52:24 PST
Thanks a lot for fixing these rounding issues!
SVG is not really my area of work but this patch looks good, minus the red win EWS :)

Have you tested a GTK Debug build too?
Comment 11 Nikolas Zimmermann 2011-11-14 06:00:09 PST
Nope, t(In reply to comment #10)
> Thanks a lot for fixing these rounding issues!
> SVG is not really my area of work but this patch looks good, minus the red win EWS :)
> 
> Have you tested a GTK Debug build too?

Sorry, I didn't mention it earlier. I have not tested with Gtk, and hope you could try that maybe?
I have compared the failures mentioned in the master bug report related to gtk to the new baselines, and they match the gtk "failures" now, so I think the problem should be gone, but I didn't check all tests that are skipped only some of them.
Comment 12 Nikolas Zimmermann 2011-11-14 06:00:45 PST
Forgot to say, fixing the win build is a matter of exporting the new symbol for String::number - I'll upload a new patch soon, but it won't affect gtk/mac/qt.
Comment 13 Nikolas Zimmermann 2011-11-14 07:12:36 PST
Created attachment 114942 [details]
Patch v2

Attempt to fix build/style issues.
Comment 14 Nikolas Zimmermann 2011-11-14 11:56:52 PST
(In reply to comment #13)
> Created an attachment (id=114942) [details]
> Patch v2
> 
> Attempt to fix build/style issues.

Philippe was so kind to build and test on Gtk and send me the results.
There are still some differences, and I'm trying to find out what I can do next. Stay tuned.
Comment 15 WebKit Review Bot 2011-11-15 03:18:18 PST
Comment on attachment 114942 [details]
Patch v2

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

New failing tests:
svg/W3C-SVG-1.1/struct-frag-02-t.svg
svg/custom/circular-marker-reference-1.svg
svg/W3C-SVG-1.1/fonts-elem-03-b.svg
svg/W3C-SVG-1.1/fonts-elem-02-t.svg
svg/W3C-SVG-1.1/painting-marker-03-f.svg
fast/backgrounds/size/contain-and-cover-zoomed.html
svg/W3C-SVG-1.1/coords-viewattr-01-b.svg
svg/carto.net/scrollbar.svg
svg/custom/circular-marker-reference-3.svg
media/track/track-webvtt-tc003-newlines.html
svg/W3C-SVG-1.1-SE/text-tspan-02-b.svg
svg/W3C-SVG-1.1/fonts-elem-01-t.svg
svg/custom/circular-marker-reference-2.svg
svg/W3C-SVG-1.1/fonts-elem-04-b.svg
svg/W3C-SVG-1.1/struct-frag-03-t.svg
Comment 16 Nikolas Zimmermann 2011-11-15 05:49:17 PST
Created attachment 115150 [details]
Patch v3

Patch v3 won't build on win yet, the String::number() signature changed, and I need to wait for the win build to show me the right symbol name that I can add to the JSC.def file.
I inspected the layout test results that Philippe sent me and found out that the most common source of rounding errors between gtk/mac is the dumping of the renderer positions, see here:

-        RenderSVGPath {path} at (94,162) size 220x37 [stroke={[type=SOLID] [color=#000000] [stroke width=10.00] [miter limit=17.00]}] [data="M 20 110 L 200 120 L 20 130"]
+        RenderSVGPath {path} at (94,162) size 220x36 [stroke={[type=SOLID] [color=#000000] [stroke width=10.00] [miter limit=17.00]}] [data="M 20 110 L 200 120 L 20 130"]

These values come from the absoluteClippedOverflowRect() - which maps the renderers local repaint rect through a bunch of mapRect() calls up to the root renderer.
mapRect() is using FloatQuad::boundingBox(), operating on rounded float points. It returns FloatRect(left, top, right - left, bottom - top), where all values are floats.
We should rather use doubles then calculate the subtraction, then round to float. Fixing that changes several test results on mac.

Lets see how this affects Gtk. Philippe could you build it again and produce a baseline comparison against mac?
Quick'n'dirty: mv LayoutTests/platform/gtk/svg gtk-svg-old; mv LayoutTests/platform/mac/svg/ platform/gtk/svg/
Comment 17 Nikolas Zimmermann 2011-11-15 08:18:09 PST
(In reply to comment #16)
> Lets see how this affects Gtk. Philippe could you build it again and produce a baseline comparison against mac?

I got a new baseline from Philipe, this is a progression, but we're not there yet. Still several rounding differences like:

-    RenderSVGContainer {g} at (16,556) size 76x14
+   RenderSVGContainer {g} at (16,556) size 76x15
      RenderSVGText {text} at (10,334) size 45x8 contains 1 chunk(s)
        RenderSVGInlineText {#text} at (0,0) size 45x8
          chunk 1 text run 1 at (10.00,340.00) startOffset 0 endOffset 16 width 45.00: "$Revision: 1.7 $"

As you can see the container size differs, which is nothing but the union of its children, so how can this difference be explained? What we're dumping in writePositionAndStyle() in SVGRenderTreeAsText, is the "absoluteClippedOverflowRect".

static TextStream& writePositionAndStyle(TextStream& ts, const RenderObject& object)
{
    ts << " " << const_cast<RenderObject&>(object).absoluteClippedOverflowRect();
...
absluteClippedOverflowRect() is an IntRect, and thus not affected by the TextStream::operator<<(float) changes I made, so the remaining rounding error must be somewhere else.

What happens when we call absoluteClippedOverflowRect() on a RenderSVGContainer?
LayoutRect absoluteClippedOverflowRect() const
{
    return clippedOverflowRectForRepaint(0);
}

RenderSVGContainer inherits following from RenderSVGModelObject:

LayoutRect RenderSVGModelObject::clippedOverflowRectForRepaint(RenderBoxModelObject* repaintContainer) const
{
    return SVGRenderSupport::clippedOverflowRectForRepaint(this, repaintContainer);
}

Here's the relevant code:

LayoutRect SVGRenderSupport::clippedOverflowRectForRepaint(const RenderObject* object, RenderBoxModelObject* repaintContainer)
{
...
    // Pass our local paint rect to computeRectForRepaint() which will
    // map to parent coords and recurse up the parent chain.
    LayoutRect repaintRect = enclosingLayoutRect(object->repaintRectInLocalCoordinates());
    object->computeRectForRepaint(repaintContainer, repaintRect);
    return repaintRect;
}

void SVGRenderSupport::computeRectForRepaint(const RenderObject* object, RenderBoxModelObject* repaintContainer, LayoutRect& repaintRect, bool fixed)
{
...
    // Translate to coords in our parent renderer, and then call computeRectForRepaint on our parent
    repaintRect = object->localToParentTransform().mapRect(repaintRect);
    object->parent()->computeRectForRepaint(repaintContainer, repaintRect, fixed);
}

The culprit must be clippedOverflowRectForRepaint(). We're using enclosingIntRect on the repaint rect of the <g> (which is the union of the repaint rects of all children, all stored as FloatRects) and then map that through the parent CTM hierarchy up to RenderSVGRoot, via localToParentTransform(), and calling the parents computeRectForRepaint method.

This is error-prone, and should be avoided. I'm now trying a computeFloatRectForRepaint method, smilar to nodeAtFloatPoint vs. nodeAtPoint, that uses more precision inside the SVG tree - float vs int).

Let's see how that goes :-)
Comment 18 WebKit Review Bot 2011-11-15 11:06:52 PST
Comment on attachment 115150 [details]
Patch v3

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

New failing tests:
svg/W3C-SVG-1.1/animate-elem-34-t.svg
svg/W3C-SVG-1.1/struct-frag-02-t.svg
svg/custom/circular-marker-reference-1.svg
svg/W3C-SVG-1.1/fonts-elem-03-b.svg
svg/W3C-SVG-1.1/fonts-elem-02-t.svg
svg/W3C-SVG-1.1/painting-marker-03-f.svg
fast/backgrounds/size/contain-and-cover-zoomed.html
svg/W3C-SVG-1.1/coords-viewattr-01-b.svg
svg/carto.net/scrollbar.svg
svg/W3C-SVG-1.1-SE/text-tspan-02-b.svg
svg/W3C-SVG-1.1/fonts-elem-01-t.svg
svg/W3C-SVG-1.1/fonts-elem-04-b.svg
svg/W3C-SVG-1.1/painting-stroke-07-t.svg
svg/W3C-SVG-1.1/struct-frag-03-t.svg
Comment 19 Nikolas Zimmermann 2011-11-16 00:47:53 PST
Created attachment 115343 [details]
Patch v4

The patch is growing: a noticable number of new baselines is needed on Mac, and included within the patch.
I hope this fixes the last category of rounding problems remaining on Gtk: container sizes/positions.

If you look through the patch you'll see that on Mac, the container positions remained the same compared to the last patch, but the child positions/sizes changed slightly. I hope this will compensate the rounding problem seen on container sizes/positions on Gtk. It need testing.

Unfortunately this doesn't fix the win build yet, I'm missing another symbol and have to wait for EWS.
Comment 20 Nikolas Zimmermann 2011-11-16 04:38:27 PST
Created attachment 115358 [details]
Patch v5

Another iteration, this time concentrating on text.
Thanks again to both ossy & phil for building & testing my patches!
Comment 21 Gustavo Noronha (kov) 2011-11-16 04:49:45 PST
Comment on attachment 115358 [details]
Patch v5

Attachment 115358 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10484757
Comment 22 WebKit Review Bot 2011-11-16 05:10:06 PST
Comment on attachment 115358 [details]
Patch v5

Attachment 115358 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10473280
Comment 23 Gyuyoung Kim 2011-11-16 05:16:31 PST
Comment on attachment 115358 [details]
Patch v5

Attachment 115358 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10484764
Comment 24 Early Warning System Bot 2011-11-16 06:10:41 PST
Comment on attachment 115358 [details]
Patch v5

Attachment 115358 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10497001
Comment 25 Nikolas Zimmermann 2011-11-16 07:14:19 PST
Created attachment 115376 [details]
Patch v6

Next try.
Comment 26 Gyuyoung Kim 2011-11-16 07:46:57 PST
Comment on attachment 115376 [details]
Patch v6

Attachment 115376 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10487737
Comment 27 WebKit Review Bot 2011-11-16 07:56:38 PST
Comment on attachment 115376 [details]
Patch v6

Attachment 115376 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10310976
Comment 28 Gustavo Noronha (kov) 2011-11-16 08:21:32 PST
Comment on attachment 115376 [details]
Patch v6

Attachment 115376 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10483745
Comment 29 Nikolas Zimmermann 2011-11-16 13:48:20 PST
Created attachment 115442 [details]
Patch v7

New patch for phil/ossy. to try.
Comment 30 Nikolas Zimmermann 2011-11-17 01:52:37 PST
Created attachment 115543 [details]
Patch v8

Thats what you get from touching lots of test results: merging errors. Updated to ToT, I hope it applies now.
Comment 31 Nikolas Zimmermann 2011-11-17 06:08:11 PST
Phillppe built it once again, thank you so much.

While I'm confident that 32/64 bit problems should be resolve now, there is following problem remaining affecting most gtk svg tests with text:

-          RenderSVGText {text} at (290,107) size 20x40 contains 1 chunk(s)
-            RenderSVGInlineText {#text} at (0,0) size 20x40
+          RenderSVGText {text} at (290,107) size 20x41 contains 1 chunk(s)
+            RenderSVGInlineText {#text} at (0,0) size 20x41
               chunk 1 text run 1 at (290.00,140.00) startOffset 0 endOffset 1 width 20.00: "0"

Just gdb'ing this with phil with a simple reduction:
<svg viewBox="0 0 480 360" xmlns="http://www.w3.org/2000/svg" >
<text x="10" y="340" font-family="Arial" font-size="6" fill="black">$Revision: 1.4 $</text>
</svg>

Breaking here: Breakpoint 1, WebCore::writeSVGInlineTextBox (ts=@0x7fff5fbfdf90, textBox=0x108360758, indent=3) at SVGRenderTreeAsText.cpp:410

That's how it looks like on Mac:

(gdb) p textBox->textFragments().at(0)
$12 = ('WebCore::SVGTextFragment' &) @0x108862000: {
  characterOffset = 0, 
  metricsListOffset = 0, 
  length = 16, 
  isTextOnPath = false, 
  x = 10, 
  y = 340, 
  width = 45, 
  height = 6.703125, 
  transform = {
    m_transform = {1, 0, 0, 1, 0, 0}
  }, 
  lengthAdjustTransform = {
    m_transform = {1, 0, 0, 1, 0, 0}
  }
}

(gdb) p textBox->textRenderer()->style()->fontMetrics()
$15 = (const 'WebCore::FontMetrics' &) @0x1078c900c: {
  m_unitsPerEm = 2048, 
  m_ascent = 5.43164062, 
  m_descent = 1.27148438, 
  m_lineGap = 0.196289062, 
  m_lineSpacing = 6, 
  m_xHeight = 3.11132812
}

Gtk results from phil:
philn-tp: $1 = (WebCore::SVGTextFragment &) @0xe1f160: {characterOffset = 0, metricsListOffset = 0, length = 16, isTextOnPath = false, x = 10, y = 340,  width = 45, height = 7.80000019, transform = {m_transform = {1, 0, 0, 1, 0, 0}}, lengthAdjustTransform = {m_transform = {1, 0, 0, 1, 0, 0}}}

philn-tp: $2 = (const WebCore::FontMetrics &) @0xe15c4c: {m_unitsPerEm = 1000, m_ascent = 6, m_descent = 2, m_lineGap = 0, m_lineSpacing = 8, m_xHeight = 4}

Sorry for the formatting, that's over IRC.
All integers on Gtk! Let's continue investigating.
Comment 32 Philippe Normand 2011-11-17 06:23:27 PST
In WebCore::SimpleFontData::platformInit,

p font_extents
$1 = {ascent = 6, descent = 2, height = 6.9999999999999991, max_x_advance = 6.9999999999999991, max_y_advance = 0}

And in WebCore::writeSVGInlineTextBox:

p textBox->textRenderer()->style()->font()
$1 = (const WebCore::Font &) @0xe3d858: {static s_codePath = WebCore::Font::Auto, 
  static s_roundingHackCharacterTable = "\000\000\000\000\000\000\000\000\000\001\001", '\000' <repeats 21 times>, "\001", '\000' <repeats 12 times>, "\001", '\000' <repeats 17 times>, "\001", '\000' <repeats 96 times>, "\001", '\000' <repeats 94 times>, m_fontDescription = {m_familyList = {
      m_family = "Arial", m_next = {<WTF::RefPtr<WebCore::SharedFontFamily>> = {m_ptr = 0x0}, <No data fields>}}, m_featureSettings = {m_ptr = 
    0x0}, m_specifiedSize = 6, m_computedSize = 6, m_orientation = 0, m_textOrientation = 0, m_widthVariant = 0, m_italic = 0, m_smallCaps = 0, 
    m_isAbsoluteSize = true, m_weight = 3, m_genericFamily = 0, m_usePrinterFont = false, m_renderingMode = 0, m_keywordSize = 0, 
    m_fontSmoothing = 0, m_textRendering = 0, m_isSpecifiedFont = true, m_script = USCRIPT_COMMON}, m_fontList = {m_ptr = 0xe3d8b0}, 
  m_letterSpacing = 0, m_wordSpacing = 0, m_isPlatformFont = false, m_needsTranscoding = false}
Comment 33 Philippe Normand 2011-11-17 06:43:06 PST
I've just updated my libfreetype6 and fonts packages. It seems I get new results!

p textBox->textFragments().at(0)
$1 = (WebCore::SVGTextFragment &) @0xe54570: {characterOffset = 0, metricsListOffset = 0, length = 16, isTextOnPath = false, x = 10, y = 340, 
  width = 45, height = 6.5999999, transform = {m_transform = {1, 0, 0, 1, 0, 0}}, lengthAdjustTransform = {m_transform = {1, 0, 0, 1, 0, 0}}}

The font metrics are still bad though:

(gdb) p textBox->textRenderer()->style()->fontMetrics()
[Thread 0x7fffa58f7700 (LWP 606) exited]
$2 = (const WebCore::FontMetrics &) @0xe1c53c: {m_unitsPerEm = 1000, m_ascent = 5, m_descent = 1, m_lineGap = 1, m_lineSpacing = 7, m_xHeight = 4}
Comment 34 Nikolas Zimmermann 2011-11-18 08:13:55 PST
Created attachment 115811 [details]
Patch v9

This is the same patch as version 8, but with a fix for bug 72614 included, which give precise non-integer TextMetrics for Freetype text backends. Its just a matter of setting CAIRO_HINT_METRICS_OFF.
Comment 35 Nikolas Zimmermann 2011-11-18 08:24:49 PST
(In reply to comment #34)
> Created an attachment (id=115811) [details]
> Patch v9
> 
> This is the same patch as version 8, but with a fix for bug 72614 included, which give precise non-integer TextMetrics for Freetype text backends. Its just a matter of setting CAIRO_HINT_METRICS_OFF.
Bug 72614 discusses whats wrong with the Gtk/Cairo/Freetype usage at present and how to fix it easily :-)
This fix is included in patch v9.
Comment 36 Nikolas Zimmermann 2011-11-18 10:28:40 PST
Created attachment 115830 [details]
Patch v10

Okay, we finally have non-integer FontMetrics on Gtk, now I've modified SimpleFontDataFreeType to match the Mac metrics calculations, let's see how it affects Gtk. Note: I couldn't test compilation on Gtk, this is a blind attempt.

Ossy, can you do another 32bit vs. 64bit build on Qt as well?
Comment 37 Gyuyoung Kim 2011-11-18 11:04:02 PST
Comment on attachment 115830 [details]
Patch v10

Attachment 115830 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10374639
Comment 38 Nikolas Zimmermann 2011-11-18 11:19:53 PST
Created attachment 115838 [details]
Patch v11

Fix gtk/efl builds.
Comment 39 WebKit Review Bot 2011-11-18 13:25:49 PST
Comment on attachment 115838 [details]
Patch v11

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

New failing tests:
fast/block/float/overhanging-tall-block.html
svg/W3C-I18N/g-dirRTL-ubNone.svg
fast/replaced/width100percent-textarea.html
platform/chromium-linux/svg/text/text-with-geometric-precision.svg
svg/W3C-I18N/g-dirLTR-ubOverride.svg
svg/W3C-I18N/g-dirLTR-ubNone.svg
fast/backgrounds/size/contain-and-cover-zoomed.html
Comment 40 Nikolas Zimmermann 2011-11-24 00:44:51 PST
Created attachment 116484 [details]
Updated patch - v1

Now that the String::number() fixes landed, here's a minimal patch that adresses SVG <path> rounding errors, 0.0 vs -0.0 issues, and the rounding problems with the RenderSVGText/RenderSVGInlineText DRT output.

Splitting up the patch into multiple pieces here makes no sense, as each of them requires a world rebaseline, and I want to avoid that!
I'll upload a seperated patch to update the Mac baseline.
Comment 41 Nikolas Zimmermann 2011-11-24 00:45:53 PST
Created attachment 116485 [details]
Updated patch - v1 (Mac rebaseline)
Comment 42 Nikolas Zimmermann 2011-11-24 00:47:20 PST
A note to all platform maintainers. Once this lands all SVG expected.txt files need an update. I highly encourage you to check wheter your platform specific rebaselines are still needed, as lots of small DRT output problems are fixed, as they output is now platform independant, not using snprintfs anymore etc.
A unique 0.0 is enforced, removing the problems win/qt/gtk see with 0.0 vs -0.0.
Comment 43 Adam Barth 2011-11-24 00:52:19 PST
The baseline optimizer can automagically remove redundant baselines.  It's probably worth running that over the svg baselines after this patch lands and folks update their baselines.
Comment 44 Zoltan Herczeg 2011-11-24 01:43:49 PST
Comment on attachment 116484 [details]
Updated patch - v1

r=me

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

> Source/WebCore/svg/SVGPathStringBuilder.cpp:120
> +    DEFINE_STATIC_LOCAL(const String, noString, ("0"));
> +    DEFINE_STATIC_LOCAL(const String, yesString, ("1"));

I think a simple String::number conversion would be enough here. This seems overcomplicate things.
Comment 45 Zoltan Herczeg 2011-11-24 01:45:07 PST
Niko, please contact with the bot managers before landing this patch, it will likely break things.
Comment 46 Csaba Osztrogonác 2011-11-24 04:03:55 PST
Created attachment 116494 [details]
Updated patch - v1 (Qt rebaseline)
Comment 47 WebKit Review Bot 2011-11-24 19:38:15 PST
Comment on attachment 116485 [details]
Updated patch - v1 (Mac rebaseline)

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

New failing tests:
fast/replaced/width100percent-textarea.html
svg/W3C-SVG-1.1-SE/coords-dom-02-f.svg
svg/W3C-SVG-1.1-SE/coords-dom-01-f.svg
svg/W3C-SVG-1.1-SE/painting-marker-07-f.svg
svg/W3C-SVG-1.1/filters-turb-02-f.svg
svg/W3C-SVG-1.1-SE/styling-pres-02-f.svg
svg/W3C-SVG-1.1-SE/interact-pointer-03-t.svg
svg/W3C-SVG-1.1-SE/coords-dom-04-f.svg
svg/W3C-SVG-1.1-SE/coords-dom-03-f.svg
svg/W3C-SVG-1.1-SE/filters-felem-01-b.svg
svg/W3C-SVG-1.1-SE/pservers-pattern-04-f.svg
svg/W3C-SVG-1.1-SE/painting-control-04-f.svg
svg/W3C-SVG-1.1-SE/text-tref-03-b.svg
svg/W3C-SVG-1.1-SE/pservers-grad-20-b.svg
Comment 48 WebKit Review Bot 2011-11-26 11:46:17 PST
Comment on attachment 116484 [details]
Updated patch - v1

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

New failing tests:
fast/block/float/overhanging-tall-block.html
svg/W3C-I18N/g-dirLTR-ubNone.svg
platform/chromium-linux/svg/text/text-with-geometric-precision.svg
svg/W3C-SVG-1.1/animate-elem-04-t.svg
svg/W3C-I18N/text-anchor-dirNone-anchorMiddle.svg
fast/replaced/width100percent-textarea.html
svg/W3C-I18N/text-anchor-dirLTR-anchorStart.svg
svg/W3C-SVG-1.1/animate-elem-05-t.svg
svg/W3C-I18N/text-anchor-dirNone-anchorStart.svg
svg/W3C-I18N/text-anchor-dirLTR-anchorMiddle.svg
svg/W3C-I18N/text-anchor-dirLTR-anchorEnd.svg
svg/W3C-I18N/text-anchor-dirNone-anchorEnd.svg
svg/W3C-I18N/g-dirLTR-ubOverride.svg
svg/W3C-I18N/g-dirRTL-ubNone.svg
svg/W3C-I18N/g-dirRTL-ubOverride.svg
Comment 49 Nikolas Zimmermann 2011-11-29 01:29:59 PST
I'm going to land this patch now, as discussed with the Gtk/Qt/Chromium gardeners, which are all around on IRC to rebaseline their platforms. Thanks for this great coordination!
Comment 50 Nikolas Zimmermann 2011-11-29 01:41:57 PST
Landed in r101342, now waiting for the bots to cycle...
Comment 51 Csaba Osztrogonác 2011-11-29 03:22:22 PST
(In reply to comment #50)
> Landed in r101342, now waiting for the bots to cycle...

Qt rebaselines landed in:
http://trac.webkit.org/changeset/101344
http://trac.webkit.org/changeset/101346
Comment 52 Csaba Osztrogonác 2011-11-29 03:41:07 PST
Created attachment 116939 [details]
failing tests on 64 bit QtWebKit after r101342

I skipped them: http://trac.webkit.org/changeset/101353
Comment 53 Nikolas Zimmermann 2011-11-29 04:41:32 PST
Landed Win baseline update in r101358.
Comment 54 Nikolas Zimmermann 2011-11-29 05:31:11 PST
Landed lion rebaselines in r101363.
Comment 56 Nikolas Zimmermann 2011-12-02 14:54:41 PST
(In reply to comment #55)
> These still appear to be failing on lion:
> http://build.webkit.org/results/Lion%20Intel%20Release%20(Tests)/r101515%20(3080)/svg/W3C-I18N/text-anchor-inherited-dirLTR-anchorStart-pretty-diff.html

I'm working on a fix for that, stay tuned.
Comment 57 Nikolas Zimmermann 2012-02-08 01:54:36 PST
The SVG path d attribute output is fixed, closing this particular bug now.