WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
62034
[EFL] DRT: Add DumpRenderTree.cpp
https://bugs.webkit.org/show_bug.cgi?id=62034
Summary
[EFL] DRT: Add DumpRenderTree.cpp
Leandro Pereira
Reported
2011-06-03 11:50:32 PDT
[EFL] DRT: Add DumpRenderTree.cpp and DumpRenderTreeEfl.h
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
Show Obsolete
(12)
View All
Add attachment
proposed patch, testcase, etc.
Leandro Pereira
Comment 1
2011-06-03 11:53:55 PDT
Created
attachment 95940
[details]
Patch
Eric Seidel (no email)
Comment 2
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?
Raphael Kubo da Costa (:rakuco)
Comment 3
2011-06-21 11:01:25 PDT
Created
attachment 98012
[details]
Patch
Raphael Kubo da Costa (:rakuco)
Comment 4
2011-06-21 12:25:21 PDT
Created
attachment 98042
[details]
Patch
Raphael Kubo da Costa (:rakuco)
Comment 5
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.
WebKit Review Bot
Comment 6
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.
Raphael Kubo da Costa (:rakuco)
Comment 7
2011-06-22 07:27:09 PDT
Created
attachment 98167
[details]
Patch
Raphael Kubo da Costa (:rakuco)
Comment 8
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.
Raphael Kubo da Costa (:rakuco)
Comment 9
2011-06-27 07:46:04 PDT
Created
attachment 98723
[details]
New patch with the missing DumpHistoryItem
Raphael Kubo da Costa (:rakuco)
Comment 10
2011-06-27 08:00:19 PDT
Created
attachment 98727
[details]
Replace WTF::String with String, improve the ChangeLog
Raphael Kubo da Costa (:rakuco)
Comment 11
2011-07-06 06:46:59 PDT
Created
attachment 99827
[details]
New patch containing only DumpRenderTree.cpp
Raphael Kubo da Costa (:rakuco)
Comment 12
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.
Raphael Kubo da Costa (:rakuco)
Comment 13
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
Leandro Pereira
Comment 14
2011-08-19 13:04:50 PDT
***
Bug 66405
has been marked as a duplicate of this bug. ***
Leandro Pereira
Comment 15
2011-08-19 13:06:28 PDT
Created
attachment 104549
[details]
Updated DumpRenderTree.cpp. EFL-specific DumpRenderTree.h isn't needed anymore.
Kenneth Rohde Christiansen
Comment 16
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?
Raphael Kubo da Costa (:rakuco)
Comment 17
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).
Raphael Kubo da Costa (:rakuco)
Comment 18
2011-09-19 10:43:17 PDT
Created
attachment 107881
[details]
Minor changes, depend on
bug 67114
Raphael Kubo da Costa (:rakuco)
Comment 19
2011-10-03 12:25:08 PDT
Created
attachment 109507
[details]
Fix a few bugs, rebase on top of trunk
Raphael Kubo da Costa (:rakuco)
Comment 20
2011-10-03 12:25:55 PDT
CC'ing reviewers.
Antonio Gomes
Comment 21
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
Raphael Kubo da Costa (:rakuco)
Comment 22
2011-10-04 09:22:25 PDT
Created
attachment 109631
[details]
Fix the issues pointed out by tonikitoo
Raphael Kubo da Costa (:rakuco)
Comment 23
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 :)
WebKit Review Bot
Comment 24
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
>
WebKit Review Bot
Comment 25
2011-10-04 11:14:45 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug