RESOLVED FIXED Bug 47467
SVG <path> DRT dumps have rounding problems across platforms
https://bugs.webkit.org/show_bug.cgi?id=47467
Summary SVG <path> DRT dumps have rounding problems across platforms
Nikolas Zimmermann
Reported 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.
Attachments
Patch (34.58 KB, patch)
2010-10-10 03:31 PDT, Nikolas Zimmermann
no flags
Patch v1 (163.37 KB, patch)
2011-11-14 04:42 PST, Nikolas Zimmermann
no flags
Patch v2 (164.50 KB, patch)
2011-11-14 07:12 PST, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Patch v3 (201.13 KB, patch)
2011-11-15 05:49 PST, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Patch v4 (1.11 MB, patch)
2011-11-16 00:47 PST, Nikolas Zimmermann
no flags
Patch v5 (1.14 MB, patch)
2011-11-16 04:38 PST, Nikolas Zimmermann
gustavo: commit-queue-
Patch v6 (deleted)
2011-11-16 07:14 PST, Nikolas Zimmermann
gyuyoung.kim: commit-queue-
Patch v7 (deleted)
2011-11-16 13:48 PST, Nikolas Zimmermann
no flags
Patch v8 (deleted)
2011-11-17 01:52 PST, Nikolas Zimmermann
no flags
Patch v9 (deleted)
2011-11-18 08:13 PST, Nikolas Zimmermann
no flags
Patch v10 (deleted)
2011-11-18 10:28 PST, Nikolas Zimmermann
gyuyoung.kim: commit-queue-
Patch v11 (deleted)
2011-11-18 11:19 PST, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Updated patch - v1 (62.89 KB, patch)
2011-11-24 00:44 PST, Nikolas Zimmermann
zherczeg: review+
zherczeg: commit-queue-
Updated patch - v1 (Mac rebaseline) (deleted)
2011-11-24 00:45 PST, Nikolas Zimmermann
webkit.review.bot: commit-queue-
Updated patch - v1 (Qt rebaseline) (1.06 MB, patch)
2011-11-24 04:03 PST, Csaba Osztrogonác
no flags
failing tests on 64 bit QtWebKit after r101342 (188.52 KB, application/octet-stream)
2011-11-29 03:41 PST, Csaba Osztrogonác
no flags
Nikolas Zimmermann
Comment 1 2010-10-10 03:31:44 PDT
WebKit Review Bot
Comment 2 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.
Nikolas Zimmermann
Comment 3 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.
Dirk Schulze
Comment 4 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?
Dirk Schulze
Comment 5 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.
Nikolas Zimmermann
Comment 6 2011-11-11 07:28:17 PST
*** Bug 23927 has been marked as a duplicate of this bug. ***
Nikolas Zimmermann
Comment 7 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.
Nikolas Zimmermann
Comment 8 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!
WebKit Review Bot
Comment 9 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.
Philippe Normand
Comment 10 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?
Nikolas Zimmermann
Comment 11 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.
Nikolas Zimmermann
Comment 12 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.
Nikolas Zimmermann
Comment 13 2011-11-14 07:12:36 PST
Created attachment 114942 [details] Patch v2 Attempt to fix build/style issues.
Nikolas Zimmermann
Comment 14 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.
WebKit Review Bot
Comment 15 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
Nikolas Zimmermann
Comment 16 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/
Nikolas Zimmermann
Comment 17 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 :-)
WebKit Review Bot
Comment 18 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
Nikolas Zimmermann
Comment 19 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.
Nikolas Zimmermann
Comment 20 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!
Gustavo Noronha (kov)
Comment 21 2011-11-16 04:49:45 PST
WebKit Review Bot
Comment 22 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
Gyuyoung Kim
Comment 23 2011-11-16 05:16:31 PST
Early Warning System Bot
Comment 24 2011-11-16 06:10:41 PST
Nikolas Zimmermann
Comment 25 2011-11-16 07:14:19 PST
Created attachment 115376 [details] Patch v6 Next try.
Gyuyoung Kim
Comment 26 2011-11-16 07:46:57 PST
WebKit Review Bot
Comment 27 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
Gustavo Noronha (kov)
Comment 28 2011-11-16 08:21:32 PST
Nikolas Zimmermann
Comment 29 2011-11-16 13:48:20 PST
Created attachment 115442 [details] Patch v7 New patch for phil/ossy. to try.
Nikolas Zimmermann
Comment 30 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.
Nikolas Zimmermann
Comment 31 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.
Philippe Normand
Comment 32 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}
Philippe Normand
Comment 33 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}
Nikolas Zimmermann
Comment 34 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.
Nikolas Zimmermann
Comment 35 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.
Nikolas Zimmermann
Comment 36 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?
Gyuyoung Kim
Comment 37 2011-11-18 11:04:02 PST
Nikolas Zimmermann
Comment 38 2011-11-18 11:19:53 PST
Created attachment 115838 [details] Patch v11 Fix gtk/efl builds.
WebKit Review Bot
Comment 39 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
Nikolas Zimmermann
Comment 40 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.
Nikolas Zimmermann
Comment 41 2011-11-24 00:45:53 PST
Created attachment 116485 [details] Updated patch - v1 (Mac rebaseline)
Nikolas Zimmermann
Comment 42 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.
Adam Barth
Comment 43 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.
Zoltan Herczeg
Comment 44 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.
Zoltan Herczeg
Comment 45 2011-11-24 01:45:07 PST
Niko, please contact with the bot managers before landing this patch, it will likely break things.
Csaba Osztrogonác
Comment 46 2011-11-24 04:03:55 PST
Created attachment 116494 [details] Updated patch - v1 (Qt rebaseline)
WebKit Review Bot
Comment 47 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
WebKit Review Bot
Comment 48 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
Nikolas Zimmermann
Comment 49 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!
Nikolas Zimmermann
Comment 50 2011-11-29 01:41:57 PST
Landed in r101342, now waiting for the bots to cycle...
Csaba Osztrogonác
Comment 51 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
Csaba Osztrogonác
Comment 52 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
Nikolas Zimmermann
Comment 53 2011-11-29 04:41:32 PST
Landed Win baseline update in r101358.
Nikolas Zimmermann
Comment 54 2011-11-29 05:31:11 PST
Landed lion rebaselines in r101363.
Nikolas Zimmermann
Comment 56 2011-12-02 14:54:41 PST
Nikolas Zimmermann
Comment 57 2012-02-08 01:54:36 PST
The SVG path d attribute output is fixed, closing this particular bug now.
Note You need to log in before you can comment on or make changes to this bug.