Bug 48199

Summary: [GTK] Implement DumpRenderTreeSupportGtk (similarly to DumpRenderTreeSupportQt idea)
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: Tools / TestsAssignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, mrobinson, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 48398, 48429    
Bug Blocks: 47793    
Attachments:
Description Flags
patch v0.1 - feedback wanted
none
patch v0.2 - feedback wanted
none
(committed with r70609, r=mrobinson) patch v1 - initial version of DumpRenderTreeSupportGtk. none

Description Antonio Gomes 2010-10-23 18:27:20 PDT
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...
Comment 1 Antonio Gomes 2010-10-23 19:18:53 PDT
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?
Comment 2 WebKit Review Bot 2010-10-23 19:21:09 PDT
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.
Comment 3 Antonio Gomes 2010-10-23 19:34:29 PDT
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 \
Comment 4 WebKit Review Bot 2010-10-23 19:36:45 PDT
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.
Comment 5 Martin Robinson 2010-10-24 21:14:13 PDT
>  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?
Comment 6 Antonio Gomes 2010-10-24 21:28:38 PDT
(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.
Comment 7 Martin Robinson 2010-10-24 21:37:16 PDT
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
Comment 8 Antonio Gomes 2010-10-24 23:02:05 PDT
(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 ...
Comment 9 WebKit Review Bot 2010-10-25 16:00:04 PDT
Attachment 71665 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4786002
Comment 10 Antonio Gomes 2010-10-26 21:24:42 PDT
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.
Comment 11 WebKit Review Bot 2010-10-26 21:28:09 PDT
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.
Comment 12 Antonio Gomes 2010-10-26 21:42:19 PDT
> 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.
Comment 13 Martin Robinson 2010-10-26 22:05:46 PDT
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 <tonikitoo@webkit.org>
> +
> +    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.
Comment 14 Antonio Gomes 2010-10-26 22:23:29 PDT
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>
Comment 15 Antonio Gomes 2010-10-27 07:39:36 PDT
Filed bug 48429 about populating DumpRenderTreeSupportGtk.