Bug 95808

Summary: [EFL] Remove '+=' usage in String
Product: WebKit Reporter: Kangil Han <kangil.han>
Component: WebKit EFLAssignee: Kangil Han <kangil.han>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, cdumez, gyuyoung.kim, kenneth, lucas.de.marchi, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
patch
none
patch
none
patch
gyuyoung.kim: review-
patch
benjamin: review+, benjamin: commit-queue-
patch none

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
DetailsFormatted DiffDiff
patch (5.59 KB, patch)
2012-09-04 23:51 PDT, Kangil Han 		no flags
DetailsFormatted DiffDiff
patch (5.59 KB, patch)
2012-09-04 23:52 PDT, Kangil Han 		gyuyoung.kim: review-
DetailsFormatted DiffDiff
patch (5.75 KB, patch)
2012-09-05 00:57 PDT, Kangil Han 		benjamin: review+
benjamin: commit-queue-
DetailsFormatted DiffDiff
patch (5.73 KB, patch)
2012-09-05 03:43 PDT, Kangil Han 		no flags
DetailsFormatted DiffDiff
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.