| Summary: | [EFL] Remove '+=' usage in String | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Kangil Han <kangil.han> | ||||||||||||
| Component: | WebKit EFL | Assignee: | 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
Kangil Han
2012-09-04 18:21:42 PDT
I was planning to work on this as well but it seems you were faster :) Created attachment 162165 [details]
patch
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. Created attachment 162169 [details]
patch
Done!
Created attachment 162170 [details]
patch
Try again.
Comment on attachment 162169 [details]
patch
LGTM.
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. Comment on attachment 162170 [details]
patch
Benjamin Thank you for your comment. Set r- again.
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? 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. > > 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.
(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. :-) Created attachment 162180 [details]
patch
Done!
(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. ;-) Comment on attachment 162180 [details]
patch
LGTM.
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)"); Created attachment 162210 [details]
patch
> 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! (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. (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. Comment on attachment 162210 [details] patch Clearing flags on attachment: 162210 Committed r127604: <http://trac.webkit.org/changeset/127604> All reviewed patches have been landed. Closing bug. |