[webkit-reviews] review denied: [Bug 18994] LANG/LC_ALL influences the result of element.style.opacity : [Attachment 23471] Deprecate String::format()

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alp Toker <alp at nuanti.com>'s request
for review:
Bug 18994: LANG/LC_ALL influences the result of element.style.opacity
https://bugs.webkit.org/show_bug.cgi?id=18994

Attachment 23471: Deprecate String::format()
https://bugs.webkit.org/attachment.cgi?id=23471&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
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.