Summary: | [GTK] Implement DumpRenderTreeSupportGtk (similarly to DumpRenderTreeSupportQt idea) | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antonio Gomes <tonikitoo> | ||||||||
Component: | Tools / Tests | Assignee: | 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
Antonio Gomes
2010-10-23 18:27:20 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? 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.
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 \
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.
> 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?
(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. 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 (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 ... Attachment 71665 [details] did not build on gtk: Build output: http://queues.webkit.org/results/4786002 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. 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.
> 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 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 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> Filed bug 48429 about populating DumpRenderTreeSupportGtk. |