Use StringBuilder::appendNumber() instead of String::number(). Patch is coming.
Created attachment 163269 [details] patch
Comment on attachment 163269 [details] patch Attachment 163269 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13811395
Comment on attachment 163269 [details] patch Attachment 163269 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13824080
Comment on attachment 163269 [details] patch Attachment 163269 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13804808
Created attachment 163271 [details] patch. fix build error.
Comment on attachment 163271 [details] patch. fix build error. Attachment 163271 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13827037
Comment on attachment 163271 [details] patch. fix build error. Attachment 163271 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13824084
Comment on attachment 163271 [details] patch. fix build error. Attachment 163271 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13824087
Comment on attachment 163271 [details] patch. fix build error. Attachment 163271 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13805758
Comment on attachment 163271 [details] patch. fix build error. Attachment 163271 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13811401
Created attachment 163275 [details] patch. fix build error.
Comment on attachment 163275 [details] patch. fix build error. View in context: https://bugs.webkit.org/attachment.cgi?id=163275&action=review > Source/WebCore/ChangeLog:3 > + Deploy StringBuilder::appendNumber() in more place Update the title to Deploy StringBuilder::appendNumber() and StringBuilder::appendLiteral() in more place > Source/WebCore/ChangeLog:9 > + StringBuilder::appendLiteral() instead of String::appned() in the surrounding code. typo: String::appned() > Source/WebCore/css/CSSAspectRatioValue.cpp:43 > StringBuilder result; > result.append(String::number(m_numeratorValue)); > - result.append("/"); > + result.append('/'); > result.append(String::number(m_denominatorValue)); > return result.toString(); This whole function should use String Operators. > Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:95 > - source.append(enableds[i]? "true" : "false"); > + source.appendLiteral(enableds[i]? "true" : "false"); I doubt this compile, StringBuilder::appendLiteral() is resolved at compile time. It should be: if (enableds[i]) source.appendLiteral("true"); else source.appendLiteral("false"); > Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:113 > - source.append(selecteds[i]? "true" : "false"); > + source.appendLiteral(selecteds[i]? "true" : "false"); Ditto. > Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:-111 > - builder.append(it->second + "\r\n"); Gosh, that's sad.
Created attachment 163280 [details] patch
(In reply to comment #12) > (From update of attachment 163275 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163275&action=review > > > Source/WebCore/ChangeLog:3 > > + Deploy StringBuilder::appendNumber() in more place > > Update the title to > Deploy StringBuilder::appendNumber() and StringBuilder::appendLiteral() in more place Done! > > > Source/WebCore/ChangeLog:9 > > + StringBuilder::appendLiteral() instead of String::appned() in the surrounding code. > > typo: String::appned() Fixed! > > > Source/WebCore/css/CSSAspectRatioValue.cpp:43 > > StringBuilder result; > > result.append(String::number(m_numeratorValue)); > > - result.append("/"); > > + result.append('/'); > > result.append(String::number(m_denominatorValue)); > > return result.toString(); > > This whole function should use String Operators. Done! > > > Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:95 > > - source.append(enableds[i]? "true" : "false"); > > + source.appendLiteral(enableds[i]? "true" : "false"); > > I doubt this compile, StringBuilder::appendLiteral() is resolved at compile time. > It should be: > if (enableds[i]) > source.appendLiteral("true"); > else > source.appendLiteral("false"); Done! > > > Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:113 > > - source.append(selecteds[i]? "true" : "false"); > > + source.appendLiteral(selecteds[i]? "true" : "false"); > > Ditto. Done! > > > Source/WebKit2/UIProcess/InspectorServer/WebSocketServerConnection.cpp:-111 > > - builder.append(it->second + "\r\n"); > > Gosh, that's sad. yes, indeed.
Comment on attachment 163280 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=163280&action=review > Source/WebCore/ChangeLog:3 > + Deploy StringBuilder::appendNumber() and StringBuilder::appendLiteral() in more place Typo: place->places > Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.cpp:62 > + source.appendLiteral("</style></head><body>\n"); > + source.appendLiteral("<script>\n"); > + source.appendLiteral("window.addEventListener('load', function () {"); Hum, this would actually be even better in one append(): source.appendLiteral("</style></head><body>\n" "<script>\n" "window.addEventListener('load', function () {"); > Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.cpp:98 > + source.appendLiteral("</script>\n"); > + source.appendLiteral("</body> </html>\n"); ditto, one literal would be better. > Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:78 > + source.appendLiteral("</style></head><body>\n"); > + source.appendLiteral("<script>\n"); > + source.appendLiteral("window.addEventListener('load', function () {"); Ditto. > Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:125 > + source.appendLiteral("] "); > + source.appendLiteral(", 'Cancel'"); Ditto. > Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:132 > + source.appendLiteral("</script>\n"); > + source.appendLiteral("</body> </html>\n"); Ditto.
Created attachment 163293 [details] patch. Applied Benjamin's comments. Thanks a lot. :)
Comment on attachment 163280 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=163280&action=review >> Source/WebCore/ChangeLog:3 >> + Deploy StringBuilder::appendNumber() and StringBuilder::appendLiteral() in more place > > Typo: place->places fixed. >> Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.cpp:62 >> + source.appendLiteral("window.addEventListener('load', function () {"); > > Hum, this would actually be even better in one append(): > source.appendLiteral("</style></head><body>\n" > "<script>\n" > "window.addEventListener('load', function () {"); Done. >> Source/WebKit/blackberry/WebCoreSupport/DatePickerClient.cpp:98 >> + source.appendLiteral("</body> </html>\n"); > > ditto, one literal would be better. Done. >> Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:78 >> + source.appendLiteral("window.addEventListener('load', function () {"); > > Ditto. Done. >> Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:125 >> + source.appendLiteral(", 'Cancel'"); > > Ditto. Done. >> Source/WebKit/blackberry/WebCoreSupport/SelectPopupClient.cpp:132 >> + source.appendLiteral("</body> </html>\n"); > > Ditto. Done.
Comment on attachment 163293 [details] patch. Looks good.
Comment on attachment 163293 [details] patch. View in context: https://bugs.webkit.org/attachment.cgi?id=163293&action=review > Source/WebCore/css/CSSAspectRatioValue.cpp:39 > - StringBuilder result; > - result.append(String::number(m_numeratorValue)); > - result.append("/"); > - result.append(String::number(m_denominatorValue)); > - return result.toString(); > + return String::number(m_numeratorValue) + '/' + String::number(m_denominatorValue); It isn’t so great that this creates three String objects and then destroys two. We should figure out how to make uses like this efficient. It might even have been better to have this continue to use StringBuilder.
Comment on attachment 163293 [details] patch. Clearing flags on attachment: 163293 Committed r128204: <http://trac.webkit.org/changeset/128204>
All reviewed patches have been landed. Closing bug.
> > Source/WebCore/css/CSSAspectRatioValue.cpp:39 > > - StringBuilder result; > > - result.append(String::number(m_numeratorValue)); > > - result.append("/"); > > - result.append(String::number(m_denominatorValue)); > > - return result.toString(); > > + return String::number(m_numeratorValue) + '/' + String::number(m_denominatorValue); > > It isn’t so great that this creates three String objects and then destroys two. We should figure out how to make uses like this efficient. It might even have been better to have this continue to use StringBuilder. There is no StringBuilder::number() for floats. At the moment, this is the best solution. We should have StringBuilder::number() for double and a StringOperator for numbers, but that is not there yet.
(In reply to comment #22) > > > Source/WebCore/css/CSSAspectRatioValue.cpp:39 > > > - StringBuilder result; > > > - result.append(String::number(m_numeratorValue)); > > > - result.append("/"); > > > - result.append(String::number(m_denominatorValue)); > > > - return result.toString(); > > > + return String::number(m_numeratorValue) + '/' + String::number(m_denominatorValue); > > > > It isn’t so great that this creates three String objects and then destroys two. We should figure out how to make uses like this efficient. It might even have been better to have this continue to use StringBuilder. > > There is no StringBuilder::number() for floats. At the moment, this is the best solution. I'm working on, but I want to cleanup some parts first. Otherwise I have to duplicate too much code. > We should have StringBuilder::number() for double and a StringOperator for numbers, but that is not there yet.