Bug 96344

Summary: Deploy StringBuilder::appendNumber() and StringBuilder::appendLiteral() in more places
Product: WebKit Reporter: Jinwoo Song <jinwoo7.song>
Component: New BugsAssignee: Jinwoo Song <jinwoo7.song>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cmarcelo, dglazkov, dino, gustavo, macpherson, menard, mifenton, paroga, peter+ews, philn, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
webkit.review.bot: commit-queue-
patch. fix build error.
buildbot: commit-queue-
patch. fix build error.
none
patch
benjamin: review+, benjamin: commit-queue-
patch. none

Description Jinwoo Song 2012-09-10 17:56:42 PDT 
Use StringBuilder::appendNumber() instead of String::number(). Patch is coming.
Comment 1 Jinwoo Song 2012-09-10 20:44:44 PDT 
Created attachment 163269 [details]
patch
Comment 2 WebKit Review Bot 2012-09-10 20:48:32 PDT 
Comment on attachment 163269 [details]
patch

Attachment 163269 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13811395
Comment 3 Peter Beverloo (cr-android ews) 2012-09-10 20:54:03 PDT 
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 4 Build Bot 2012-09-10 20:57:37 PDT 
Comment on attachment 163269 [details]
patch

Attachment 163269 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13804808
Comment 5 Jinwoo Song 2012-09-10 21:03:07 PDT 
Created attachment 163271 [details]
patch. fix build error.
Comment 6 Build Bot 2012-09-10 21:09:29 PDT 
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 7 WebKit Review Bot 2012-09-10 21:11:51 PDT 
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 8 Early Warning System Bot 2012-09-10 21:15:41 PDT 
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 9 Early Warning System Bot 2012-09-10 21:18:43 PDT 
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 10 Peter Beverloo (cr-android ews) 2012-09-10 21:32:54 PDT 
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
Comment 11 Jinwoo Song 2012-09-10 21:36:02 PDT 
Created attachment 163275 [details]
patch. fix build error.
Comment 12 Benjamin Poulain 2012-09-10 21:50:42 PDT 
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.
Comment 13 Jinwoo Song 2012-09-10 22:32:15 PDT 
Created attachment 163280 [details]
patch
Comment 14 Jinwoo Song 2012-09-10 22:34:12 PDT 
(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 15 Benjamin Poulain 2012-09-10 23:46:18 PDT 
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.
Comment 16 Jinwoo Song 2012-09-11 00:29:57 PDT 
Created attachment 163293 [details]
patch.

Applied Benjamin's comments. Thanks a lot. :)
Comment 17 Jinwoo Song 2012-09-11 00:35:59 PDT 
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 18 Benjamin Poulain 2012-09-11 10:29:09 PDT 
Comment on attachment 163293 [details]
patch.

Looks good.
Comment 19 Darin Adler 2012-09-11 10:34:58 PDT 
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 20 WebKit Review Bot 2012-09-11 10:40:14 PDT 
Comment on attachment 163293 [details]
patch.

Clearing flags on attachment: 163293

Committed r128204: <http://trac.webkit.org/changeset/128204>
Comment 21 WebKit Review Bot 2012-09-11 10:40:19 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 22 Benjamin Poulain 2012-09-11 11:08:46 PDT 
> > 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.
Comment 23 Patrick R. Gansterer 2012-09-11 17:07:59 PDT 
(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.