[webkit-reviews] review granted: [Bug 95808] [EFL] Remove '+=' usage in String : [Attachment 162180] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Sep 5 01:17:58 PDT 2012
Benjamin Poulain <benjamin at webkit.org> has granted Kangil Han
<kangil.han at samsung.com>'s request for review:
Bug 95808: [EFL] Remove '+=' usage in String
https://bugs.webkit.org/show_bug.cgi?id=95808
Attachment 162180: patch
https://bugs.webkit.org/attachment.cgi?id=162180&action=review
------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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)");
More information about the webkit-reviews
mailing list