Currently the Gtk+ port is not doing any regression testing. This bug is supposed to change it. I would like to do the following 1.) Skip every test 2.) Enable the tests that succeed and classify the non succeeding tests in crash, fail, timeout/hang, new result, so that we can do something about it one by one.
Created attachment 24845 [details] Disable all tests. Make run-webkit-tests --gtk succeed by disabling all tests.
Created attachment 24846 [details] Classify the fast/ tests. Only skip the tests that crash, have a wrong result or generate new results.
On run-webkit-tests with an empty Skipped file and a little fix to get the svg tests tested, 4169 (43%) tests succeed, 565 (5%) fail, 4765 (50%) tests are new, 4 time out and one crashes, while 267 (2%) tests have stderr output (all approx.*). To separate tests by different results, I've modified run-webkit-tests script to distribute test locations into files assigned to that specific result. Diff and files attachments follow. * some tests do not always time out/fail, e.g. animations/big-rotation.html does not always succeed.
Created attachment 24986 [details] run-webkit-tests diff It'd be useful if anyone else runs this script so anomalies could be targeted.
Created attachment 24987 [details] succeeding tests
Created attachment 24988 [details] failing tests
Created attachment 24989 [details] new tests
Created attachment 24990 [details] crashing or timing-out tests
(In reply to comment #4) > Created an attachment (id=24986) [edit] > run-webkit-tests diff > > It'd be useful if anyone else runs this script so anomalies could be targeted. Do you know the review flag? If you want to ask for inclusion (I think you should), you should toggle this flag. See http://webkit.org/coding/contributing.html for more details.
For the reference: - Making Curl not print to stdout is fixing some tests (patch coming) - I currently hunt a bug in DRT or WebCore/editing of not adding a space " " to our text only results.
(In reply to comment #9) > (In reply to comment #4) > > Created an attachment (id=24986) [edit] > > run-webkit-tests diff > > > > It'd be useful if anyone else runs this script so anomalies could be targeted. > > Do you know the review flag? If you want to ask for inclusion (I think you > should), you should toggle this flag. See > http://webkit.org/coding/contributing.html for more details. > It was not meant as an inclusion, just to show publicly how the tests were separated. Otherwise, yes, I do know the review flag. Furthermore, a few things I've noticed that would improve results: - allowing JavaScript access the clipboard - implementing EventSender in gtk part of DumpRenderTree - should be fun - deleting a word also deletes the following whitespace Also, new tests should be, as suggested, compared to those of mac port. At the moment, I'm diffing a pair of results with a python script which then counts changed lines and detects any added or deleted ones. Tested on tests in css1 directory, 30 tests out of 106 have more or less lines than generated tests from mac port. In most cases, this is text continuing in new line. Getting the buildbot to run layout tests on gtk port should also be necessary, so we can have a neutral machine running the tests.
(In reply to comment #11) > Furthermore, a few things I've noticed that would improve results: > - deleting a word also deletes the following whitespace which test do you have in mind? Editing tests or in general? I currently attempt to understand TextItertator in WebCore/editing. > Also, new tests should be, as suggested, compared to those of mac port. At the > moment, I'm diffing a pair of results with a python script which then counts > changed lines and detects any added or deleted ones. Tested on tests in css1 > directory, 30 tests out of 106 have more or less lines than generated tests > from mac port. In most cases, this is text continuing in new line. Yes, and this can have at least two reasons: - We don't use the right fonts (e.g. you need the webkit test fonts from the Nokia/Qt Software site), DRT should add them using fontconfig.. same as the Qt version of DumpRenderTree - We don't set the DPI to 72 for the tests. > Getting the buildbot to run layout tests on gtk port should also be necessary, > so we can have a neutral machine running the tests. Right, I will have to ask Mark to do the change.
(In reply to comment #12) > > > Furthermore, a few things I've noticed that would improve results: > > - deleting a word also deletes the following whitespace > > which test do you have in mind? Editing tests or in general? I currently > attempt to understand TextItertator in WebCore/editing. http://trac.webkit.org/browser/trunk/LayoutTests/editing/deleting/5729680.html is the one I noticed. Even though EventSender is not implemented, replacing keyDown command with document.execCommand("Delete") does the job. The result is, however, "This a test", and I believe the problem is in smart deleting, as r30213 suggests - http://trac.webkit.org/changeset/30213
Gtk side of DumpRenderTree also lacks an implementation of outputting editing callbacks, but this can for now easily be bypassed with using "--strip-editing-callbacks" on run-webkit-tests. Using this brings some more tests to success state.
Created attachment 25195 [details] Add WIP implementation of EventSender for DRT Work in progress implementation to send events...
Created attachment 25277 [details] WIP implementation of test Netscape plugin Implements the Netscape plugin for testing. There are 14-15 tests failing, half of them require ObjC plugin, others need some more hacking to get them running. There are also 2 tests crashing.
Comment on attachment 24845 [details] Disable all tests. Clearing review. the patch is in.
Comment on attachment 24846 [details] Classify the fast/ tests. clearing review too.
Created attachment 25397 [details] First attempt at final patch for TestNetscapePlugin implementation
Comment on attachment 25397 [details] First attempt at final patch for TestNetscapePlugin implementation > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 38694) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,14 @@ > +2008-11-23 Zan Dobersek <zandobersek@gmail.com> > + > + Reviewed by NOBODY (OOPS!). > + > + Add "/tmp/webkit-test-plugin" to PluginDatabase's paths on Gtk+ > + port so run-webkit-tests script has a proper place to copy test > + plugin to. > + > + * plugins/PluginDatabase.cpp: > + (WebCore::PluginDatabase::defaultPluginDirectories): > + > 2008-11-22 Nikolas Zimmermann <nikolas.zimmermann@torchmobile.com> > > Reviewed by Holger Freyther. > Index: WebCore/plugins/PluginDatabase.cpp > =================================================================== > --- WebCore/plugins/PluginDatabase.cpp (revision 38694) > +++ WebCore/plugins/PluginDatabase.cpp (working copy) > @@ -300,6 +300,13 @@ Vector<String> PluginDatabase::defaultPl > paths.append("/usr/lib64/netscape/plugins") > paths.append("/usr/lib64/mozilla/plugins"); > > +#if PLATFORM(GTK) > + // On GTK port, test plugin gets copied into a temporary directory > + // FIXME: Make this directory changeable through environment variable > + // that run-webkit-tests sets > + paths.append("/tmp/webkit-test-plugin"); > +#endif > + Sorry for the drastic review. But this is a security issue and I don't want anyone to say r=+. Anyone can create /tmp/webkit-test-plugin and apps like epiphany/midori will load random plugins.... >
Comment on attachment 25397 [details] First attempt at final patch for TestNetscapePlugin implementation > +TestNetscapePlugin_libtestnetscapeplugin_la_SOURCES = \ > + WebKitTools/DumpRenderTree/gtk/TestNetscapePlugin/PluginObject.cpp \ > + WebKitTools/DumpRenderTree/gtk/TestNetscapePlugin/PluginObject.h \ > + WebKitTools/DumpRenderTree/gtk/TestNetscapePlugin/TestObject.cpp \ > + WebKitTools/DumpRenderTree/gtk/TestNetscapePlugin/TestObject.h ^^^^^ these can be taken directly from the shared WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/ directory. There is no reason to have a 1:1 copy > + WebKitTools/DumpRenderTree/gtk/TestNetscapePlugin/TestNetscapePlugin.cpp \ okay, this is copied from the main.cpp of the above directory. The only difference is the handle event method? > Index: WebKitTools/Scripts/run-webkit-tests > =================================================================== > --- WebKitTools/Scripts/run-webkit-tests (revision 38694) > +++ WebKitTools/Scripts/run-webkit-tests (working copy) > @@ -334,6 +334,21 @@ my %testType = (); > > system "ln", "-s", $testDirectory, "/tmp/LayoutTests" unless -x "/tmp/LayoutTests"; > > +# On GTK port, copy libtestnetscapeplugin.so into temporary directory > +# FIXME: Make this temporary directory possible to change through some argument > +# and set the environment variable PluginDatabase can then use. a) you can cheat with the MOZHOME environment variable. You just need $MOZHOME/plugins available. b) we can add SemiPublicInterface to add a plugin path... or just add proper API and call it from DumPRenderTree > Index: LayoutTests/platform/gtk/Skipped > =================================================================== > --- LayoutTests/platform/gtk/Skipped (revision 38694) > +++ LayoutTests/platform/gtk/Skipped (working copy) cool
Created attachment 25398 [details] Second attempt at final patch for TestNetscapePlugin implementation
(In reply to comment #21) > okay, this is copied from the main.cpp of the above directory. The only > difference is the handle event method? At first, only the skeleton was taken - basic functions that are empty. The engine did not load the plugin, so I took a look at the Totem's plugin and rewrote this one to work in the same way. So this plugin basically has structure of Totem's plugin and methods of the plugin in TestNetscapePlugIn.subproj/main.cpp.
Comment on attachment 25398 [details] Second attempt at final patch for TestNetscapePlugin implementation > +void FrameLoaderClient::addExtraPluginDirectory(const String& directory) > +{ > + PluginDatabase::installedPlugins()->addExtraPluginDirectory(directory); > +} > + why do you add this level of abstraction? Calling PluginDatabase directly from webkitwebview is not doing any harm. > +void webkit_web_view_add_extra_plugin_directory(WebKitWebView* webView, const gchar* directory) > +{ > + g_return_if_fail(WEBKIT_IS_WEB_VIEW(webView)); > + > + Frame* coreFrame = core(webkit_web_view_get_main_frame(webView)); > + WebKit::FrameLoaderClient* client = static_cast<WebKit::FrameLoaderClient*>(coreFrame->loader()->client()); > + > + client->addExtraPluginDirectory(String(directory)); PluginDatabase::installedPlugins()->addExtraPluginDirectory(filenameToString(directory)); the constructor you pick will convert from ASCII to the string which is almost never correct. > Index: WebKit/gtk/webkit/webkitwebview.h > =================================================================== > --- WebKit/gtk/webkit/webkitwebview.h (revision 38694) > +++ WebKit/gtk/webkit/webkitwebview.h (working copy) > @@ -277,6 +277,10 @@ WEBKIT_API void > webkit_web_view_set_full_content_zoom (WebKitWebView *web_view, > gboolean full_content_zoom); > > +WEBKIT_API void > +webkit_web_view_add_extra_plugin_directory (WebKitWebView *web_view, > + const gchar *directory); > + Could you move that to webkitprivate.h? I think the API should be added to WebKitWebSettings together with being able to completely control the paths. As this will require discussion let us start with the semi public interface. > =================================================================== > --- WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp (revision 38694) > +++ WebKitTools/DumpRenderTree/TestNetscapePlugIn.subproj/PluginObject.cpp (working copy) > @@ -23,12 +23,18 @@ > * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > */ > > +#include "config.h" ^^^ unlikely that we want that here. The plugin should not know anything about WebCore/JSC so including config.h is certainly wrong. > +#if PLATFORM(GTK) > +#include <stdlib.h> > +#include <string.h> > +#endif > + we can include that unconditionally. > +#if PLATFORM(GTK) > +#include "npfunctions.h" > +#else > #include <WebKit/npfunctions.h> > +#endif Two options: a) As you proposed on IRC add a Forwarding Header to the DumpRenderTree/gtk/TestNetscapePlugin... b) change the CPP flags to define something and use that for the condition. config.h may not be included. > +#include "config.h" > #include "TestObject.h" > + > #include "PluginObject.h" > > +#if PLATFORM(GTK) > +#include <stdlib.h> > +#include <string.h> > +#endif > + same comments. > +#if PLATFORM(GTK) > +#include "npapi.h" > +#include "npruntime.h" > +#else > #include <WebKit/npapi.h> > #include <WebKit/npruntime.h> > +#endif same options. > + > + webkit_web_view_add_extra_plugin_directory(webView, "/tmp/webkit-test-plugin"); Can't we point to the build directory? Add that to the CPPFLAGS and use that here? > > +# On GTK port, copy libtestnetscapeplugin.so into temporary directory > +if (isGtk()) { > + my $testPluginDir = catdir(productDir(), "TestNetscapePlugin/.libs"); > + my $testPluginFile = "$testPluginDir/libtestnetscapeplugin.so"; > + > + my $tmpPluginDir = "/tmp/webkit-test-plugin"; > + my $tmpPluginFile = "$tmpPluginDir/libtestnetscapeplugin.so"; > + mkdir($tmpPluginDir, 0755); > + > + copy($testPluginFile, $tmpPluginFile); > + chmod(0755, $tmpPluginFile); > +} with the above I think it is avoidable. Alternatively introduce an environment var for DRT to read and set the plugin dir.
Created attachment 25401 [details] Third attempt at final patch for TestNetscapePlugin implementation
Created attachment 25402 [details] Third attempt at final patch for TestNetscapePlugin implementation
Comment on attachment 25402 [details] Third attempt at final patch for TestNetscapePlugin implementation Looks good, I will do a final review tomorrow.
Okay I reviewed the patch and did land it. For the future please use ./WebKitTools/Scripts/prepare-ChangeLog and avoid bogus whitespace in the source files. I have added an extra include path to get it compiled. Please see http://webkit.org/coding/contributing.html (coding style and patch generation in particular). And we have 16 more test cases enabled! Good job!
Created attachment 25482 [details] A fix for plugins/plugin-javascript-access.html to succeed. Gets another test (plugins/plugin-javascript-access.html) working on Gtk+ port.
Comment on attachment 25482 [details] A fix for plugins/plugin-javascript-access.html to succeed. Better would be to re-write this test to not depend on file names. This is an ugly, old-test. I don't think we need to even have it dump the file name. Ideally it would be re-written as a Js-only test (like the tests in fast/js/resources, but that's not required. I would just remove the filename dumping from the test and gtk will pass.
Created attachment 25520 [details] A proper fix for plugins/plugin-javascript-access.html to succeed Fixed, as proposed by Eric.
Comment on attachment 25520 [details] A proper fix for plugins/plugin-javascript-access.html to succeed This fix removes of the test. Now the test doesn't check if plugin.filename works any more, which is not an improvement. Is there some other way to make the test platform independent without removing the check of the filename property altogether?
(In reply to comment #32) > (From update of attachment 25520 [details] [review]) > This fix removes of the test. Now the test doesn't check if plugin.filename > works any more, which is not an improvement. Is there some other way to make > the test platform independent without removing the check of the filename > property altogether? Right. We can add the plugin names for Linux, OSX and Windows to test and then print "OK" if the plugin name is within our known. This has the downside of moving parts of the result into the test. The other option is a platform specific test as proposed earlier.
Created attachment 25988 [details] Implementation of animation and transition pausing Since revision 39187[1], animation tests use animation pausing heavily. This patch implements these functionalities in DumpRenderTree for Gtk+ port.
Comment on attachment 25988 [details] Implementation of animation and transition pausing r=me
Comment on attachment 25988 [details] Implementation of animation and transition pausing > - // FIXME: implement > + gchar* name = JSStringCopyUTF8CString(propertyName); > + gchar* element = JSStringCopyUTF8CString(elementId); > + return webkit_web_frame_pause_animation(mainFrame, name, time, element); animation??? I will make this transition... > }
Comment on attachment 25988 [details] Implementation of animation and transition pausing Landed in r39273.
Created attachment 26053 [details] Enabling more tests Brings the number of tested tests to about 4000.
(In reply to comment #38) > Created an attachment (id=26053) [review] > Enabling more tests > > Brings the number of tested tests to about 4000. On the buildbot and also here we have a couple of regressions (but this issue is weird). I'm not certain if we should increase the number of tests even if we have the current issues?
(In reply to comment #39) > On the buildbot and also here we have a couple of regressions (but this issue > is weird). Tests on the buildbot mostly fail because of the bidi bug, other reasons are lack of implementation of GCController[1] and EventSender[2] in the DRT, build without video support[3], selections bug[4] and a bug in document layout which I think is connected to bug #22842[5]. Until fixed, we can skip them with no hard feelings. > I'm not certain if we should increase the number of tests even if we > have the current issues? > As I've tested, all the tests enabled by that patch pass. [1] fast/dom/Window/timeout-released-on-close-diffs.txt [2] fast/events/special-key-events-in-input-text-diffs.txt [3] fast/dom/dom-constructors-diffs.txt [4] fast/forms/textarea-selection-preservation-diffs.txt [5] https://bugs.webkit.org/show_bug.cgi?id=22842
Comment on attachment 26053 [details] Enabling more tests Okay, next thing is to reclassify the former crashing tests (all crashes should be gone)
Comment on attachment 25520 [details] A proper fix for plugins/plugin-javascript-access.html to succeed Darin is right, this is making the test more weak. I think my plan to use plugin.filename and to compare to a list of good filenames would be acceptable as well, or we can go back to your initial patch of a custom result. Whatever you prefer.
In discussion[1] on the webkit-dev mailing list, I proposed some methods to push layout tests testing forward a bit. Since this is a Gtk-specific issue and the thread is not, I'll explain it a bit more here. The idea is to empty the Skipped file and fill it with all the tests that are generating new results. We shouldn't start applying these results into trunk until proper work is done in DRT - complete implementation of EventSender, AccessibilityUIElement, AccessibilityController GCController etc., proper fonts for DRT, font size and so on. Only then should a script be assembled to start semantically diffing expected results for Mac and Gtk ports, port them into trunk and do troubleshooting if needed. About `semantically` - pixel-for-pixel rendering between Mac and Gtk port is improbable. Render dumps would be in such script dissected and checked for consistence in proper rendering element and content. After that, many failures and timeouts would be seen on the buildbot. Tests with a same cause of failure would then be grouped and added to the Skipped list, and a proper bug report would be filled in. With this, it would be easy to fix tests and furthermore implement the Gtk port. Prior to grouping, we should do, as proposed on the list, stressing with randomizing the test sequences.While this would then only be a bypassed problem that can still occur to normal users and should be debugged in the engine itself, we would gain a list of constant failures and add that to the Skipped. What I'd like is your opinion on this method. What else to do, what not to. There are at the moment some pain-in-the-arse failures on the buildbot (there are also JSC regressions, but that's not the subject of discussion here) that can be fixed in some way. _But there's no fire_, layout tests are not essential, though they are there to help see regressions and to inform developers that they've done something wrong or haven't done anything yet about a specific issue. There are now more than 10k tests being tested on Mac buildbots. 80+% would be lovely to see on the Gtk buildbot. Regards, Zan Dobersek [1] https://lists.webkit.org/pipermail/webkit-dev/2009-January/006253.html
Comment on attachment 26053 [details] Enabling more tests This patch is no longer fresh given the changes that happened in GTK+ since it was approved.
We are all using DRT... closing it.