Bug 18994

I couldn't reproduce with r33029 nightly.

I had the same reply from someone from the Qt/Trolltech team. My guess is that there is some incompatibility with my system (openSUSE 10.3, X11 7.2-143.11, libXrender-7.2-65, gcc 4.2 24, NVIDIA-Linux-x86_64-169.09). r33026 and 33030 don't seem to contain changes which might influence this. I'd like to put some log statements into FancyToolbar.prototype.animateToolbar() to see where it clogs. How do I do that?

(In reply to comment #2) > I'd like to put some log statements into > FancyToolbar.prototype.animateToolbar() to see where it clogs. How do I do > that? > console.log() will log to the JS console, but depending on which port you're using, that may require some minor work in ChromeClient to actually get logged somewhere.

Okay, I found it. On my system, LANG is "de_DE.UTF-8". So I see this in the console: this.toolbarElement.style.opacity=0,1 parseFloat("0,1") returns 0 (it expects "0.1"), so the value stays at "0.1" plus I get a nice busy loop. When I set LANG=C, the example works as expected. So the problem is that float values returned by querying properties are formatted according to the locale. I don't think that this is a good idea.

These numbers are formatted with vsnprintf in WebCore::String::format(). Should be easy to fix by using vsnprintf_l - but does Linux have this or equivalent functionality?

I couldn't find anything :/ I doubt that glibc offers this after finally offering automatic locale specific formatting. Why does the parseDouble() function expect "." even when the locale specifies ","? Also, I wonder what a fix in String::format() would break. Will it work for JavaScript, too? Like: var x = 1.5; var s = "" + x; assert "1.5" == s; My next idea was to set LANG/LC_ALL in the startup script but that would break if someone would embed WebKit in an application. Or we could patch the field "decimal_point" in "struct lconv". But that might introduce even more hideous bugs during embedding, especially when threads are used. Which leads to another question: Might the web developer want to *show* float values to the user? In this case, which locale should be used to format them? C? The locale of the page? The locale in which the browser runs? Argh... a Pandora Bug! If everyone was using String::number(double) to format double values, we could replace "," by "." in there. But that isn't very safe as long as String::format() is not private (someone might try to format double values manually, for example in a more complex format string). The GNU header file locale.h mentions that there is some proof of concept code to allow per-thread locales. But I'm not sure how fast this is (i.e. getting the current locale, replace it with C, set it back after the double value has been formatted). *sigh* It would be easier if style.opacity would simply return a double value :) I'll keep my eyes open for a solution for this but half an hour on Google didn't turn anything really useful up. Maybe ask the glibc maintainers for help?

(In reply to comment #6) > Why does the parseDouble() function expect "." even when the locale specifies > ","? Well, a script downloaded from the Web can always pass a double as a literal string, so we should worry about that case, too, not just round-tripping. User locale should have no effect on scripts, except for methods that are specifically documented as locale-sensitive. > Also, I wonder what a fix in String::format() would break. On Mac OS X, locale does not affect printf/scanf by default, so such a fix is unlikely to cause any problems. > Will it work for JavaScript, too? No, Platform::String is not used in JavaScriptCore. I am not aware of similar bugs remaining in JavaScriptCore (although there used to be some, see bug 16162). > My next idea was to set LANG/LC_ALL in the startup script but that would break > if someone would embed WebKit in an application. Precisely. > Or we could patch the field "decimal_point" in "struct lconv". But that might > introduce even more hideous bugs during embedding, especially when threads are > used. Ugh, I was assuming that locale was per-thread on Linux, and thought of this as the most promising approach. > Which leads to another question: Might the web developer want to *show* float > values to the user? In this case, which locale should be used to format them? > C? The locale of the page? The locale in which the browser runs? ECMAScript-262 talks about "host environment's current locale" (see e.g. 15.5.4.9), which is what JavaScriptCore does. Other specifications have different approaches, which are not always explicitly spelled out and which we don't always fully implement yet. But let's keep discussion in this bug focused on the specific issue at hand! > I'll keep my eyes open for a solution for this but half an hour on Google > didn't turn anything really useful up. Maybe ask the glibc maintainers for > help? Sounds like a good idea. I would also expect that Qt/gtk/wx have their solutions for this issue, which we could potentially use.

See also: bug 18985.

(In reply to comment #6) > I couldn't find anything :/ I doubt that glibc offers this after finally > offering automatic locale specific formatting. > > Why does the parseDouble() function expect "." even when the locale specifies > ","? > > Also, I wonder what a fix in String::format() would break. Will it work for > JavaScript, too? Like: > > var x = 1.5; > var s = "" + x; > assert "1.5" == s; > > My next idea was to set LANG/LC_ALL in the startup script but that would break > if someone would embed WebKit in an application. > > Or we could patch the field "decimal_point" in "struct lconv". But that might > introduce even more hideous bugs during embedding, especially when threads are > used. > > Which leads to another question: Might the web developer want to *show* float > values to the user? In this case, which locale should be used to format them? > C? The locale of the page? The locale in which the browser runs? > > Argh... a Pandora Bug! > > If everyone was using String::number(double) to format double values, we could > replace "," by "." in there. But that isn't very safe as long as > String::format() is not private (someone might try to format double values > manually, for example in a more complex format string). > > The GNU header file locale.h mentions that there is some proof of concept code > to allow per-thread locales. But I'm not sure how fast this is (i.e. getting > the current locale, replace it with C, set it back after the double value has > been formatted). > > *sigh* It would be easier if style.opacity would simply return a double value > :) > > I'll keep my eyes open for a solution for this but half an hour on Google > didn't turn anything really useful up. Maybe ask the glibc maintainers for > help? > It's not only the decimal-point: (from man printf) For some numeric conversions a radix character (‘decimal point’) or thousands’ grouping character is used. The actual character used depends on the LC_NUMERIC part of the locale. The POSIX locale uses ‘.’ as radix character, and does not have a grouping character. Thus, printf("%’.2f", 1234567.89); results in ‘1234567.89’ in the POSIX locale, in ‘1234567,89’ in the nl_NL locale, and in ‘1.234.567,89’ in the da_DK locale.

(In reply to comment #5) > These numbers are formatted with vsnprintf in WebCore::String::format(). Should > be easy to fix by using vsnprintf_l - but does Linux have this or equivalent > functionality? Hmm, as far as I can see the numbers parsed by the lexer of the CSS parser are formatted with WebCore::charactersToDouble. This function, implemented in platform/text/String.cpp uses KJS::strtod for the conversion, which is locale safe. In fact I'm having trouble to reproduce this bug now with trunk.

See e.g. CSSPrimitiveValue::cssText(), which uses String::format(). I think that this should still be reproducible on Linux.

As of rev 34662, the bug is fixed on Linux. Did you add a test case to make sure opacity is never formatted with a comma?

(In reply to comment #12) > As of rev 34662, the bug is fixed on Linux. I still get the problem, like described in the first comment. And String::format() is a general problem. Some more files where String::format is used with float: FTPDirectoryDocument.cpp PathCairo.cpp SVGUseElement.cpp SVGUseElement: see https://bugs.webkit.org/show_bug.cgi?id=18985

For the Qt port we could implement String::format using QString::vsprintf.

For the Qt port we have switched to the locale-independent QString::vsprintf in String::format in revision 35712

Eventually, we need a cross-platform solution. Although this doesn't affect Safari, this is still a problem even on Mac OS X for 3rd party clients that may call setlocale().

Alexey, could we use the string formatting/parsing features provided by ICU here? This would also have the benefit of working directly in UChars.

Created attachment 23471 [details] Deprecate String::format() Use UString::from() instead of String::format() in String::number() functions where possible. Start to replace uses of String::format() in WebCore with StringBuilder and direct concatenation. As a side-effect, several incorrect or needless string conversions that were done by String::format() callers are now avoided. String::format() should not be used in new code. Note: These changes need to be checked for regressions with the Mac layout tests before going in.

This patch converts most, but not all String::format() callers to instead build strings directly. There are still ~50 calls yet to be ported (listed below) though the remaining ones are low-risk (no float/double formatting). It'd be great to drop String::format() completely or move it well out of the way some day since it clearly leads to sloppy code. WebCore/css/CSSParser.cpp: String str = String::format("%06d", (int)(value->fValue+.5)); WebCore/dom/StyledElement.cpp: color = String::format("#%02x%02x%02x", colors[0], colors[1], colors[2]); WebCore/page/Console.cpp: String message = String(title) + String::format(": %.0fms", elapsed); WebCore/storage/DatabaseTracker.cpp: filename = pathByAppendingComponent(originPath, String::format("%016llx.db", seq)); WebCore/storage/DatabaseTracker.cpp: if (!addDatabase(origin, name, String::format("%016llx.db", seq))) WebCore/loader/FTPDirectoryDocument.cpp: return String::format("%.2f KB", static_cast<float>(bytes)/1000); WebCore/loader/FTPDirectoryDocument.cpp: return String::format("%.2f MB", static_cast<float>(bytes)/1000000); WebCore/loader/FTPDirectoryDocument.cpp: return String::format("%.2f GB", static_cast<float>(bytes)/1000000000); WebCore/loader/FTPDirectoryDocument.cpp: timeOfDay = String::format(", %i:%02i AM", hour, fileTime.tm_min); WebCore/loader/FTPDirectoryDocument.cpp: timeOfDay = String::format(", %i:%02i PM", hour, fileTime.tm_min); WebCore/loader/FTPDirectoryDocument.cpp: dateString = String::format("%s %i, %i", months[month], fileTime.tm_mday, fileTime.tm_year); WebCore/loader/FTPDirectoryDocument.cpp: dateString = String::format("%s %i, %i", months[month], fileTime.tm_mday, now.tm_year); WebCore/loader/archive/cf/LegacyWebArchive.cpp: String iframeMarkup = String::format("<iframe frameborder=\"no\" marginwidth=\"0\" marginheight=\"0\" width=\"98%%\" height=\"98%%\" src=\"%s\"></iframe>", WebCore/platform/qt/PlatformKeyboardEventQt.cpp: return String::format("U+%04X", toupper(keyCode)); WebCore/platform/wx/KeyboardEventWx.cpp: return String::format("U+%04X", toupper(keyCode)); WebCore/platform/gtk/KeyEventGtk.cpp: return String::format("U+%04X", gdk_keyval_to_unicode(gdk_keyval_to_upper(keyCode))); WebCore/platform/mac/KeyEventMac.mm: return String::format("U+%04X", toASCIIUpper(c)); WebCore/platform/sql/SQLiteDatabase.cpp: executeCommand(String::format("PRAGMA synchronous = %i", sync)); WebCore/platform/win/ClipboardUtilitiesWin.cpp: append(result, String::format(header, startHTMLOffset, endHTMLOffset, startFragmentOffset, endFragmentOffset).utf8()); WebCore/platform/win/GDIObjectCounter.cpp: init(String::format("%s (%p)", className.latin1().data(), instance)); WebCore/platform/win/Language.cpp: computedDefaultLanguage = String::format("%s-%s", languageName.latin1().data(), countryName.latin1().data()); WebCore/platform/win/KeyEventWin.cpp: return String::format("U+%04X", toASCIIUpper(keyCode)); WebCore/platform/text/String.cpp:String String::format(const char *format, ...) WebCore/platform/text/String.cpp: return String::format("%d", n); WebCore/platform/text/String.cpp: return String::format("%u", n); WebCore/platform/text/String.cpp: return String::format("%ld", n); WebCore/platform/text/String.cpp: return String::format("%lu", n); WebCore/platform/text/String.cpp: return String::format("%I64i", n); WebCore/platform/text/String.cpp: return String::format("%lli", n); WebCore/platform/text/String.cpp: return String::format("%I64u", n); WebCore/platform/text/String.cpp: return String::format("%llu", n); WebCore/platform/text/String.cpp: return String::format("%.6lg", n); WebCore/platform/graphics/cg/ImageBufferCG.cpp: return String::format("data:%s;base64,%s", mimeType.utf8().data(), out.data()); WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm: String text = String::format("%1.2f", frameRate); WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp: String text = String::format("%1.2f", frameRate); WebCore/platform/graphics/cairo/PathCairo.cpp: string += String::format("M %.2f,%.2f", WebCore/platform/graphics/cairo/PathCairo.cpp: string += String::format("L %.2f,%.2f", WebCore/platform/graphics/cairo/PathCairo.cpp: string += String::format("C %.2f,%.2f,%.2f,%.2f,%.2f,%.2f", WebCore/platform/graphics/Color.cpp: return String::format("#%02X%02X%02X%02X", red(), green(), blue(), alpha()); WebCore/platform/graphics/Color.cpp: return String::format("#%02X%02X%02X", red(), green(), blue()); WebCore/rendering/RenderMedia.cpp: return String::format("%02d:%02d:%02d", hours, minutes, seconds); WebCore/rendering/RenderTreeAsText.cpp: String hex = String::format("\\x{%X}", u); WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp: return String::format("%s %s", name.sysname, name.machine); WebKit/win/WebKitStatistics.cpp: append(vector, String::format("%4u", current->second)); WebKit/win/WebCoreSupport/WebInspectorClient.cpp: // FIXME: The series of appends should be replaced with a call to String::format() WebKit/win/WebView.cpp: osVersion = String::format("Windows NT %d.%d", versionInfo.dwMajorVersion, versionInfo.dwMinorVersion); WebKit/win/WebView.cpp: osVersion = String::format("Windows %d.%d", versionInfo.dwMajorVersion, versionInfo.dwMinorVersion); WebKit/win/WebView.cpp: m_userAgentStandard = String::format("Mozilla/5.0 (Windows; U; %s; %s) AppleWebKit/%s (KHTML, like Gecko)%s%s", osVersion().latin1().data(), defaultLanguage().latin1().data(), webKitVersion().latin1().data(), (m_applicationName.length() ? " " : ""), m_applicationName.latin1().data());

Comment on attachment 23471 [details] Deprecate String::format() I think it's a great idea to do this and the patch looks great. But we need to make sure the new idiom is at least as fast as the old or faster. If we're going to write new code for many call sites I want to be sure it's designed to be fast enough that we won't have to visit them all again to rewrite them again. + String message = "Unsafe JavaScript attempt to access frame with URL "; + message += targetURL.string(); + message += " from frame with URL "; + message += originURL.string(); + message += ". Domains, protocols and ports must match.\n"; + return message; This is a bad idiom for performance. The String class is very slow to append to, so changing calls that previously used String::format to code that uses String appends is not generally a good idea, unless the code is not at all performance critical. I'm concerned that overall this is going to slow things down. Idioms that are fast are using StringBuffer or Vector<UChar> to create the string and then String::adopt to make a String out of it. - text = String::format("%.6lg%%", m_value.num); + text = String::number(m_value.num) + "%"; Same thing here. Given the String implementation, this is a particularly inefficient idiom and I'd prefer not to add lots of instances of it without considering performance. - String m_errorMessages; + StringBuilder m_errorMessages; This is the right idea! Although I do slightly prefer Vector<UChar> (see below). String String::number(unsigned n) { +#if USE(JSC) + return String(UString::from(n)); +#else return String::format("%u", n); +#endif } The above is not efficient because it involves allocating an unnecessary temporary UString. We need to make these numeric conversions operations efficient. I think we need a form of number formatting that appends to a Vector<UChar> or a StringBuffer or both, and we also want String::number() to be efficient for the occasions where we really do need to create an entire String out of a number. We should consider what we need to do in JavaScriptCore to offer the right kind of efficient numeric conversion for use elsewhere without requiring allocation of a UString for the result. + // TODO: Use UString::from() return String::format("%lu", n); Why is that still a TODO? - String statusLine = String::format("HTTP %lu OK\n", m_resourceResponse.httpStatusCode()); + String statusLine = "HTTP "; + statusLine += String::number(static_cast<unsigned>(m_resourceResponse.httpStatusCode())); + statusLine += " OK\n"; stringBuilder.append(statusLine.characters(), statusLine.length()); Here we need to append right to the stringBuilder local variable, not go out of our way to instead create a String. I also don't understand the reason for the static_cast. That seems wrong. I think we should standardize on either StringBuilder or Vector<UChar> (sorry that there are two!). I personally prefer Vector<UChar> because StringBuilder forces us to allocate separate strings for each piece, whereas Vector<UChar> uses a single buffer. On the other hand, StringBuilder has a more attractive name and is more clearly for this purpose. Maybe we can improve StringBuilder so that it is as efficient as Vector<UChar> when the pieces to append aren't already in WebCore::String objects and then we don't have a difficult choice. Anyway, we would add an operation for appending a formatted number to a Vector<UChar> or StringBuilder. And then convert the String::format call sites to that approach. Sorry, I'm not trying to be difficult; but if we're going to rewrite these I'd really like the new idiom to be as efficient as possible. I hope this doesn't make the task take too long! I'd love to hear your thoughts.

(In reply to comment #20) > Idioms that are fast are using StringBuffer or Vector<UChar> to create the > string and then String::adopt to make a String out of it. How about using TextStream.h? Although it uses snprintf() now it looks like it has good optimisation potential, as long as code like this isn't objectionable: ts << "text run at (" << run.m_x << "," << run.m_y << ") width " << run.m_width; Right now TextStream is only used by RenderTreeAsText and a couple of places in SVG. If we go this way we'll need to extend TextStream to choose the decimal precision when formatting doubles etc.

(In reply to comment #21) > How about using TextStream.h? Although it uses snprintf() now it looks like it > has good optimisation potential, as long as code like this isn't objectionable: Yes, that'd be OK. I do worry that we end up with too many different options, between Vector<UChar>, StringBuffer, and TextStream. I was hoping to phase out TextStream. If we can make it efficient and a great option then I think many will like using the stream-style syntax. I don't think the name of TextStream is very clear -- the fact that it's for constructing a string seems subtle. Maybe we can add the streaming syntax to StringBuffer instead?

*** Bug 26984 has been marked as a duplicate of this bug. ***

This came up on the Chrome bug tracker: http://code.google.com/p/chromium/issues/detail?id=22782 Same issue as above. Since we're not the Qt port, String::format is still locale-dependent. I will try to create a layout test for this.

Created attachment 40411 [details] patch v1

I've attached a patch for consideration -- it doesn't quite work yet, the style's wrong, but feedback on the general approach is appreciated. I'm new to this WebKit stuff.

Eric explained to me the multitude of ways in which I fail. I will post a reasonable patch later, please disregard that last one.

Created attachment 40646 [details] patch

Ok, please take a look at my new patch. I will now try to verify the test fails on a WebKitGtk build.

Created attachment 40648 [details] patch

Created attachment 40649 [details] add a regression test

Since last review: - eliminated tabs - setLocale -> setPOSIXLocale - reset locale in platform-specific resetter functions rather than in layout test

Created attachment 40662 [details] add a regression test

I have now verified this fails on Linux in the expected way. I also upped the locale buffer size on recommendation from kov in #webkit-gtk. I think this is ready to go.

Comment on attachment 40662 [details] add a regression test I would have wrapped: setlocale(LC_ALL, ""); into some shared: resetToDefaultPOSIXLocale() function or similar. But in general this looks fine.

Out of curiosity, I ran the existing layout tests from a German (commas for decimals) locale under Chrome on Linux. I had 412 tests fail (but some of them are due to differing plugins on my system vs our tester bots...); many of the locale-caused failures were under svg, but also a random set of others. Here are a few selections, just to give you a feel for it: LayoutTests/fast/text/shadow-translucent-fill.html = FAIL LayoutTests/transforms/2d/transform-2d.html = FAIL LayoutTests/animations/matrix-anim.html = FAIL

Comment on attachment 40662 [details] add a regression test Rejecting patch 40662 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11396 test cases. fast/css/opacity-float.html -> failed Exiting early after 1 failures. 5136 tests run. 90.65s total testing time 5135 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output

It seems the reason this failed is that OS X is vulnerable to the same problem other platforms are. I suspect the reason it hasn't happened in the past is apparently people don't call setlocale() in normal OS X apps. (?) So I think the test can't be checked in until WebKit is setlocale() safe.

The test can be checked in with an expected failure.

Created attachment 40749 [details] add a regression test

Comment on attachment 40749 [details] add a regression test with mac expected-fail baseline

Created attachment 40753 [details] start patching out CSS floats with a Vector<UChar>

Comment on attachment 40753 [details] start patching out CSS floats with a Vector<UChar> Hey Darin, Could you take a glance at this and see if it's in the direction of where you were suggesting Alp go? Obviously I'd need to relocate these functions to the appropriate places. Note that dtoa works with chars so we need to do a conversion step when we use it, so appending a letter at a time to the uchar vector doesn't seem so horrible. However, the matrix line that I highlighted in the patch probably does call for some sort of builder class. I looked at the existing classes you mentioned but it's not clear to me what the right thing to do is.

Comment on attachment 40753 [details] start patching out CSS floats with a Vector<UChar> General direction looks right here. I agree there's no obvious way to use a builder class for the matrix case. Instead, I suggest you write the code out longhand, including some way to reserve the capacity for the string, and then consider how to factor it into some kind of builder afterward. I think we should consider eliminating String::format entirely.

Comment on attachment 40749 [details] add a regression test Clearing flags on attachment: 40749 Committed r49253: <http://trac.webkit.org/changeset/49253>

All reviewed patches have been landed. Closing bug.

Reopening because the commitbot is overzealous -- I just had provided a test. :)

Yeah, the commit-queue has no concept of "land this and keep the bug open". It assumes a one-patch-per-bug workflow. There is at least bug 28230 filed on this.

Created attachment 40828 [details] use dtoa directly in CSS units

Comment on attachment 40828 [details] use dtoa directly in CSS units This fixes both of the tests I recently added, but doesn't resolve this bug completely. I'd prefer to write more failing tests before I speculatively touch other parts of the code. LayoutTests/fast/css/opacity-float.html LayoutTests/fast/css/large-number-round-trip.html Note that this changes the output for a few other tests, which I'll need to regenerate baselines for. The TextStream logging functions have slightly different output requirements than the CSS output ones do. :( The changes will change DumpRenderTree outputs like this: RenderBody {BODY} at (8,8) size 784x584 - RenderBlock {DIV} at (0,0) size 784x152 [textFillColor=#800080] [textStrokeWidth=2.00] + RenderBlock {DIV} at (0,0) size 784x152 [textFillColor=#800080] [textStrokeWidth=2] RenderText {#text} at (0,1) size 755x149 text run at (0,1) width 612: "Purple\x{300} fill, black stroke," text run at (0,77) width 755: "complex text, black underline" - RenderBlock {DIV} at (0,152) size 784x152 [textStrokeColor=#FFA500] [textStrokeWidth=1.33] + RenderBlock {DIV} at (0,152) size 784x152 [textStrokeColor=#FFA500] [textStrokeWidth=1.3] I don't know what you think of that.

Having thought about this for a day (I've had the patch for a while), I fear that it will be difficult to share code for this appendFloat function; the output required by CSS (max 6 significant digits, no exponents) is different than the output apparently produced by TextStream (min 2 digits after the decimal). I can add more options to appendFloat but maybe that gets us back into performance worry territory. Another option is to just write a specific implementation of it for each call site.

Comment on attachment 40828 [details] use dtoa directly in CSS units > +void appendDouble(Vector<UChar>& vector, double value, int digits) > +{ > + // From the CSS specification section titled "Integers and real numbers", > + // real numbers are only formatted as [sign] [digits] "." [digits], with > + // no special handling of exponents, NaN, etc. This is a confusing thing to say here. This says what the specification says, but not what the function does. What does this function do for NaN and exponents and such? Should it simply never be called on such numbers? Don't leave us hanging! I think the name of this function should contain the words CSS if the behavior is something driven by the CSS specification. The name appendDouble makes it sound more general-purpose. Or maybe there is a term of art for this type of numeric formatting? Sounds fine for CSS, but doesn't this change the behavior of the TextStream operators? Why is that OK? Do we have test coverage for those? Including edge cases like exponents, infinity, and NAN? > + vector.reserveCapacity(vector.size() + length + 2); Where does the 2 come from here? From reading the code below it looks like you can have a lot more characters than "length" added. Can we do the reserveCapacity completely accurately instead of approximately? Do we have test cases that cover all the branches of the appendDouble function? I'll say review- because of some of the comments above, but this looks really good and on the right track.

(In reply to comment #51) > I can add more options to appendFloat but maybe that gets us back into > performance worry territory. I'm not too worried about that. More options would probably be OK. It's more of a clarity than performance issue, in my opinion. As far as regression test output is concerned, the best approach is to not change that format if possible so we don't have to revise all our tests.

Blah, I keep going in circles on this. UString.cpp's concatenate() is almost exactly what I want (it's where I got the dtoa handling from) except that it does exponents and works with UStrings. I could maybe factor that out into something that writes into a char* buffer (which is what it does, then convert to a wide buffer as the last step) but then I'd also need to add special-casing for whether we want exponents or not. To handle the TextStream case fully it also needs to behave differently for significant digits -- it's basically the difference between how %f and %g in printf handle the precision argument. I think I bit off more than I should have in my last patch; I will write a new one that *only* touches the CSS floats (what this bug was initially about) and not float handling in general. Since those floats need special handling (no exponents), it seems reasonable to me to have a special function for it. I think even in that case I'll need new baselines for a bunch of SVG tests where the 10e-6th digit of a float changes by one due to rounding. :( Regarding reserving the correct buffer from the start: I think it could be done, at the cost of uglifying the code a bit. I will write it and see what you think.

(In reply to comment #54) > I will write a new > one that *only* touches the CSS floats (what this bug was initially about) and > not float handling in general. Since those floats need special handling (no > exponents), it seems reasonable to me to have a special function for it. I > think even in that case I'll need new baselines for a bunch of SVG tests where > the 10e-6th digit of a float changes by one due to rounding. Sounds good. > Regarding reserving the correct buffer from the start: I think it could be > done, at the cost of uglifying the code a bit. I will write it and see what > you think. Thanks for taking a crack at it. Probably not super-important.

Created attachment 40916 [details] use dtoa directly in CSS units

Comment on attachment 40916 [details] use dtoa directly in CSS units This is my current plan. Only four tests fail after this patch: all of them are due to the %fs in the matrix printing. The size-reservation code is a bit silly; I wonder if it'd be better to just always reserve 8 characters (the expected number + decimal point + sign) and have CSS where the values are obscene (like 20 digits long) cause us to realloc.

(In reply to comment #57) > This is my current plan. Only four tests fail after this patch: all of them > are due to the %fs in the matrix printing. Looks good. > The size-reservation code is a bit silly; I wonder if it'd be better to just > always reserve 8 characters (the expected number + decimal point + sign) and > have CSS where the values are obscene (like 20 digits long) cause us to > realloc. I think it would be better. Sorry for steering you wrong before!

Created attachment 40923 [details] use dtoa directly in CSS units

Still three tests failing, all under transforms/2d. Two of them are that the last digit in matrices changes by 1. (Rounding error.) The last is transforms/2d/transform-accuracy: transform "rotate(-10deg) rotate(10deg) rotate(360deg) rotate(-360deg) rotate(360deg)" expected "matrix(1, 0, 0, 1, 0, 0)", got "matrix(0.999999, -0.000000000000000244929, 0.000000000000000244929, 0.999999, 0, 0)" : FAIL I think I could maybe introduce a rounding step into the testing library they share to work around it. I think it's a bit sketchy to be doing equality tests on floats.

(PS: the %fs in the matrix code earlier were a red herring: matrices are printed as a list of CSS floats so this is still the code path they use.)

Created attachment 40924 [details] use dtoa directly in CSS units

(In reply to comment #62) > Created an attachment (id=40924) [details] > use dtoa directly in CSS units Maybe I missed something but where did you run the test, on Gtk? Mac?

(In reply to comment #63) > Maybe I missed something but where did you run the test, on Gtk? Mac? Which test? You mean the ones I rebaselined in the patch? I did them on a Mac.

Created attachment 41052 [details] stringify CSS units manually to match CSS spec

Comment on attachment 41052 [details] stringify CSS units manually to match CSS spec I had a flash of inspiration over the weekend. Since we explicitly want to round to 6 figures, we can skip all the dtoa business. New patch is significantly simpler, makes two tests start passing, and I only had to rebaseline one test due to rounding differences.

Comment on attachment 41052 [details] stringify CSS units manually to match CSS spec Looks good. > + // For compatibility with existing layout tests, we target 6 digits of precision. This seems strange to me, even though we've been discussing this one back and forth. I hope that eventually we can make a choice based on something other than "layout tests". Perhaps the best way to word the comment would be to say "compatibility with what was returned by older versions of WebKit"? > + const int digits = 6; This name is too terse. Maybe it should be digitsAfterDecimalPoint? > + long long rounded = round(fabs(value) * 1000000.0); The best function to use to round a double and put the result in a long long would be llround, not round. What makes long long big enough? If you are trying to avoid overflow, long long is not sufficient for that. There are definitely values that fit in a double that are too large to fit in a long long. How does this handle super-large numbers? Do we have test cases that cover that? > + if (rounded == 0) { > + vector.append('0'); > + return; > + } Need to indent by four spaces here, not two. > + char buf[80]; > + int length = snprintf(buf, sizeof(buf), "%lld", rounded); Since snprintf has rather poor performance, it would be better to use some other code here to convert the integer to a string. Functions like UString::from in JavaScriptCore do this job, and I'd like to see this use a similar function in the String class rather than relying on snprintf. > + // Reserve an estimate of space for the number of digits we anticipate > + // along with a minus sign/initial zero/decimal point. That sounds like length + 3. > + vector.reserveCapacity(vector.size() + 2 + length); And this says 2 + length. Why doesn't the code match the comment? I'd like to see test cases for the edge cases of this code. I worry that this will do the wrong thing for extremely large and extremely small numbers -- test cases could make it clear that the behavior hasn't changed, or that it has improved. I'm going to say review- because I'd at least like you to rename "digits", but please consider the rest of the comments too.

Created attachment 41055 [details] stringify CSS units manually to match CSS spec

(In reply to comment #67) > (From update of attachment 41052 [details]) > Looks good. Thank you a ton for your patience and careful reviews. > > > + // For compatibility with existing layout tests, we target 6 digits of precision. > > This seems strange to me, even though we've been discussing this one back and > forth. I hope that eventually we can make a choice based on something other > than "layout tests". Perhaps the best way to word the comment would be to say > "compatibility with what was returned by older versions of WebKit"? Done. > > > + const int digits = 6; > > This name is too terse. Maybe it should be digitsAfterDecimalPoint? Done. > > > + long long rounded = round(fabs(value) * 1000000.0); > > The best function to use to round a double and put the result in a long long > would be llround, not round. Ah, I wasn't even aware of this function. > > What makes long long big enough? If you are trying to avoid overflow, long long > is not sufficient for that. There are definitely values that fit in a double > that are too large to fit in a long long. How does this handle super-large > numbers? Do we have test cases that cover that? It is true. Here's my thinking. One of the test cases I'm fixing here is "large-number-round-trip", which is an old test that predates me discovering this bug. (I checked it in recently since I discovered it was related.) It shows that WebKit currently gets numbers as small as 90010000 (roughly 10e8) wrong. If you're willing to assume a long long is 64 bits (call it 10e19), and my multiply-by-10e6 trick eats 6 of that exponent, I'm making the upper bound around 10e13, which is at least better than where we were before. Since we're talking about CSS units, I don't think that's too bad of a bound; the spec is silent on the bounds of these values. http://www.w3.org/TR/CSS2/syndata.html#value-def-number Using a plain long is 32 bits, or 10e9, which would bound the values to 10e3 after the rounding trick, which is clearly enough. > > + if (rounded == 0) { > > + vector.append('0'); > > + return; > > + } > > Need to indent by four spaces here, not two. This is really embarrassing. I am normally really finicky about this sort of thing. My only excuse is that the other code I work on has 2 space tabs stops. > > > + char buf[80]; > > + int length = snprintf(buf, sizeof(buf), "%lld", rounded); > > Since snprintf has rather poor performance, it would be better to use some > other code here to convert the integer to a string. Functions like > UString::from in JavaScriptCore do this job, and I'd like to see this use a > similar function in the String class rather than relying on snprintf. Oops, I missed this before I uploaded my new patch. Will investigate. > > + // Reserve an estimate of space for the number of digits we anticipate > > + // along with a minus sign/initial zero/decimal point. > > That sounds like length + 3. > > > + vector.reserveCapacity(vector.size() + 2 + length); > > And this says 2 + length. Why doesn't the code match the comment? Overzealous premature optimization (most numbers only have at most two of the three mentioned characteristics). Fixed. > I'd like to see test cases for the edge cases of this code. I worry that this > will do the wrong thing for extremely large and extremely small numbers -- test > cases could make it clear that the behavior hasn't changed, or that it has > improved. The opacity-float test changes try to. Was there anything else you had in mind? Or do you mean testing the failure cases -- when the values exceed the allowed inputs? I will upload a patch fixing the snprintf issue once I figure out a proper solution. (In my defense, the old code used snprintf for floats.)

(In reply to comment #69) > Using a plain long is 32 bits, or 10e9, which would bound the values to 10e3 > after the rounding trick, which is clearly enough. Er, that should be "clearly *not* enough".

(In reply to comment #69) > I will upload a patch fixing the snprintf issue once I figure out a proper > solution. (In my defense, the old code used snprintf for floats.) We can do that in a separate step. I don't consider it a show-stopper.

(In reply to comment #69) > > I'd like to see test cases for the edge cases of this code. I worry that this > > will do the wrong thing for extremely large and extremely small numbers -- test > > cases could make it clear that the behavior hasn't changed, or that it has > > improved. > > The opacity-float test changes try to. Was there anything else you had in > mind? Or do you mean testing the failure cases -- when the values exceed the > allowed inputs? My approach would be to have tests at the various boundaries: highest value that works, lowest that does not work, that sort of thing. Make sure all the code paths are tested, even the less important ones. I think we're OK. Not sure. I didn't re-review the tests.

Created attachment 41059 [details] stringify CSS units manually to match CSS spec

In the process of adding more tests, I discovered some bugs, so that was a wise suggestion. I rearranged opacity-float so it's more clear what it's testing (pairs of [expected, input]), so any other suggestions for that are welcome. Should I file a bug about the snprintf thing? I could write an integer->string loop myself, I suppose. My Unix-wizard officemate says there are no POSIX builtins that do this, and volunteered some code.

Created attachment 41061 [details] stringify CSS units manually to match CSS spec

Comment on attachment 41061 [details] stringify CSS units manually to match CSS spec Graahhh, now I've confused myself again. Will reinvestigate this failing test.

Comment on attachment 41061 [details] stringify CSS units manually to match CSS spec Actually, I'm kind of exhausted on this bug. From the printf docs %.6g means you show up to 6 significant digits, but then somehow it doesn't happen when your number very is close to zero and they also arbitrarily chop off trailing digits if they are 0 even when they are significant. This patch passes all the layout tests, but I had to change the precision on one, but I find it hard to care too much about it.

Comment on attachment 41061 [details] stringify CSS units manually to match CSS spec > -FAIL: read 90010000px back as 9.001e+07px, read again as 0px > +PASS: read 90010000px back as 90010000px, read again as 90010000px This succeeds only because the large number isn't large enough, right? With a still larger number we'd have some other problem. I'd like to have a test showing that we still fail with even larger numbers. > This test verifies that reading a floating-point opacity from CSS attributes gets back a properly-formatted float. Improperly handling locales that cause decimals to be written as commas might break it. > > PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS > +PASS In the future, you should use the script-tests approach to build tests like this. This style of tests that write out more about what they're testing and why they pass that you get automatically from that are superior. A good example of how to do this sort of test is LayoutTests/fast/dom/HTMLFormElement/script-tests/elements-not-in-document.js and you may be able to see from that how this test could have been done that way. > From the printf docs %.6g means you show up to 6 significant digits, but then > somehow it doesn't happen when your number very is close to zero and they also > arbitrarily chop off trailing digits if they are 0 even when they are > significant. This patch passes all the layout tests, but I had to change the > precision on one, but I find it hard to care too much about it. As I understand it, 0.00000123456 is an example with 6 significant digits. Maybe you misunderstood what it meant. But exactly matching "%.6lg" isn't the point. Matching what we used to do is a good idea, but matching other browsers or matching specifications are even more important. I think at this point the patch is OK. Here are areas for future improvement and refinement: - check on how other browsers handle formatting fractional values in CSS; how many digits of precision, etc. - figure out if we want to stick with the 6-digits-of-precision thing forever, or if we should have a different formatting rule - eliminate the use of snprintf -- as I said, JavaScriptCore has a function with the relevant code already, JSC::UString::from, so you could just port that to live in String.cpp - change the test to use script-tests for better clarity - add quite a few more kinds of numbers to the test cases - test all the different types of units -- a test should fail if we made any mistakes in the switch statements r=me

> > From the printf docs %.6g means you show up to 6 significant digits, but then > > somehow it doesn't happen when your number very is close to zero and they also > > arbitrarily chop off trailing digits if they are 0 even when they are > > significant. This patch passes all the layout tests, but I had to change the > > precision on one, but I find it hard to care too much about it. > > As I understand it, 0.00000123456 is an example with 6 significant digits. > Maybe you misunderstood what it meant. But exactly matching "%.6lg" isn't the > point. Matching what we used to do is a good idea, but matching other browsers > or matching specifications are even more important. That is my understanding of significant digits as well. But from one of my earlier iterations of the patch, I was printing 0.00000000000123456 at the end of some error-accumultating computation; the printf implementation prints 0 for that. See comment #60. This is what prompted my new approach, which chops to 6 digits effectively by rounding to the nearest 10e-6. > - check on how other browsers handle formatting fractional values in CSS; > how many digits of precision, etc. > - figure out if we want to stick with the 6-digits-of-precision thing > forever, or if we should have a different formatting rule Filed https://bugs.webkit.org/show_bug.cgi?id=30341 > - eliminate the use of snprintf -- as I said, JavaScriptCore has a function > with the relevant code already, JSC::UString::from, so you could just port that > to live in String.cpp Filed https://bugs.webkit.org/show_bug.cgi?id=30342 > - change the test to use script-tests for better clarity > - add quite a few more kinds of numbers to the test cases > - test all the different types of units -- a test should fail if we made > any mistakes in the switch statements https://bugs.webkit.org/show_bug.cgi?id=30340 I can't promise I'll fix all of those, but I will try to at least improve the test soon since I'm the one who wrote it.

Comment on attachment 41061 [details] stringify CSS units manually to match CSS spec This patch does not have a ChangeLog :)

Created attachment 41131 [details] stringify CSS units manually to match CSS spec

Comment on attachment 41131 [details] stringify CSS units manually to match CSS spec Identical patch to the one Darin r+'d above; now includes ChangeLogs.

Comment on attachment 41131 [details] stringify CSS units manually to match CSS spec Forwarding Darin's r+ to the new patch with a ChangeLog.

Comment on attachment 41131 [details] stringify CSS units manually to match CSS spec Rejecting patch 41131 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Last 500 characters of output: tTests Testing 11429 test cases. http/tests/loading/bad-server-subframe.html -> timed out Sampling process 18462 for 10 seconds with 10 milliseconds of run time between samples Sampling completed, processing symbols... Sample analysis of process 18462 written to file /Users/eseidel/Library/Logs/DumpRenderTree/HangReport.txt Exiting early after 1 failures. 8510 tests run. 552.63s total testing time 8509 test cases (99%) succeeded 1 test case (<1%) timed out 4 test cases (<1%) had stderr output

Comment on attachment 41131 [details] stringify CSS units manually to match CSS spec setting cq+ because i believe that failure was flakiness

Comment on attachment 41131 [details] stringify CSS units manually to match CSS spec actually, probably needs someone else to set cq+ since i'm not a committer

Comment on attachment 41131 [details] stringify CSS units manually to match CSS spec Clearing flags on attachment: 41131 Committed r49554: <http://trac.webkit.org/changeset/49554>

All reviewed patches have been landed. Closing bug.

Rolled out as http://trac.webkit.org/changeset/49554 due to Win and Chromium compile break.

Maybe llround is not available on Windows? If so, the fix could be to add it to <wtf/MathExtras.h>.

I didn't actually look at the Windows failure -- it might not have been llround.

I think I am going to sit on this patch until I have commit access, since breaking the tree and being unable to revert it myself is not very helpful. If anyone else would like to take up the crusade I welcome your help. Until then, I recommend "export LC_NUMERIC=en_US.UTF-8". ;)

For future reference, the Windows breakage was: CSSPrimitiveValue.cpp ..\css\CSSPrimitiveValue.cpp(698) : error C3861: 'llround': identifier not found ..\css\CSSPrimitiveValue.cpp(705) : error C3861: 'snprintf': identifier not found I'm trying to find the failed Linux logs but I'm not seeing them; maybe I am getting my failures mixed up.

Dmitri said “Win and Chromium”. I suspect it was Windows in both cases -- not sure it broke on Linux.

Right, I don't think it broke anything non-Windows.

I'll check this in. The fix for snprintf is to include <wtf/StringExtras.h>. The fix for llround is to include <wtf/MathExtras.h> and add llround to MathExtras.h alongside lround.

(In reply to comment #84) > (From update of attachment 41131 [details]) > Rejecting patch 41131 from commit-queue. > http/tests/loading/bad-server-subframe.html -> timed out Filed bug 30392 about the false failure.

We still have this bug on WebKitGtk. I noticed it on http://www.svgbasics.com/radial_gradients.html. Maybe just on SVG's <use> element. Because the example on the first comment works.

(In reply to comment #99) > We still have this bug on WebKitGtk. I noticed it on > http://www.svgbasics.com/radial_gradients.html. Maybe just on SVG's <use> > element. Because the example on the first comment works. I attempted to split this bug into smaller bugs just so it wouldn't go on forever. See comment 79. Is it a problem with element.style.opacity?

(In reply to comment #100) > (In reply to comment #99) > > We still have this bug on WebKitGtk. I noticed it on > > http://www.svgbasics.com/radial_gradients.html. Maybe just on SVG's <use> > > element. Because the example on the first comment works. > > I attempted to split this bug into smaller bugs just so it wouldn't go on > forever. See comment 79. Is it a problem with element.style.opacity? You find String::format("translate(%f, %f)", x().value(this), y().value(this)); and some other String::format and String::number in WebCore/SVGUseElement.cpp. This causes the wrong position in this case. But also some animations don't work like: http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-22-b.html http://www.w3.org/Graphics/SVG/Test/20061213/htmlObjectHarness/full-animate-elem-29-b.html I can't say what causes the wrong behavior on the SVG animations.

r49585 caused a regression at ziprealty.com - please see https://bugs.webkit.org/show_bug.cgi?id=32078.

Reopening. I reverted the change in r52071. I noticed is that the new tests that were added to opacity-float.html didn't pass in Firefox. I wonder if the JS library used in ziprealty.com was expecting wrong behavior. Might be something investigate when re-fixing this bug.

*** Bug 33322 has been marked as a duplicate of this bug. ***

We can fix this! We just need to go out of our way to keep behavior changes to a minimum. Someone should take a crack at it.

Since I'm responsible for most of the noise on this bug, I should probably write up my current thoughts. 1) I attached a page that shows a bunch of cases as comment #2 on https://bugs.webkit.org/show_bug.cgi?id=30341 if you intend to change WebKit's behavior, you should probably check whether you match other browsers using that page, as well as the real website that prompted reverting my previous attempt at this bug. I noticed Firefox is different than WebKit both before and after my change. (But see my comment #5 on there.) 2) glib actually has a function that takes a printf-style format and does a locale-inspecific decimal formatted string. I was curious how it worked: http://www.google.com/codesearch/p?hl=en#RGCD84x9Jg0/trunk/xbmc/lib/libmms/glib-2.20.4/glib/gstrfuncs.c&q=g_ascii_formatd&sa=N&cd=1&ct=rc&l=609 (If that link breaks, it's a code search for g_ascii_formatd.) It works by checking if your locale is one that doesn't use '.' for decimal points, and if so, does a search and replace on the string result of calling printf! This feels like a huge hack but actually doesn't seem so bad to me at this point.

Seemed like a little bug when I opened it. This is getting rather silly. So many brains and we can't come up with a solution. Okay, here is one: - Find the source code for atof.c and printf()-style formatting from a version of glibc which either doesn't support a locale or just omit the locale-specific part. - Cut'n'paste. Yeah, it's a pain but it's also only a couple of lines of code. So why bother. - Remove support for the old formatting code in String() to cause compile errors. - Check whether ACID passes. When ACID passes and some internal tests fail, then fix the internal tests. It's bad when some tests fail because items move by a pixel but then, an important feature of the browser doesn't work at all. On top of that, the fixed code should produce the same results as the old one except for the locale specific decimal point. So tests *should* not fail. If they do, then probably something else broke. But the focus should be on fixing this bug. If it breaks old tests, that's unfortunate and probably worth an investigation because it shouldn't (= there is no reason why it should except for rounding errors which probably means that the new code has a bug). But it shouldn't stop the fix.

*** Bug 36485 has been marked as a duplicate of this bug. ***

*** Bug 41541 has been marked as a duplicate of this bug. ***

*** Bug 41658 has been marked as a duplicate of this bug. ***

See also: bug 42755.

Gavin is working on some num -> string code that might help here.

Created attachment 70394 [details] Work in progress Attaching a first work in progress patch, and my plan how I want to fix this problem (and related SVG problems): List of the problems: - String::format() may be localy dependant. - String::number(double) uses String::format("%.6lg") and thus is affected as well Related problems, that affect SVG, is rounding. The String::format() usage truncated, instead of rounding properly to say N significant figures. - TextStream::operator<<(float/double) is affected. - SVGPahtStringBuilder (most prominent area of problems) - SVGPointList/SVGTransformList/SVGAnimatedPropertyTraits (all using String::format) The way I plan to fix this is outlined in the attached "Work in progress" patch. Step #1) Patch String::number(double) to utilize wtf/DecimalNumber (which uses dtoa) to convert the double into a string. Apply some extra operations to make it mimic the String::format("%.6lg") output (cut off trailing zeros 1.5000 -> 1.5, leave integers unchanged 12.000 -> 12) Step #2) Convert all users of String::format() to use String::number() + StringBuilder/Vector<Uchar> based string creation. Step #3) Kill String::format(), and it's platform specific implementations (like Qt did). What do you think?

When removing String::format(), following code has to change. String::format() users that don't dump floating-point numbers, and can be converted to a String::number() + StringBuilder/Vector<UChar> based solution right now: bindings/js/JSDOMWindowBase.cpp: return String::format("Unsafe JavaScript attempt to access frame with URL %s from frame with URL %s. Domains, protocols and ports must match.\n", bindings/v8/V8Proxy.cpp: String str = String::format("Unsafe JavaScript attempt to access frame " css/CSSOMUtils.cpp: append(appendTo, String::format("\\%x ", c)); dom/Document.cpp: return String::format("%02d/%02d/%04d %02d:%02d:%02d", date.month() + 1, date.monthDay(), date.fullYear(), date.hour(), date.minute(), date.second()); dom/ExceptionBase.cpp: m_message = String::format("%s: %s Exception %d", description.name, description.typeName, description.code); dom/ExceptionBase.cpp: m_message = String::format("%s Exception %d", description.typeName, description.code); dom/StyledElement.cpp: color = String::format("#%02x%02x%02x", colors[0], colors[1], colors[2]); dom/XMLDocumentParser.cpp: m_errorMessages += String::format("warning on line %d at column %d: %s", lineNumber, columnNumber, m); dom/XMLDocumentParser.cpp: m_errorMessages += String::format("error on line %d at column %d: %s", lineNumber, columnNumber, m); html/DateComponents.cpp: return String::format("%02d:%02d", m_hour, m_minute); html/DateComponents.cpp: return String::format("%02d:%02d:%02d", m_hour, m_minute, m_second); html/DateComponents.cpp: return String::format("%02d:%02d:%02d.%03d", m_hour, m_minute, m_second, m_millisecond); html/DateComponents.cpp: return String::format("%04d-%02d-%02d", m_year, m_month + 1, m_monthDay); html/DateComponents.cpp: return String::format("%04d-%02d-%02dT", m_year, m_month + 1, m_monthDay) html/DateComponents.cpp: return String::format("%04d-%02d-%02dT", m_year, m_month + 1, m_monthDay) html/DateComponents.cpp: return String::format("%04d-%02d", m_year, m_month + 1); html/DateComponents.cpp: return String::format("%04d-W%02d", m_year, m_week); html/FTPDirectoryDocument.cpp: timeOfDay = String::format(", %i:%02i AM", hour, fileTime.tm_min); html/FTPDirectoryDocument.cpp: timeOfDay = String::format(", %i:%02i PM", hour, fileTime.tm_min); html/FTPDirectoryDocument.cpp: dateString = String::format("%s %i, %i", months[month], fileTime.tm_mday, fileTime.tm_year); html/FTPDirectoryDocument.cpp: dateString = String::format("%s %i, %i", months[month], fileTime.tm_mday, now.tm_year); inspector/CodeGeneratorInspector.pm: push(@function, " protocolErrors->pushString(String::format(\"Protocol Error: %s handler is not available.\", \"$domain\"));"); inspector/CodeGeneratorInspector.pm: push(@function, " protocolErrors->pushString(String::format(\"Protocol Error: 'arguments' property with type 'object' was not found.\"));"); inspector/CodeGeneratorInspector.pm: push(@function, " protocolErrors->pushString(String::format(\"Protocol Error: Argument '%s' with type '%s' was not found.\", \"$name\", \"$JSONType\"));"); inspector/CodeGeneratorInspector.pm: push(@function, " protocolErrors->pushString(String::format(\"Protocol Error: Argument '%s' has wrong type. It should be '%s'.\", \"$name\", \"$JSONType\"));"); inspector/CodeGeneratorInspector.pm: reportProtocolError(callId, String::format("Protocol Error: Invalid command was received. '%s' wasn't found.", command.utf8().data())); inspector/InspectorController.cpp: String message = String::format("Failed to load resource: the server responded with a status of %u (", response.httpStatusCode()) + response.httpStatusText() + ")"; inspector/InspectorController.cpp: String identifier = title + String::format("@%s:%d", sourceID.utf8().data(), lineNumber); inspector/InspectorController.cpp: String message = String::format("%s: %d", title.utf8().data(), count); inspector/InspectorDebuggerAgent.cpp: return String::format("%s:%d", sourceID.utf8().data(), lineNumber); inspector/InspectorProfilerAgent.cpp: String message = String::format("Profile \"webkit-profile://%s/%s#%d\" finished.", CPUProfileType, encodeWithURLEscapeSequences(title).utf8().data(), profile->uid()); inspector/InspectorProfilerAgent.cpp: String message = String::format("Profile \"webkit-profile://%s/%s#0\" started.", CPUProfileType, encodeWithURLEscapeSequences(title).utf8().data()); inspector/InspectorProfilerAgent.cpp: return String::format("%s.%d", UserInitiatedProfileName, m_currentUserInitiatedProfileNumber); inspector/InspectorProfilerAgent.cpp: String title = String::format("%s.%d", UserInitiatedProfileName, m_nextUserInitiatedHeapSnapshotNumber++); inspector/InspectorValues.cpp: String symbolCode = String::format("\\u%04X", symbol); loader/archive/cf/LegacyWebArchive.cpp: String iframeMarkup = String::format("<iframe frameborder=\"no\" marginwidth=\"0\" marginheight=\"0\" width=\"98%%\" height=\"98%%\" src=\"%s\"></iframe>", loader/CachedResourceLoader.cpp: String::format("Unsafe attempt to load URL %s.", loader/CachedResourceLoader.cpp: String::format("Unsafe attempt to load URL %s from frame with URL %s. " loader/FrameLoader.cpp: String message = String::format("The page at %s displayed insecure content from %s.\n", loader/FrameLoader.cpp: String message = String::format("The page at %s ran insecure content from %s.\n", loader/FrameLoader.cpp: String message = String::format("Unsafe JavaScript attempt to initiate a navigation change for frame with URL %s from frame with URL %s.\n", page/DOMWindow.cpp: String message = String::format("Unable to post message to %s. Recipient has origin %s.\n", page/PrintContext.cpp: return String::format("%d", style->marginLeft().rawValue()); page/PrintContext.cpp: return String::format("%d", style->lineHeight().rawValue()); page/PrintContext.cpp: return String::format("%d", style->fontDescription().computedPixelSize()); page/PrintContext.cpp: return String::format("%s", style->fontDescription().family().family().string().utf8().data()); page/PrintContext.cpp: return String::format("%d %d", style->pageSize().width().rawValue(), style->pageSize().height().rawValue()); page/PrintContext.cpp: return String::format("pageProperty() unimplemented for: %s", propertyName); page/PrintContext.cpp: return String::format("(%d, %d) %d %d %d %d", pageSize.width(), pageSize.height(), marginTop, marginRight, marginBottom, marginLeft); page/XSSAuditor.cpp: String consoleMessage = String::format("Refused to load an object. URL found within request: \"%s\".\n", url.utf8().data()); platform/audio/HRTFElevation.cpp: String resourceName = String::format("IRC_%s_C_R0195_T%03d_P%03d", subjectName.utf8().data(), azimuth, positiveElevation); platform/brew/FileSystemBrew.cpp: return String::format("fs:/~0X%08X/", webViewClassId); platform/brew/PlatformKeyboardEventBrew.cpp: return String::format("U+%04X", toASCIIUpper(keyCode)); platform/cocoa/KeyEventCocoa.mm: return String::format("U+%04X", toASCIIUpper(charCode)); platform/efl/PlatformKeyboardEventEfl.cpp: String key = String::format("F%d", i); platform/efl/PlatformKeyboardEventEfl.cpp: String key = String::format("%c", alphabet[i]); platform/efl/PlatformKeyboardEventEfl.cpp: String key = String::format("%d", i); platform/efl/PlatformKeyboardEventEfl.cpp: String key = String::format("F%d", i); platform/graphics/brew/ImageBrew.cpp: String resourcePath = homeDirectoryPath() + String::format("res/%s.png", name); platform/graphics/cg/ImageBufferCG.cpp: return String::format("data:%s;base64,%s", mimeType.utf8().data(), out.data()); platform/graphics/chromium/FontUtilsChromiumWin.cpp: String fontKey = String::format("%1d:%d:%ls", style, logfont->lfHeight, family); platform/graphics/chromium/GraphicsLayerChromium.cpp: String name = String::format("GraphicsLayerChromium(%p) GraphicsLayer(%p) ", m_layer.get(), this) + inName; platform/graphics/Color.cpp: return String::format("#%02x%02x%02x", red(), green(), blue()); platform/graphics/Color.cpp: return String::format("rgba(%u, %u, %u, 0.0)", red(), green(), blue()); platform/graphics/Color.cpp: return String::format("#%02X%02X%02X%02X", red(), green(), blue(), alpha()); platform/graphics/Color.cpp: return String::format("#%02X%02X%02X", red(), green(), blue()); platform/graphics/efl/ImageEfl.cpp: RefPtr<SharedBuffer> buffer = SharedBuffer::createWithContentsOfFile(String::format(DATA_DIR "/webkit-1.0/images/%s.png", name)); platform/graphics/GraphicsLayer.cpp: return String::format("-|transition%c-", property); platform/graphics/gtk/ImageBufferGtk.cpp: return String::format("data:%s;base64,%s", mimeType.utf8().data(), out.data()); platform/graphics/haiku/ImageBufferHaiku.cpp: return String::format("data:%s;base64,%s", mimeType.utf8().data(), platform/graphics/mac/GraphicsLayerCA.mm: return animationName + String::format("_%d_%d", property, index); platform/graphics/mac/GraphicsLayerCA.mm: String longName = String::format("CALayer(%p) GraphicsLayer(%p) ", m_layer.get(), this) + name; platform/graphics/mac/GraphicsLayerCA.mm: String name = String::format("CALayer(%p) GraphicsLayer(%p) ", m_layer.get(), this) + m_name; platform/graphics/qt/ImageBufferQt.cpp: return String::format("data:%s;base64,%s", mimeType.utf8().data(), data.toBase64().data()); platform/graphics/skia/ImageBufferSkia.cpp: return String::format("data:image/png;base64,%s", base64EncodedData.data()); platform/graphics/win/GraphicsLayerCACF.cpp: String longName = String::format("CALayer(%p) GraphicsLayer(%p) ", m_layer.get(), this) + name; platform/graphics/win/GraphicsLayerCACF.cpp: String name = String::format("CALayer(%p) GraphicsLayer(%p) %s", m_layer.get(), this, m_usingTiledLayer ? "[Tiled Layer] " : "") + m_name; platform/graphics/win/WebLayer.cpp: String text = String::format("%d", m_owner->incrementRepaintCount());; platform/gtk/KeyEventGtk.cpp: return String::format("U+%04X", gdk_keyval_to_unicode(gdk_keyval_to_upper(keyCode))); platform/haiku/PlatformKeyboardEventHaiku.cpp: return String::format("U+%04X", toASCIIUpper(singleByte)); platform/network/CredentialStorage.cpp: return url.protocol() + "://" + url.host() + String::format(":%i/", url.port()); platform/qt/PlatformKeyboardEventQt.cpp: return String::format("U+%04X", toupper(keyCode)); platform/sql/SQLiteDatabase.cpp: executeCommand(String::format("PRAGMA synchronous = %i", sync)); platform/sql/SQLiteFileSystem.cpp: fileName = pathByAppendingComponent(dbDir, String::format("%016"PRIx64".db", seq)); platform/sql/SQLiteFileSystem.cpp: return String::format("%016"PRIx64".db", seq); platform/text/wince/TextCodecWinCE.cpp: String cpName = String::format("cp%d", cpInfo.uiCodePage); platform/win/ClipboardUtilitiesWin.cpp: append(result, String::format(header, startHTMLOffset, endHTMLOffset, startFragmentOffset, endFragmentOffset).utf8()); platform/win/GDIObjectCounter.cpp: init(String::format("%s (%p)", className.latin1().data(), instance)); platform/win/KeyEventWin.cpp: return String::format("U+%04X", toASCIIUpper(keyCode)); platform/win/Language.cpp: computedDefaultLanguage = String::format("%s-%s", languageName.latin1().data(), countryName.latin1().data()); platform/wx/KeyboardEventWx.cpp: return String::format("U+%04X", toupper(keyCode)); plugins/PluginStream.cpp: String statusLine = String::format("HTTP %d OK\n", m_resourceResponse.httpStatusCode()); rendering/RenderTheme.cpp: return String::format("%s%02d:%02d:%02d", (time < 0 ? "-" : ""), hours, minutes, seconds); rendering/RenderTheme.cpp: return String::format("%s%01d:%02d:%02d", (time < 0 ? "-" : ""), hours, minutes, seconds); rendering/RenderTheme.cpp: return String::format("%s%02d:%02d", (time < 0 ? "-" : ""), minutes, seconds); rendering/RenderTreeAsText.cpp: String hex = String::format("\\x{%X}", u); svg/SVGUseElement.cpp: text += String::format("SVGElementInstance this=%p, (parentNode=%s (%p), firstChild=%s (%p), correspondingElement=%s (%p), shadowTreeElement=%s (%p), id=%s)\n", svg/SVGUseElement.cpp: text += String::format("Corresponding element is associated with %i instance(s):\n", elementInstances.size()); svg/SVGUseElement.cpp: text += String::format(" -> SVGElementInstance this=%p, (refCount: %i, shadowTreeElement in document? %i)\n", websockets/WebSocket.cpp: builder.append(String::format("\\u%04X", protocol[i])); websockets/WebSocket.cpp: scriptExecutionContext()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, String::format("WebSocket port %d blocked", url.port()), 0, scriptExecutionContext()->securityOrigin()->toString()); websockets/WebSocketChannel.cpp: m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, String::format("WebSocket frame (at %lu bytes) is too long.", static_cast<unsigned long>(newBufferSize)), 0, m_handshake.clientOrigin()); websockets/WebSocketHandshake.cpp: m_context->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, String::format("Unexpected response code: %d", statusCode), 0, clientOrigin()); String::format() users that dump floating-point numbers, and would need my patch for String::number() before they could be converted: css/CSSParser.cpp: String str = String::format("%06d", (int)(value->fValue+.5)); css/CSSPrimitiveValue.cpp: text = String::format("%.6lg%%", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgem", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgex", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgrem", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgpx", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgcm", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgmm", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgin", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgpt", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgpc", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgdeg", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgrad", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lggrad", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgms", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgs", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lghz", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgkhz", m_value.num); css/CSSPrimitiveValue.cpp: text = String::format("%.6lgturn", m_value.num); css/WebKitCSSMatrix.cpp: return String::format("matrix(%f, %f, %f, %f, %f, %f)", css/WebKitCSSMatrix.cpp: return String::format("matrix3d(%f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f, %f)", html/FTPDirectoryDocument.cpp: return String::format("%.2f KB", static_cast<float>(bytes)/1000); html/FTPDirectoryDocument.cpp: return String::format("%.2f MB", static_cast<float>(bytes)/1000000); html/FTPDirectoryDocument.cpp: return String::format("%.2f GB", static_cast<float>(bytes)/1000000000); inspector/InspectorValues.cpp: String value = String::format("%f", m_doubleValue); page/Console.cpp: String message = title + String::format(": %.0fms", elapsed); platform/graphics/Color.cpp: return String::format("rgba(%u, %u, %u, %.5f)", red(), green(), blue(), alpha() / 255.0f); platform/graphics/gtk/ImageBufferGtk.cpp: String qualityString = String::format("%f", *quality); platform/graphics/mac/MediaPlayerPrivateQTKit.mm: String text = String::format("%1.2f", frameRate); svg/SVGAnimatedPropertyTraits.h: static String toString(PassType type) { return String::format("%f %f %f %f", type.x(), type.y(), type.width(), type.height()); } svg/SVGTransformList.cpp: return String::format("matrix(%f %f %f %f %f %f)", matrix.a(), matrix.b(), matrix.c(), matrix.d(), matrix.e(), matrix.f()); If my patch is considered good, I'd change the usages of String::format("%f") to the new String::number() implementation combined StringBuilder/Vector<UChar> to replace the String::format() calls. That would be a good first step, fixing this bug, and then later on we can kill String::format(). I've just realized that not all code wants to round to 6 significant figures, some explicitely demand 2 decimals, some code just uses %f. That means my patch has to be adapted to that. The code in FTPDirectoryDocument for example doesn't want to print exponential numbers, never, that is also not handled yet, when using String::number(double). So maybe we want another enum, that precisely describes the conversion mode. enum FloatingPointConversion { RoundToSignificantFigures, RoundToDecimalPlaces, AvoidExponentials }; String String::number(double number, unsigned = RoundToSignificantFigures, unsigned significantFiguresOrDecimalPlaces = 6). The second argument could be used to combine eg. RoundToSiginificantFigures | AvoidExponentials. Enough for today, what do others think? :-)

I think it's a good general direction. This bug is related to bug 20674.

Hi Nikolas, I've been looking a bit at strings & number/string conversions lately, and had a couple of comments on this: > Step #3) Kill String::format(), and it's platform specific implementations (like Qt did). Yes! We we absolutely should get rid of this. > Step #2) Convert all users of String::format() to use String::number() + StringBuilder/Vector<Uchar> based string creation. I think there is something slightly better we could do here. StringBuilder is great if you're appending an arbitrary number of strings together, but being able to handle arbitrary amounts of data adds overhead (currently in the form of building the Vector, converting all C-strings to Strings. In the near future we're likely to move from using a rope to an overcapacity based approach, which will remove these cost but possibly add some copying cost instead). So one thing I've been meaning to do for a while is to add a String::cat() method to WTF::String. Logically this would be a varargs function, but in practice since varargs provides no type information & only handles POD data, there would actually be multiple implementations, with methods handling something like 2-to-8 arguments. In a trivial implementation all arguments would be 'const String&'s, and String::cat() could just wrap StringBuilder (rather than using StringBuilder directly) such that the internal implementation can be improved later. A better implementation we could make the arguments to cat be templated such that data could be added to the string without converting all arguments to String format first. We already have this implemented in JavaScriptCore (and since we're close to unifying the string implementations we could ultimately fully share this code), in a set of methods called makeString. These allow an arbitrary mix of UStrings and plain C-strings to be concatenated together, without first converting the C-strings to Strings. > Step #1) Patch String::number(double) to utilize wtf/DecimalNumber (which uses dtoa) to convert the double into a string. Apply some extra operations to make it mimic the String::format("%.6lg") output (cut off trailing zeros 1.5000 -> 1.5, leave integers unchanged 12.000 -> 12) So, the question here is, what do we want String::number() to do? * The rules for number to String conversion are different in different web technology specifications. * SVG has an uncommon requirement, in that it never overflows to an exponential representation. * HTML5 and ECMAScript agree on number-to-string conversion (in that the HTML5 spec requires ECMAScript conversion). My thought here is that we want String::number() to match HTML5/ECMAScript number-to-string conversion. That is to say, like UString::number it should call WTF::numberToString in dtoa.h, which wraps DecimalNumber with appropriate bounds (<10^-6 & >=10^21) spilling to exponential representation. We should probably add a different method specifically for SVG's number-to-string behaviour. This could be a free function within the svg code, taking a double and returning an String. This method should also be able to use DecimalNumber in its implementation, but would likely chose to always call toStringDecimal, rather than toStringExponential. If elsewhere in the code we do require numbers formatted in a specific fashion (with a fixed number of digits, or significant figures) we should add methods to convert with appropriate constraints as necessary. Hopefully we shouldn't need to mimic "%.6lg" behaviour, since this is not spec defined, probably doesn't match other browsers, and as such is hopefully not relied on anywhere. Changing this should probably only fix spec compliance. How does this sound? cheers, G.

(In reply to comment #116) > Hi Nikolas, > > I've been looking a bit at strings & number/string conversions lately, and had a couple of comments on this: Thanks! > > > Step #3) Kill String::format(), and it's platform specific implementations (like Qt did). > > Yes! We we absolutely should get rid of this. Great. > > > Step #2) Convert all users of String::format() to use String::number() + StringBuilder/Vector<Uchar> based string creation. > > I think there is something slightly better we could do here. StringBuilder is great if you're appending an arbitrary number of strings together, but being able to handle arbitrary amounts of data adds overhead (currently in the form of building the Vector, converting all C-strings to Strings. In the near future we're likely to move from using a rope to an overcapacity based approach, which will remove these cost but possibly add some copying cost instead). > > So one thing I've been meaning to do for a while is to add a String::cat() method to WTF::String. Logically this would be a varargs function, but in practice since varargs provides no type information & only handles POD data, there would actually be multiple implementations, with methods handling something like 2-to-8 arguments. > > In a trivial implementation all arguments would be 'const String&'s, and String::cat() could just wrap StringBuilder (rather than using StringBuilder directly) such that the internal implementation can be improved later. Excellent, that would be really handy. Do you have already started coding this? > > A better implementation we could make the arguments to cat be templated such that data could be added to the string without converting all arguments to String format first. We already have this implemented in JavaScriptCore (and since we're close to unifying the string implementations we could ultimately fully share this code), in a set of methods called makeString. These allow an arbitrary mix of UStrings and plain C-strings to be concatenated together, without first converting the C-strings to Strings. I see, seems like it would be very wise to share the code in wtf/. > > > Step #1) Patch String::number(double) to utilize wtf/DecimalNumber (which uses dtoa) to convert the double into a string. Apply some extra operations to make it mimic the String::format("%.6lg") output (cut off trailing zeros 1.5000 -> 1.5, leave integers unchanged 12.000 -> 12) > > So, the question here is, what do we want String::number() to do? Right. > > * The rules for number to String conversion are different in different web technology specifications. > * SVG has an uncommon requirement, in that it never overflows to an exponential representation. > * HTML5 and ECMAScript agree on number-to-string conversion (in that the HTML5 spec requires ECMAScript conversion). SVGs requirement is uncommon right, but we don't want this for the render path dumps, we definately want exponential numbers in the SVG DRT dumps. > > My thought here is that we want String::number() to match HTML5/ECMAScript number-to-string conversion. That is to say, like UString::number it should call WTF::numberToString in dtoa.h, which wraps DecimalNumber with appropriate bounds (<10^-6 & >=10^21) spilling to exponential representation. Correct. > > We should probably add a different method specifically for SVG's number-to-string behaviour. This could be a free function within the svg code, taking a double and returning an String. This method should also be able to use DecimalNumber in its implementation, but would likely chose to always call toStringDecimal, rather than toStringExponential. I'm not sure if we need a special SVG method, I'd rather prefer an enum that describes how the number conversion should happen. The default should match HTML5/ECMAScript. > > If elsewhere in the code we do require numbers formatted in a specific fashion (with a fixed number of digits, or significant figures) we should add methods to convert with appropriate constraints as necessary. Hopefully we shouldn't need to mimic "%.6lg" behaviour, since this is not spec defined, probably doesn't match other browsers, and as such is hopefully not relied on anywhere. Changing this should probably only fix spec compliance. I tried to mimic the output initially to reduce the DRT changes for SVG, to see wheter the patch would fix the rounding issues between gtk 32bit and 64bit. It's not a requirment to mimic the output of String::format, agreed. > > How does this sound? Excellent! I'm happy to work on this, I hope you already wrote the String::cat stuff? If yes, I'd work on a String::number() implementation (like the one in my attached patch) which gives us more flexibility (add an FloatingPointConversionMode enum, with AvoidExponentials, RoundSiginifcantFigures, RoundDecimalPlaces modes, that can be OR'ed together, etc..) How does this sound? :-)

> > In a trivial implementation all arguments would be 'const String&'s, and String::cat() could just wrap StringBuilder (rather than using StringBuilder directly) such that the internal implementation can be improved later. > Excellent, that would be really handy. Do you have already started coding this? I haven't, but this should be pretty trivial: String String::cat(const String& s1, const String& s2) { StringBuilder builder; builder.append(s1); builder.append(s2); return builder.toSting(); } String String::cat(const String& s1, const String& s2, const String& s3) { StringBuilder builder; builder.append(s1); builder.append(s2); builder.append(s3); return builder.toSting(); } And so on for more more arguments. One problem, I think StringBuilder is currently in WebCore, not WTF, if should move to wtf/text with the rest of the string code. (I have a patch I'm working on to rewrite StringBuilder, so we may conflict a little there, but if you're just moving the code should be easy to resolve for whoever lands second!) > > A better implementation we could make the arguments to cat be templated such that data could be added to the string without converting all arguments to String format first. We already have this implemented in JavaScriptCore (and since we're close to unifying the string implementations we could ultimately fully share this code), in a set of methods called makeString. These allow an arbitrary mix of UStrings and plain C-strings to be concatenated together, without first converting the C-strings to Strings. > I see, seems like it would be very wise to share the code in wtf/. *nod. The only reason this code is currently JSC specific is because it works on UStrings not Strings, and the difference in String::number & UString::number is one of the few remaining issues to resolve before we can remove UString, so this change should get us closer. > > * The rules for number to String conversion are different in different web technology specifications. > > * SVG has an uncommon requirement, in that it never overflows to an exponential representation. > > * HTML5 and ECMAScript agree on number-to-string conversion (in that the HTML5 spec requires ECMAScript conversion). > SVGs requirement is uncommon right, but we don't want this for the render path dumps, we definately want exponential numbers in the SVG DRT dumps. Ah I see, yes okay. so for these cases the HTML5/ECMAScript behaviour should probably be okay? > > We should probably add a different method specifically for SVG's number-to-string behaviour. This could be a free function within the svg code, taking a double and returning an String. This method should also be able to use DecimalNumber in its implementation, but would likely chose to always call toStringDecimal, rather than toStringExponential. > I'm not sure if we need a special SVG method, I'd rather prefer an enum that describes how the number conversion should happen. The default should match HTML5/ECMAScript. > > Excellent! I'm happy to work on this, I hope you already wrote the String::cat stuff? If yes, I'd work on a String::number() implementation (like the one in my attached patch) which gives us more flexibility (add an FloatingPointConversionMode enum, with AvoidExponentials, RoundSiginifcantFigures, RoundDecimalPlaces modes, that can be OR'ed together, etc..) My main concern here would be that UString::number can be a hot function, so it would probably be undesirable to add unnecessary overhead to switch on a parameter to check the conversion type. Also, there may be some subtleties to the requirements of any other conversion functions, so we may be better adding them as we need them. For example, the decimal number conversion uses a fixed size buffer on the stack, which would not be safe to convert large double values to decimal format. To safely use this conversion the simple solution would probably be to truncate values into a range (which I believe is explicitly permitted in the SVG spec, Simon?) – however String::number having this truncation built in would be a little weird and arbirtary. Also, to-fixed methods may require a precision argument. So perhaps it would just be best to switch String::number to have the HTML5/ECMAScript behaviour (matching UString::number) for all uses, for now?

Linking some bug reports, that I've worked on in order to prepare fixing this bug.

(In reply to comment #118) Here are some news, for those that don't follow all webkit commits: > > Excellent, that would be really handy. Do you have already started coding this? > > I haven't, but this should be pretty trivial: > > String String::cat(const String& s1, const String& s2) .. This is obsolete now: we have wtf/text/StringConcatenate, solving this problem in an elegant and performant way. > And so on for more more arguments. One problem, I think StringBuilder is currently in WebCore, not WTF, if should move to wtf/text with the rest of the string code. (I have a patch I'm working on to rewrite StringBuilder, so we may conflict a little there, but if you're just moving the code should be easy to resolve for whoever lands second!) StringBuilder lives in wtf/text now, but using the more performant implementation based on runtime/StringBuilder. > > > SVGs requirement is uncommon right, but we don't want this for the render path dumps, we definately want exponential numbers in the SVG DRT dumps. > > Ah I see, yes okay. so for these cases the HTML5/ECMAScript behaviour should probably be okay? Exactly, the only drawback is that each expected.txt file is affected, but I think we can live with that, instead of inventing extra hacks (remove trailing zeros etc..) just for the sake of maintaining compatibility between the new & current DRT dump style. > > Excellent! I'm happy to work on this, I hope you already wrote the String::cat stuff? If yes, I'd work on a String::number() implementation (like the one in my attached patch) which gives us more flexibility (add an FloatingPointConversionMode enum, with AvoidExponentials, RoundSiginifcantFigures, RoundDecimalPlaces modes, that can be OR'ed together, etc..) > > My main concern here would be that UString::number can be a hot function, so it would probably be undesirable to add unnecessary overhead to switch on a parameter to check the conversion type. Also, there may be some subtleties to the requirements of any other conversion functions, so we may be better adding them as we need them. For example, the decimal number conversion uses a fixed size buffer on the stack, which would not be safe to convert large double values to decimal format. To safely use this conversion the simple solution would probably be to truncate values into a range (which I believe is explicitly permitted in the SVG spec, Simon?) – however String::number having this truncation built in would be a little weird and arbirtary. Also, to-fixed methods may require a precision argument. I made up my mind, and think we should have a simple String::number(double) method which serializes the double using the numberToString() method (which uses DecimalNumber) to match the HTML5/ECMAScript default way of doing this. I'm concerned as well to add any extra logic, given the fact we ideally want to switch from UString to WTFString soon. We can add another method, which takes a precision argument, with explicit naming, whenever we need it (String::numberRoundingSignificantFigures(number, 6)) etc.. > > So perhaps it would just be best to switch String::number to have the HTML5/ECMAScript behaviour (matching UString::number) for all uses, for now? Definately. I'm still preparing patches, before I can actually work on fixing String::number. My current plan is: 1) Remove all "simple" String::format usage (no floating-point formatting involved, no pointer dumps) from all of WebCore/WebKit - replace by StringConcatenate (see bug 47664) 2) Identify a common set of missing methods that would be needed to replace the rest of the String::format usages (String::numberAsHex, etc.) 3) Implement them, kill String::format completly. 4) Be happy! :-)

Attachment 70394 [details] did not build on win: Build output: http://queues.webkit.org/results/4436061

Comment on attachment 70394 [details] Work in progress DecimalNumber is the correct approach. See bug 20674.

(In reply to comment #122) > (From update of attachment 70394 [details]) > DecimalNumber is the correct approach. See bug 20674. Yeah, just as we discussed above. I forgot to clear the r?.

Fixed in http://trac.webkit.org/changeset/69928