Bug 62527 – [EFL][WK2] Change string operation to makeString()

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKit2

[Eunmi Lee]

I've changed string operation codes to use makeString().

[Eunmi Lee]

Created attachment 96918 [details]
Patch

[Gyuyoung Kim]

LGTM.

[Raphael Kubo da Costa (:rakuco)]

Informal r+ from my side.

[Kenneth Rohde Christiansen]

Comment on attachment 96918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96918&action=review

> Source/WebKit2/ChangeLog:6
> +        [EFL][WK2] Change string operation to makeString()
> +        https://bugs.webkit.org/show_bug.cgi?id=62527

Please add one line explaining why, like why that makes sense, why that is better etc? 

like 'use makeStrings() instead of creating individual strings and concatenating them'

[Eunmi Lee]

Created attachment 97109 [details]
Patch

[Eunmi Lee]

(In reply to comment #4)
> (From update of attachment 96918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96918&action=review
> 
> > Source/WebKit2/ChangeLog:6
> > +        [EFL][WK2] Change string operation to makeString()
> > +        https://bugs.webkit.org/show_bug.cgi?id=62527
> 
> Please add one line explaining why, like why that makes sense, why that is better etc? 
> 
> like 'use makeStrings() instead of creating individual strings and concatenating them'

Done. 
I've added one line for explaining.

[Leandro Pereira]

Comment on attachment 97109 [details]
Patch

Patch looks OK; changelog could use more work.

[Eric Seidel (no email)]

Comment on attachment 97109 [details]
Patch

Looks OK to me.

[Gyuyoung Kim]

Comment on attachment 97109 [details]
Patch

Informal r+

[Eunmi Lee]

Created attachment 97207 [details]
Patch

[Eunmi Lee]

(In reply to comment #10)
> Created an attachment (id=97207) [details]
> Patch

modify "use" to "Use"

[Gyuyoung Kim]

Comment on attachment 97207 [details]
Patch

Internal r+

[WebKit Review Bot]

Comment on attachment 97207 [details]
Patch

Clearing flags on attachment: 97207

Committed r88907: <http://trac.webkit.org/changeset/88907>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

This patch is wrong. makeString is deprecated, using String operator+ gives exactly the same performance. This has been changed recently. 

use "osVersion = name.sysname + " " + name.machine;"
You are not required to cast to Strings first.

The StringAppend template solution will reduce this String concatting operation to just a single malloc of the final size, no appending is done at all.

[Leandro Pereira]

(In reply to comment #15)
> This patch is wrong. makeString is deprecated, using String operator+ gives
> exactly the same performance. This has been changed recently. 
> 
> use "osVersion = name.sysname + " " + name.machine;"
> You are not required to cast to Strings first.
> 
> The StringAppend template solution will reduce this String concatting operation
> to just a single malloc of the final size, no appending is done at all.

You're right. I've rolled this out as r88944. Changing this bug status to 'invalid'.