Bug 62034

Summary: [EFL] DRT: Add DumpRenderTree.cpp
Product: WebKit Reporter: Leandro Pereira <leandro>
Component: New BugsAssignee: Leandro Pereira <leandro>
Status: RESOLVED FIXED    
Severity: Normal CC: donggwan.kim, gyuyoung.kim, kenneth, rakuco, rniwa, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 62877, 63086, 63989, 63992, 63993, 67114    
Bug Blocks: 61962, 61974    
Attachments:
Description Flags
Patch
eric: review-
Patch
none
Patch
none
Same patch with one small bug fix.
none
Patch
none
New patch with the missing DumpHistoryItem
none
Replace WTF::String with String, improve the ChangeLog
none
New patch containing only DumpRenderTree.cpp
none
Fix up getFinalTestURL() to return URL scheme even if URL contains a pixel test hash
none
Updated DumpRenderTree.cpp.
none
Minor changes, depend on bug 67114
none
Fix a few bugs, rebase on top of trunk
none
Fix the issues pointed out by tonikitoo none

Description Leandro Pereira 2011-06-03 11:50:32 PDT
[EFL] DRT: Add DumpRenderTree.cpp and DumpRenderTreeEfl.h
Comment 1 Leandro Pereira 2011-06-03 11:53:55 PDT
Created attachment 95940 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-06-10 12:19:42 PDT
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?
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-06-21 11:01:25 PDT
Created attachment 98012 [details]
Patch
Comment 4 Raphael Kubo da Costa (:rakuco) 2011-06-21 12:25:21 PDT
Created attachment 98042 [details]
Patch
Comment 5 Raphael Kubo da Costa (:rakuco) 2011-06-21 13:19:35 PDT
Created attachment 98048 [details]
Same patch with one small bug fix.

Fix one small linebreak issue in the code.
Comment 6 WebKit Review Bot 2011-06-21 13:22:14 PDT
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.
Comment 7 Raphael Kubo da Costa (:rakuco) 2011-06-22 07:27:09 PDT
Created attachment 98167 [details]
Patch
Comment 8 Raphael Kubo da Costa (:rakuco) 2011-06-22 07:27:38 PDT
(In reply to comment #7)
> Created an attachment (id=98167) [details]
> Patch

This is the same patch, now trying to please the bots.
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-06-27 07:46:04 PDT
Created attachment 98723 [details]
New patch with the missing DumpHistoryItem
Comment 10 Raphael Kubo da Costa (:rakuco) 2011-06-27 08:00:19 PDT
Created attachment 98727 [details]
Replace WTF::String with String, improve the ChangeLog
Comment 11 Raphael Kubo da Costa (:rakuco) 2011-07-06 06:46:59 PDT
Created attachment 99827 [details]
New patch containing only DumpRenderTree.cpp
Comment 12 Raphael Kubo da Costa (:rakuco) 2011-07-06 06:47:50 PDT
OK, the patch now contains only DumpRenderTree.cpp to make it easier to review. The other files have been moved to separate bug reports.
Comment 13 Raphael Kubo da Costa (:rakuco) 2011-07-14 10:01:20 PDT
Created attachment 100819 [details]
Fix up getFinalTestURL() to return URL scheme even if URL contains a pixel test hash
Comment 14 Leandro Pereira 2011-08-19 13:04:50 PDT
*** Bug 66405 has been marked as a duplicate of this bug. ***
Comment 15 Leandro Pereira 2011-08-19 13:06:28 PDT
Created attachment 104549 [details]
Updated DumpRenderTree.cpp.

EFL-specific DumpRenderTree.h isn't needed anymore.
Comment 16 Kenneth Rohde Christiansen 2011-09-12 15:46:17 PDT
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?
Comment 17 Raphael Kubo da Costa (:rakuco) 2011-09-12 17:15:13 PDT
(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).
Comment 18 Raphael Kubo da Costa (:rakuco) 2011-09-19 10:43:17 PDT
Created attachment 107881 [details]
Minor changes, depend on bug 67114
Comment 19 Raphael Kubo da Costa (:rakuco) 2011-10-03 12:25:08 PDT
Created attachment 109507 [details]
Fix a few bugs, rebase on top of trunk
Comment 20 Raphael Kubo da Costa (:rakuco) 2011-10-03 12:25:55 PDT
CC'ing reviewers.
Comment 21 Antonio Gomes 2011-10-04 08:15:12 PDT
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
Comment 22 Raphael Kubo da Costa (:rakuco) 2011-10-04 09:22:25 PDT
Created attachment 109631 [details]
Fix the issues pointed out by tonikitoo
Comment 23 Raphael Kubo da Costa (:rakuco) 2011-10-04 09:23:18 PDT
(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 24 WebKit Review Bot 2011-10-04 11:14:38 PDT
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>
Comment 25 WebKit Review Bot 2011-10-04 11:14:45 PDT
All reviewed patches have been landed.  Closing bug.