95808 – [EFL] Remove '+=' usage in String

RESOLVED FIXED 95808

[EFL] Remove '+=' usage in String

https://bugs.webkit.org/show_bug.cgi?id=95808

Summary [EFL] Remove '+=' usage in String

Kangil Han

Reported 2012-09-04 18:21:42 PDT

As adam has proposed an efficient way of string append by using StringBuilder, I think EFL port should support it.

Attachments
patch (5.69 KB, patch) 2012-09-04 23:32 PDT, Kangil Han	no flags	Details Formatted Diff Diff
patch (5.59 KB, patch) 2012-09-04 23:51 PDT, Kangil Han	no flags	Details Formatted Diff Diff
patch (5.59 KB, patch) 2012-09-04 23:52 PDT, Kangil Han	gyuyoung.kim: review-	Details Formatted Diff Diff
patch (5.75 KB, patch) 2012-09-05 00:57 PDT, Kangil Han	benjamin: review+ benjamin: commit-queue-	Details Formatted Diff Diff
patch (5.73 KB, patch) 2012-09-05 03:43 PDT, Kangil Han	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2012-09-04 22:10:36 PDT

I was planning to work on this as well but it seems you were faster :)

Kangil Han

Comment 2 2012-09-04 23:32:59 PDT

Created attachment 162165 [details] patch

Chris Dumez

Comment 3 2012-09-04 23:42:24 PDT

Comment on attachment 162165 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=162165&action=review > Source/WebKit/efl/ewk/ewk_frame.cpp:1665 > + builder.clear(); Useless since build.isEmpty() was true.

Kangil Han

Comment 4 2012-09-04 23:51:17 PDT

Created attachment 162169 [details] patch Done!

Kangil Han

Comment 5 2012-09-04 23:52:08 PDT

Created attachment 162170 [details] patch Try again.

Gyuyoung Kim

Comment 6 2012-09-04 23:52:27 PDT

Comment on attachment 162169 [details] patch LGTM.

Benjamin Poulain

Comment 7 2012-09-05 00:05:18 PDT

Comment on attachment 162170 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=162170&action=review Some comments about efficiency: > Source/WebKit/efl/ewk/ewk_frame.cpp:1679 > - size_t sourceLength = strlen(source.utf8().data()); > + size_t sourceLength = strlen(builder.toString().utf8().data()); > *frameSource = static_cast<char*>(malloc(sourceLength + 1)); > if (!*frameSource) { > CRITICAL("Could not allocate memory."); > return -1; > } > > - strncpy(*frameSource, source.utf8().data(), sourceLength); > + strncpy(*frameSource, builder.toString().utf8().data(), sourceLength); > (*frameSource)[sourceLength] = '\0'; The previous code was fairly bad, that's an opportunity to fix the logic here. Every time you create a CString, you are allocating new memory and copying the whole String. What you should do is CString utf8String = builder.toString().utf8(). then use the length and data from the CString. Avoid the strlen() here, it is a waste of time, especially for long strings. > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:377 > + StringBuilder builder; > + builder.append("<NSURLRequest URL "); > + builder.append(pathSuitableForTestResult(request->url).data()); > + builder.append(", main document URL "); > + builder.append(urlSuitableForTestResult(request->first_party).data()); > + builder.append(", http method "); > + builder.append(request->http_method ? String(request->http_method) : "(none)"); > + builder.append(">"); > + return builder.toString().utf8(); Here you should be using appendLiteral() for he strings literal. ">" -> '>'. > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:391 > + StringBuilder builder; > + builder.append("<NSURLResponse "); > + builder.append(pathSuitableForTestResult(response->url).data()); > + builder.append(", http status code "); > + builder.append(String::number(response->status_code)); > + builder.append(">"); > + return builder.toString().utf8(); Ditto.

Gyuyoung Kim

Comment 8 2012-09-05 00:09:51 PDT

Comment on attachment 162170 [details] patch Benjamin Thank you for your comment. Set r- again.

Kenneth Rohde Christiansen

Comment 9 2012-09-05 00:31:28 PDT

Comment on attachment 162170 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=162170&action=review > Source/WebKit/efl/ewk/ewk_frame.cpp:1662 > // Try to get <head> and <body> tags if <html> tag was not found. Does this happen? Doesn't webkit insert that automatically, if missing?

Grzegorz Czajkowski

Comment 10 2012-09-05 00:49:04 PDT

Comment on attachment 162170 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=162170&action=review >> Source/WebKit/efl/ewk/ewk_frame.cpp:1662 >> // Try to get <head> and <body> tags if <html> tag was not found. > > Does this happen? Doesn't webkit insert that automatically, if missing? You're right, tags <head>, <body> will be inserted automatically. The above loop looks for the node that contains <html> tag. If we find that node, it will contains all nodes (including <body> , <head>). This part you are asking is responsible for dumping pages without the <html> tag (I am not sure if WebKit adds it automatically) like java scripts source included in <head> etc.

Benjamin Poulain

Comment 11 2012-09-05 00:53:13 PDT

> > Does this happen? Doesn't webkit insert that automatically, if missing? Please, in any case, update this separately. Let's fix the strings first so we can move forward improving WTF.

Kangil Han

Comment 12 2012-09-05 00:56:35 PDT

