Bug 62034

Created attachment 95940 [details] Patch

Comment on attachment 95940 [details] Patch 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?

Created attachment 98012 [details] Patch

Created attachment 98042 [details] Patch

Created attachment 98048 [details] Same patch with one small bug fix. Fix one small linebreak issue in the code.

Attachment 98048 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 98167 [details] Patch

(In reply to comment #7) > Created an attachment (id=98167) [details] > Patch This is the same patch, now trying to please the bots.

Created attachment 98723 [details] New patch with the missing DumpHistoryItem

Created attachment 98727 [details] Replace WTF::String with String, improve the ChangeLog

Created attachment 99827 [details] New patch containing only DumpRenderTree.cpp

OK, the patch now contains only DumpRenderTree.cpp to make it easier to review. The other files have been moved to separate bug reports.

Created attachment 100819 [details] Fix up getFinalTestURL() to return URL scheme even if URL contains a pixel test hash

*** Bug 66405 has been marked as a duplicate of this bug. ***

Created attachment 104549 [details] Updated DumpRenderTree.cpp. EFL-specific DumpRenderTree.h isn't needed anymore.

Comment on attachment 104549 [details] Updated DumpRenderTree.cpp. Why are all these methods static? WebKit is a C++ project and mostly API are tolerated to be in C. Could you use a class for this like most other ports?

(In reply to comment #16) > (From update of attachment 104549 [details]) > Why are all these methods static? WebKit is a C++ project and mostly API are tolerated to be in C. Could you use a class for this like most other ports? Mostly because we'd just have a class with a lot of static methods with no apparent benefit -- the majority of the functions don't share data (the code which does is already in classes). In fact, we're following what GTK+'s DRT code does, and also what Qt's DumpRenderTreeQt.cpp does (to some extent).

Created attachment 107881 [details] Minor changes, depend on bug 67114

Created attachment 109507 [details] Fix a few bugs, rebase on top of trunk

CC'ing reviewers.

Comment on attachment 109507 [details] Fix a few bugs, rebase on top of trunk View in context: https://bugs.webkit.org/attachment.cgi?id=109507&action=review I know you guys have been working hard to get this in, so I wont hold it to fix typos. Congrats! > Tools/DumpRenderTree/efl/DumpRenderTree.cpp:39 > +#include "ewk_private.h" // FIXME: create some WebCoreSupport/DumpRenderTree.cpp instead it could be called DumpRenderTreeSupportEFL :) > Tools/DumpRenderTree/efl/DumpRenderTree.cpp:162 > + int option; > + > + while ((option = getopt_long(argc, (char* const*)argv, "", options, 0)) != -1) { I would add a new line before "int option" the drop the one below it. > Tools/DumpRenderTree/efl/DumpRenderTree.cpp:174 > +} > + > + > +static String getFinalTestURL(const String& testURL) drop one line

Created attachment 109631 [details] Fix the issues pointed out by tonikitoo

(In reply to comment #21) > (From update of attachment 109507 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=109507&action=review > > I know you guys have been working hard to get this in, so I wont hold it to fix typos. Thanks a lot, your help with all these patches we have been sending is very much appreciated :)

Comment on attachment 109631 [details] Fix the issues pointed out by tonikitoo Clearing flags on attachment: 109631 Committed r96618: <http://trac.webkit.org/changeset/96618>

All reviewed patches have been landed. Closing bug.