Bug 48199 – [GTK] Implement DumpRenderTreeSupportGtk (similarly to DumpRenderTreeSupportQt idea)

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Antonio Gomes]

As it is today, improving Gtk's layout test coverage through DRT generally requires the addition of new API (either new methods to webkitweb{view,frame,settings} and friends, or a new signal, etc). This has to go through the API review process, and sometimes, there is not real use case other than the layout tests. To workaround this situation, developers sometimes even have to add hidden API (undocument signals and methods, etc).

QtWebKit has came up with a solution to the issue: DumpRenderTreeSupportQt. It consists on a bunch of static methods that provide direct access to WebCore features.

Talking to Martin Robinson, he also agree this is something wanted by webkitgtk. Lets try to come up with a solution here...

[Antonio Gomes]

Created attachment 71664 [details]
patch v0.1 - feedback wanted

I quickly prototyped that on Friday and showed it to Martin on IRC.

Summary:

1) Patch add WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp|h. This class will basically group together a bunch of static methods to be called by LayoutTestControllerGtk. (see for example http://trac.webkit.org/browser/trunk/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp)

2) As an illustration, I added the linksIncludedInFocusChain method, which should fix bug 47793 and unskip fast/events/tab-focus-anchor.html.

To make it to link, so far I did:

diff --git a/WebKitTools/GNUmakefile.am b/WebKitTools/GNUmakefile.am
index 5257ece..faa1dd5 100644
--- a/WebKitTools/GNUmakefile.am
+++ b/WebKitTools/GNUmakefile.am
@@ -97,6 +97,7 @@ Programs_DumpRenderTree_CFLAGS = \
 
 Programs_DumpRenderTree_LDADD = \
        libwebkitgtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la \
+       WebKit/gtk/WebCoreSupport/.libs/libwebkitgtk_1_0_la-DumpRenderTreeSupportGtk.o \

please advice a better way. I am not pleased with that (and it is been possibly making the patch not-functional ...)

Also if this can be improved, it would be great:

--- a/WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp
+++ b/WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp
@@ -32,6 +32,7 @@
 #include "config.h"
 #include "DumpRenderTree.h"
 
+#include "../../../WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h"



Thoughts?

[WebKit Review Bot]

Attachment 71664 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

Created attachment 71665 [details]
patch v0.2 - feedback wanted

same as v0.1, but builds and also fixed the below code:
#include "../../../WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h" is now #include "WebCoreSupport/DumpRenderTreeSupportGtk.h"

However the below is still a problem:

 Programs_DumpRenderTree_LDADD = \
 	libwebkitgtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la \
+	WebKit/gtk/WebCoreSupport/.libs/libwebkitgtk_1_0_la-DumpRenderTreeSupportGtk.o \

[WebKit Review Bot]

Attachment 71665 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

>  Programs_DumpRenderTree_LDADD = \
>      libwebkitgtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la \
> +    WebKit/gtk/WebCoreSupport/.libs/libwebkitgtk_1_0_la-DumpRenderTreeSupportGtk.o \

Is it not possible to simply list DumpRenderTreeSupportGtk.cpp in the same way that the other WebCoreSupport source files are listed?

[Antonio Gomes]

(In reply to comment #5)
> >  Programs_DumpRenderTree_LDADD = \
> >      libwebkitgtk-@WEBKITGTK_API_MAJOR_VERSION@.@WEBKITGTK_API_MINOR_VERSION@.la \
> > +    WebKit/gtk/WebCoreSupport/.libs/libwebkitgtk_1_0_la-DumpRenderTreeSupportGtk.o \
> 
> Is it not possible to simply list DumpRenderTreeSupportGtk.cpp in the same way that the other WebCoreSupport source files are listed?

Not sure if I understood your question well. To get built, both DumpRenderTreeSupportGtk.cpp|h are build together with other webcoresupport files, as seen below:

diff --git a/GNUmakefile.am b/GNUmakefile.am
--- a/GNUmakefile.am
+++ b/GNUmakefile.am
@@ -362,6 +362,8 @@ webkitgtk_sources += \
 	WebKit/gtk/WebCoreSupport/EditorClientGtk.h \
 	WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp \
 	WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.h \
+	WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp \
+	WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h \

However, GTK's DRT seems to just see Webkit/gtk/webkit symbols, so I had to manually add the .o , so the DRT links.

[Martin Robinson]

I believe this is controlled by the ldd version script at autotools/symbols.filter. Currently it appears to be filtering all mangled symbols except for _ZN3WTF10fastMallocEj, _ZN3WTF11fastReallocEPvj, _ZN3WTF8fastFreeEPv, and _ZN3WTF10fastCallocEjj. Perhaps adding the DRTSupport symbols to this list via a wildcard will prevent needing to list it in the LDADD. line

[Antonio Gomes]

(In reply to comment #7)
> I believe this is controlled by the ldd version script at autotools/symbols.filter. Currently it appears to be filtering all mangled symbols except for _ZN3WTF10fastMallocEj, _ZN3WTF11fastReallocEPvj, _ZN3WTF8fastFreeEPv, and _ZN3WTF10fastCallocEjj. Perhaps adding the DRTSupport symbols to this list via a wildcard will prevent needing to list it in the LDADD. line

It works! Thanks Martin. Patch coming soon ...

[WebKit Review Bot]

Attachment 71665 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4786002

[Antonio Gomes]

Created attachment 71977 [details]
(committed with r70609, r=mrobinson) patch v1 - initial version of DumpRenderTreeSupportGtk.

Patch incorporates Martin's suggestion for the first two patches.

Summary:
- Implements an initial version of DumpRenderTreeSupportGtk class, that will be used as a bridge for DRT to access WebCore features. It only comprises support for the WebKitTabToLinksPreferenceKey property so far, as an exemplification, but follow up patches will populate it properly.
- Unskip the associated test.

[WebKit Review Bot]

Attachment 71977 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antonio Gomes]

> Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:24:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 11 files

Fixed locally.

[Martin Robinson]

Comment on attachment 71977 [details]
(committed with r70609, r=mrobinson) patch v1 - initial version of DumpRenderTreeSupportGtk.

View in context: https://bugs.webkit.org/attachment.cgi?id=71977&action=review

Great patch! Thanks for doing this. Please consider fixing my nits before landing.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:39
> +void DumpRenderTreeSupportGtk::setDumpRenderTreeModeEnabled(bool b)

I think instead of 'b' here this should be 'enabled' or something similar.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:48
> +void DumpRenderTreeSupportGtk::setLinksIncludedInFocusChain(bool b)

Same here.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:5
> +    Copyright (C) 2010 Antonio Gomes <[e-mail]>
> +
> +    This library is free software; you can redistribute it and/or
> +    modify it under the terms of the GNU Library General Public

This is super-nitful, but do you mind adding the asterisks along the left side of your license blocks so they match the rest of the blocks in the directory?

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:33
> +    static void setDumpRenderTreeModeEnabled(bool b);
> +    static bool dumpRenderTreeModeEnabled();
> +
> +    static void setLinksIncludedInFocusChain(bool b);
> +    static bool linksIncludedInFocusChain();

No need for the parameter names on these declarations.

[Antonio Gomes]

Comment on attachment 71977 [details]
(committed with r70609, r=mrobinson) patch v1 - initial version of DumpRenderTreeSupportGtk.

Clearing review flags on attachment: 71997

Committed with r70609: <http://trac.webkit.org/changeset/70609>

[Antonio Gomes]

Filed bug 48429 about populating DumpRenderTreeSupportGtk.