Bug 62527 - [EFL][WK2] Change string operation to makeString()
Summary: [EFL][WK2] Change string operation to makeString()
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 62732
Blocks: 61838
  Show dependency tree / graph
 
Reported: 2011-06-12 21:12 PDT by Eunmi Lee
Modified: 2011-06-15 09:20 PDT (History)
12 users (show)

See Also:

Attachments
Patch (1.83 KB, patch)
2011-06-12 21:25 PDT, Eunmi Lee 		no flags Details | Formatted Diff | Diff
Patch (1.94 KB, patch)
2011-06-14 06:40 PDT, Eunmi Lee 		no flags Details | Formatted Diff | Diff
Patch (1.94 KB, patch)
2011-06-14 18:01 PDT, Eunmi Lee 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Eunmi Lee 2011-06-12 21:12:22 PDT 
I've changed string operation codes to use makeString().
Comment 1 Eunmi Lee 2011-06-12 21:25:52 PDT 
Created attachment 96918 [details]
Patch
Comment 2 Gyuyoung Kim 2011-06-12 21:39:14 PDT 
LGTM.
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-06-13 06:10:42 PDT 
Informal r+ from my side.
Comment 4 Kenneth Rohde Christiansen 2011-06-13 07:22:11 PDT 
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'
Comment 5 Eunmi Lee 2011-06-14 06:40:37 PDT 
Created attachment 97109 [details]
Patch
Comment 6 Eunmi Lee 2011-06-14 06:41:37 PDT 
(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.
Comment 7 Leandro Pereira 2011-06-14 08:49:04 PDT 
Comment on attachment 97109 [details]
Patch

Patch looks OK; changelog could use more work.
Comment 8 Eric Seidel (no email) 2011-06-14 08:50:13 PDT 
Comment on attachment 97109 [details]
Patch

Looks OK to me.
Comment 9 Gyuyoung Kim 2011-06-14 17:09:05 PDT 
Comment on attachment 97109 [details]
Patch

Informal r+
Comment 10 Eunmi Lee 2011-06-14 18:01:49 PDT 
Created attachment 97207 [details]
Patch
Comment 11 Eunmi Lee 2011-06-14 18:02:46 PDT 
(In reply to comment #10)
> Created an attachment (id=97207) [details]
> Patch

modify "use" to "Use"
Comment 12 Gyuyoung Kim 2011-06-14 18:09:03 PDT 
Comment on attachment 97207 [details]
Patch

Internal r+
Comment 13 WebKit Review Bot 2011-06-14 23:13:17 PDT 
Comment on attachment 97207 [details]
Patch

Clearing flags on attachment: 97207

Committed r88907: <http://trac.webkit.org/changeset/88907>
Comment 14 WebKit Review Bot 2011-06-14 23:13:22 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 15 Nikolas Zimmermann 2011-06-15 07:25:39 PDT 
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.
Comment 16 Leandro Pereira 2011-06-15 09:20:09 PDT 
(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'.