Bug 62034 – [EFL] DRT: Add DumpRenderTree.cpp

• Component: New Bugs
• Status: RESOLVED FIXED

[Leandro Pereira]

[EFL] DRT: Add DumpRenderTree.cpp and DumpRenderTreeEfl.h

[Leandro Pereira]

Created attachment 95940 [details]
Patch

[Eric Seidel (no email)]

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?

[Raphael Kubo da Costa (:rakuco)]

Created attachment 98012 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Created attachment 98042 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Created attachment 98048 [details]
Same patch with one small bug fix.

Fix one small linebreak issue in the code.

[WebKit Review Bot]

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.

[Raphael Kubo da Costa (:rakuco)]

Created attachment 98167 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #7)
> Created an attachment (id=98167) [details]
> Patch

This is the same patch, now trying to please the bots.

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Leandro Pereira]

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

[Leandro Pereira]

Created attachment 104549 [details]
Updated DumpRenderTree.cpp.

EFL-specific DumpRenderTree.h isn't needed anymore.

[Kenneth Rohde Christiansen]

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?

[Raphael Kubo da Costa (:rakuco)]

(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).

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

CC'ing reviewers.

[Antonio Gomes]

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

[Raphael Kubo da Costa (:rakuco)]

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

[Raphael Kubo da Costa (:rakuco)]

(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 :)

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.