[webkit-reviews] review denied: [Bug 61958] [EFL] DumpRenderTree: Add GCController, PixelDumpsupport and WorkQueueItem : [Attachment 95803] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Leandro Pereira
<leandro at profusion.mobi>'s request for review:
Bug 61958: [EFL] DumpRenderTree: Add GCController, PixelDumpsupport and
WorkQueueItem
https://bugs.webkit.org/show_bug.cgi?id=61958

Attachment 95803: Patch
https://bugs.webkit.org/attachment.cgi?id=95803&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95803&action=review

Same comments, basically.  This is old c code, in a c++ file.  Unless EFL style
strictly requires old-looking c code, with bad memory managmeent, we should
move thsi all to smart pointers and modern C++ styling.

> Tools/DumpRenderTree/efl/GCControllerEfl.cpp:5
> + * Copyright (C) 2007 Eric Seidel <eric at webkit.org>
> + * Copyright (C) 2011 ProFUSION Embedded Systems
> + * Copyright (C) 2011 Samsung Electronics
> + *

I doubt you need all these copyrights.

> Tools/DumpRenderTree/efl/PixelDumpSupportEfl.cpp:45
> +    cairo_surface_t* surface;
> +    cairo_t* context;
> +    int width, height;
> +    Ewk_View_Smart_Data* smartData;
> +    Ewk_View_Private_Data* privateData;
> +

Do we really need to follow this old-C limitiation for c++ files?  I doubt you
can even find a C compiler which can't handle inline declarations these days.
:)

> Tools/DumpRenderTree/efl/PixelDumpSupportEfl.cpp:50
> +    context = cairo_create(surface);

No Cairo smart pointers?  The Gtk Folks might know if we have one.

> Tools/DumpRenderTree/efl/WorkQueueItemEfl.cpp:36
> +char* JSStringCopyUTF8CString(JSStringRef jsString)
> +{
> +    size_t dataSize = JSStringGetMaximumUTF8CStringSize(jsString);
> +    char* utf8 = static_cast<char*>(malloc(dataSize));
> +    JSStringGetUTF8CString(jsString, utf8, dataSize);
> +
> +    return utf8;
> +}

Woh.  so this is what youv'e been using everywhere. I thoguth it was actually
part of the JS API.  We could easily use WTF::String here.  Since other ports
do.  Not a requirement, but there are definitely smarter string classes to use
besides char* :)