Bug 62034 - [EFL] DRT: Add DumpRenderTree.cpp
Summary: [EFL] DRT: Add DumpRenderTree.cpp
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Pereira
URL:
Keywords:
: 66405 (view as bug list)
Depends on: 62877 63086 63989 63992 63993 67114
Blocks: 61962 61974
  Show dependency treegraph
 
Reported: 2011-06-03 11:50 PDT by Leandro Pereira
Modified: 2011-10-04 11:14 PDT (History)
7 users (show)

See Also:


Attachments
Patch (28.94 KB, patch)
2011-06-03 11:53 PDT, Leandro Pereira
eric: review-
Details | Formatted Diff | Diff
Patch (29.55 KB, patch)
2011-06-21 11:01 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Patch (29.55 KB, patch)
2011-06-21 12:25 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Same patch with one small bug fix. (28.71 KB, patch)
2011-06-21 13:19 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Patch (29.56 KB, patch)
2011-06-22 07:27 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
New patch with the missing DumpHistoryItem (36.64 KB, patch)
2011-06-27 07:46 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Replace WTF::String with String, improve the ChangeLog (36.53 KB, patch)
2011-06-27 08:00 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
New patch containing only DumpRenderTree.cpp (20.06 KB, patch)
2011-07-06 06:46 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Fix up getFinalTestURL() to return URL scheme even if URL contains a pixel test hash (20.11 KB, patch)
2011-07-14 10:01 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Updated DumpRenderTree.cpp. (13.76 KB, patch)
2011-08-19 13:06 PDT, Leandro Pereira
no flags Details | Formatted Diff | Diff
Minor changes, depend on bug 67114 (13.92 KB, patch)
2011-09-19 10:43 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Fix a few bugs, rebase on top of trunk (13.97 KB, patch)
2011-10-03 12:25 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff
Fix the issues pointed out by tonikitoo (13.88 KB, patch)
2011-10-04 09:22 PDT, Raphael Kubo da Costa (:rakuco)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.