Bug 96344 – Deploy StringBuilder::appendNumber() and StringBuilder::appendLiteral() in more places

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jinwoo Song]

Use StringBuilder::appendNumber() instead of String::number(). Patch is coming.

[Jinwoo Song]

Created attachment 163269 [details]
patch

[WebKit Review Bot]

Comment on attachment 163269 [details]
patch

Attachment 163269 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13811395

[Peter Beverloo (cr-android ews)]

Comment on attachment 163269 [details]
patch

Attachment 163269 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13824080

[Build Bot]

Comment on attachment 163269 [details]
patch

Attachment 163269 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13804808

[Jinwoo Song]

Created attachment 163271 [details]
patch. fix build error.

[Build Bot]

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

[WebKit Review Bot]

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

[Early Warning System Bot]

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

[Early Warning System Bot]

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

[Peter Beverloo (cr-android ews)]

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

[Jinwoo Song]

Created attachment 163275 [details]
patch. fix build error.

[Benjamin Poulain]

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.

[Jinwoo Song]

Created attachment 163280 [details]
patch

[Jinwoo Song]

(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.

[Benjamin Poulain]

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.

[Jinwoo Song]

Created attachment 163293 [details]
patch.

Applied Benjamin's comments. Thanks a lot. :)

[Jinwoo Song]

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.

[Benjamin Poulain]

Comment on attachment 163293 [details]
patch.

Looks good.

[Darin Adler]

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.

[WebKit Review Bot]

Comment on attachment 163293 [details]
patch.

Clearing flags on attachment: 163293

Committed r128204: <http://trac.webkit.org/changeset/128204>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

> > 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.

[Patrick R. Gansterer]

(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.