[webkit-reviews] review denied: [Bug 62034] [EFL] DRT: Add DumpRenderTree.cpp and DumpRenderTreeEfl.h : [Attachment 95940] 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 62034: [EFL] DRT: Add DumpRenderTree.cpp and DumpRenderTreeEfl.h
https://bugs.webkit.org/show_bug.cgi?id=62034

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

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

I would encourage you to go one more round on this patch and clean up what you
can.  There will be many people who touch your DRT implementation over the
years, and it's importnat to make it as readable/hackable as possible.

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:106
> +	       free(result);

Do we have to use manual free here?  No smart-pointers?  Other ports either use
WTF and C++ objects, or their own custom smart-pointers which are shared
between WebKit implementations and DRT, like GOwnPtr (gtk) or RetainPtr (mac).

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:124
> +static int compareHistoryItems(const void* item1, const void* item2)

Should all this history/backfoward stuff be in a separate file?

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:210
> +void dumpBackForwardListForAllWebViews(void)

Is the (void) needed/desired here?

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:282
> +	   if (!result) {
> +	       const char* errorMessage;
> +	       if (gLayoutTestController->dumpAsText())
> +		   errorMessage = "[documentelement innerText]";
> +	       else if (gLayoutTestController->dumpDOMAsWebArchive())
> +		   errorMessage = "[[mainFrame DOMDocument] webArchive]";
> +	       else if (gLayoutTestController->dumpSourceAsWebArchive())
> +		   errorMessage = "[[mainFrame dataSource] webArchive]";
> +	       else
> +		   errorMessage = "[mainFrame
renderTreeAsExternalRepresentation]";
> +	       printf("ERROR: nil result from %s", errorMessage);

This is a huge hack which should probably be shared between more ports.  Do we
acxtually have these error messages in the results?  Can we at least move this
logic into a separate function for possible future sharing?

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:478
> +static void initializeFonts(void)

I wonder if more of this could be shared between various linux ports of DRT.

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:539
> +	   snprintf(fontPath, PATH_MAX - 1, FONTS_CONF_DIR
"/../../fonts/WebKitWeightWatcher%i00.ttf", i);

Do you really want a relative path here?

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:704
> +    evas_object_smart_callback_add(browser, "load,started", onLoadStarted,
0);
> +    evas_object_smart_callback_add(browser, "load,finished", onLoadFinished,
0);
> +    evas_object_smart_callback_add(browser, "title,changed", onTitleChanged,
0);
> +    evas_object_smart_callback_add(browser, "window,object,cleared",
onWindowObjectCleared, 0);
> +    evas_object_smart_callback_add(browser, "statusbar,text,set",
onStatusbarTextSet, 0);

Could we put all of these callbacks on a C++ object (even though we're passing
them as C function pointers?)  Seems some way of keeping all these callbacks
together would be useful for readability of this file.

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:711
> +    evas_object_move(browser, 0, 0);
> +    evas_object_resize(browser, WIDTH, HEIGHT);
> +
> +    evas_object_layer_set(browser, EVAS_LAYER_MAX);
> +    evas_object_show(browser);
> +    ecore_evas_show(ee);

I would probably have split a bunch of this main function out into separate
setup functions.

> Tools/DumpRenderTree/efl/DumpRenderTree.cpp:738
> +shutdown:
> +    if (gcController) {
> +	   delete gcController;
> +	   gcController = 0;
> +    }
> +    if (ee)

I don't think we need to worry about freeing memory before termination.  Unless
you're trying to catch leaks this way?