(In reply to comment #11) > > > Does this happen? Doesn't webkit insert that automatically, if missing? > > Please, in any case, update this separately. > > Let's fix the strings first so we can move forward improving WTF. Okay, I will create a separate bug for that. :-)

Kangil Han

Comment 13 2012-09-05 00:57:36 PDT

Created attachment 162180 [details] patch Done!

Kangil Han

Comment 14 2012-09-05 01:07:02 PDT

(In reply to comment #9) > (From update of attachment 162170 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162170&action=review > > > Source/WebKit/efl/ewk/ewk_frame.cpp:1662 > > // Try to get <head> and <body> tags if <html> tag was not found. > > Does this happen? Doesn't webkit insert that automatically, if missing? Kenneth is right. WebKit automatically insert '<html>' and '</html>' tags. I will soon get back with new patch. ;-)

Chris Dumez

Comment 15 2012-09-05 01:08:44 PDT

Comment on attachment 162180 [details] patch LGTM.

Benjamin Poulain

Comment 16 2012-09-05 01:18:12 PDT

Comment on attachment 162180 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=162180&action=review Globally, the patch looks good. The new/delete/malloc/free is a big issue. I let you clear that out for landing. > Source/WebKit/efl/ewk/ewk_frame.cpp:1679 > - size_t sourceLength = strlen(source.utf8().data()); > - *frameSource = static_cast<char*>(malloc(sourceLength + 1)); > + CString utf8String = builder.toString().utf8(); > + size_t sourceLength = utf8String.length(); > + *frameSource = new char[sourceLength + 1]; > if (!*frameSource) { > CRITICAL("Could not allocate memory."); > return -1; > } > > - strncpy(*frameSource, source.utf8().data(), sourceLength); > + strncpy(*frameSource, utf8String.data(), sourceLength); Much nicer! I would not change malloc<->new without knowing how the memory is later freed. The operator new can be overloaded/replaced. If some code use one allocator to allocate the memory, and an other allocator to free the memory, you are gonna be in trouble. If the client code of ewk_frame_source_get() uses free(), you should use malloc here. > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:375 > + builder.append(request->http_method ? String(request->http_method) : "(none)"); Ideally, this would be: if (request->http_method) builder.append(String(request->http_method)); else builder.appendLiteral("(none)");

Kangil Han

Comment 17 2012-09-05 03:43:17 PDT

Created attachment 162210 [details] patch

Kangil Han

Comment 18 2012-09-05 03:47:53 PDT

> I would not change malloc<->new without knowing how the memory is later freed. > The operator new can be overloaded/replaced. If some code use one allocator to allocate the memory, and an other allocator to free the memory, you are gonna be in trouble. > > If the client code of ewk_frame_source_get() uses free(), you should use malloc here. > Agreed since this WK1 API has been introduced for some while. For compatibility, even though current coding style discourages to use of malloc/free, I would keep current implementation. > > Tools/DumpRenderTree/efl/DumpRenderTreeChrome.cpp:375 > > + builder.append(request->http_method ? String(request->http_method) : "(none)"); > > Ideally, this would be: > if (request->http_method) > builder.append(String(request->http_method)); > else > builder.appendLiteral("(none)"); Done!

Chris Dumez

Comment 19 2012-09-05 04:03:10 PDT

(In reply to comment #18) > > I would not change malloc<->new without knowing how the memory is later freed. > > The operator new can be overloaded/replaced. If some code use one allocator to allocate the memory, and an other allocator to free the memory, you are gonna be in trouble. > > > > If the client code of ewk_frame_source_get() uses free(), you should use malloc here. > > > > Agreed since this WK1 API has been introduced for some while. > For compatibility, even though current coding style discourages to use of malloc/free, I would keep current implementation. This is a public C API so using "new" here would simply be wrong. The client can only call free() so you need to use malloc(). The EFL coding style discourages the use of malloc/free for *private* implementation.

Kangil Han

Comment 20 2012-09-05 04:20:13 PDT

(In reply to comment #19) > (In reply to comment #18) > > > I would not change malloc<->new without knowing how the memory is later freed. > > > The operator new can be overloaded/replaced. If some code use one allocator to allocate the memory, and an other allocator to free the memory, you are gonna be in trouble. > > > > > > If the client code of ewk_frame_source_get() uses free(), you should use malloc here. > > > > > > > Agreed since this WK1 API has been introduced for some while. > > For compatibility, even though current coding style discourages to use of malloc/free, I would keep current implementation. > > This is a public C API so using "new" here would simply be wrong. The client can only call free() so you need to use malloc(). The EFL coding style discourages the use of malloc/free for *private* implementation. Thanks for kind explanation.

WebKit Review Bot

Comment 21 2012-09-05 10:05:17 PDT

Comment on attachment 162210 [details] patch Clearing flags on attachment: 162210 Committed r127604: <http://trac.webkit.org/changeset/127604>

WebKit Review Bot

Comment 22 2012-09-05 10:05:23 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Other

OS Linux

Product WebKit

Component WebKit EFL

Assignee

Kangil Han

Reported

2012-09-04 18:21 PDT

Modified

2012-09-05 10:05 PDT History

CC List

8 users Show

URL

Keywords

Depends on

Blocks