[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