RESOLVED FIXED 108466
[chromium] turn TestRunner library into a component build
https://bugs.webkit.org/show_bug.cgi?id=108466
Summary [chromium] turn TestRunner library into a component build
jochen
Reported 2013-01-31 05:19:16 PST
that includes gyp changes and adding TESTRUNNER_EXPORT macros
Attachments
Patch (96.03 KB, patch)
2013-02-05 14:00 PST, jochen
no flags
Patch (92.79 KB, patch)
2013-02-05 14:13 PST, jochen
no flags
Patch for landing (101.18 KB, patch)
2013-02-06 04:09 PST, jochen
no flags
Patch (100.30 KB, patch)
2013-02-06 13:22 PST, jochen
no flags
Patch (100.87 KB, patch)
2013-02-07 00:46 PST, jochen
no flags
Patch (100.78 KB, patch)
2013-02-07 12:58 PST, jochen
no flags
Patch (100.76 KB, patch)
2013-02-07 23:35 PST, jochen
no flags
jochen
Comment 1 2013-02-05 14:00:37 PST
jochen
Comment 2 2013-02-05 14:13:18 PST
jochen
Comment 3 2013-02-05 14:14:13 PST
I verified that it builds in a chromium component build.
WebKit Review Bot
Comment 4 2013-02-05 14:16:53 PST
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
jochen
Comment 5 2013-02-05 14:42:43 PST
+dirk for component build insights
Adam Barth
Comment 6 2013-02-05 15:03:48 PST
Comment on attachment 186701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186701&action=review > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 > + std::auto_ptr<TestInterfaces> m_interfaces; fishd pointed me towards this web page explaining a gotcha with auto_ptr: http://anki3d.org/stl-auto_ptr-a-bad-idea/
Adam Barth
Comment 7 2013-02-05 15:05:27 PST
Comment on attachment 186701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186701&action=review >> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 >> + std::auto_ptr<TestInterfaces> m_interfaces; > > fishd pointed me towards this web page explaining a gotcha with auto_ptr: > http://anki3d.org/stl-auto_ptr-a-bad-idea/ Also http://stackoverflow.com/questions/3697686/what-is-the-problem-with-auto-ptr
jochen
Comment 8 2013-02-05 15:08:12 PST
(In reply to comment #6) > (From update of attachment 186701 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186701&action=review > > > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 > > + std::auto_ptr<TestInterfaces> m_interfaces; > > fishd pointed me towards this web page explaining a gotcha with auto_ptr: > http://anki3d.org/stl-auto_ptr-a-bad-idea/ I guess that problem can be avoided be adding a static create method to WebTestInterfaces, so nobdoy can derive from it.
jochen
Comment 9 2013-02-05 15:08:48 PST
(In reply to comment #7) > (From update of attachment 186701 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186701&action=review > > >> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 > >> + std::auto_ptr<TestInterfaces> m_interfaces; > > > > fishd pointed me towards this web page explaining a gotcha with auto_ptr: > > http://anki3d.org/stl-auto_ptr-a-bad-idea/ > > Also http://stackoverflow.com/questions/3697686/what-is-the-problem-with-auto-ptr Yes, but we don't have C++0x everywhere yet, do we?
jochen
Comment 10 2013-02-05 15:10:38 PST
(In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 186701 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=186701&action=review > > > > > Tools/DumpRenderTree/chromium/TestRunner/public/WebTestInterfaces.h:67 > > > + std::auto_ptr<TestInterfaces> m_interfaces; > > > > fishd pointed me towards this web page explaining a gotcha with auto_ptr: > > http://anki3d.org/stl-auto_ptr-a-bad-idea/ > > I guess that problem can be avoided be adding a static create method to WebTestInterfaces, so nobdoy can derive from it. Actually, even when you derive from WebTestInterfaces, this shouldn't be a problem, as WebTestInterfaces has an explicit destructor.
Adam Barth
Comment 11 2013-02-05 15:30:09 PST
Comment on attachment 186701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186701&action=review > Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:617 > -#if OS(LINUX) && USE(GTK) > +#if defined(__linux__) && USE(GTK) Does USE(GTK) still work? > Tools/DumpRenderTree/chromium/TestRunner/src/config.h:35 > +#define USE(feature) (defined WTF_USE_##feature && WTF_USE_##feature) > +#define ENABLE(feature) (defined ENABLE_##feature && ENABLE_##feature) Ah, I see!
Adam Barth
Comment 12 2013-02-05 15:31:21 PST
Yeah, we just have to be careful to have an explicit destructor. fishd said that this was probably ok. He also mentioned that we're probably depending on base already via WebURL.
WebKit Review Bot
Comment 13 2013-02-05 17:08:14 PST
Comment on attachment 186701 [details] Patch Attachment 186701 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16379836 New failing tests: editing/spelling/spellcheck-async.html pointer-lock/pointerlockchange-pointerlockerror-events.html fast/speech/scripted/speechrecognition-basics.html editing/spelling/spelling-marker-description.html fast/speech/scripted/speechrecognition-errors.html pointer-lock/pointerlockchange-event-on-lock-lost.html fast/speech/scripted/navigate-away.html pointer-lock/pointerlockelement-null-when-pending.html
Dirk Pranke
Comment 14 2013-02-05 17:56:20 PST
Comment on attachment 186701 [details] Patch build/gyp changes look fine to me. why did you remove the OVERRIDE annotations from TestRunner.h ?
jochen
Comment 15 2013-02-06 04:09:43 PST
Created attachment 186819 [details] Patch for landing
jochen
Comment 16 2013-02-06 04:28:18 PST
(In reply to comment #14) > (From update of attachment 186701 [details]) > build/gyp changes look fine to me. > > why did you remove the OVERRIDE annotations from TestRunner.h ? I didn't work. But as Adam pointed out, the webkit API already has access to base/, so I now include <base/compiler_specific.h> and put the OVERRIDE macros back in
jochen
Comment 17 2013-02-06 04:29:16 PST
(In reply to comment #11) > (From update of attachment 186701 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186701&action=review > > > Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.cpp:617 > > -#if OS(LINUX) && USE(GTK) > > +#if defined(__linux__) && USE(GTK) > > Does USE(GTK) still work? > > > Tools/DumpRenderTree/chromium/TestRunner/src/config.h:35 > > +#define USE(feature) (defined WTF_USE_##feature && WTF_USE_##feature) > > +#define ENABLE(feature) (defined ENABLE_##feature && ENABLE_##feature) > > Ah, I see! Related, I had to remove the ENABLE() #ifdefs: the feature defines are not visible via the webkit api. And since we're only using the WebKit API, we only use methods that are present regardless of the defines.
WebKit Review Bot
Comment 18 2013-02-06 05:13:01 PST
Comment on attachment 186819 [details] Patch for landing Clearing flags on attachment: 186819 Committed r141991: <http://trac.webkit.org/changeset/141991>
WebKit Review Bot
Comment 19 2013-02-06 05:13:06 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 20 2013-02-06 06:37:59 PST
Re-opened since this is blocked by bug 109047
jochen
Comment 21 2013-02-06 13:22:58 PST
WebKit Review Bot
Comment 22 2013-02-06 14:15:54 PST
Comment on attachment 186907 [details] Patch Clearing flags on attachment: 186907 Committed r142032: <http://trac.webkit.org/changeset/142032>
WebKit Review Bot
Comment 23 2013-02-06 14:15:59 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 24 2013-02-06 14:35:27 PST
Re-opened since this is blocked by bug 109095
jochen
Comment 25 2013-02-07 00:46:12 PST
jochen
Comment 26 2013-02-07 03:47:49 PST
Comment on attachment 187008 [details] Patch Clearing flags on attachment: 187008 Committed r142090: <http://trac.webkit.org/changeset/142090>
jochen
Comment 27 2013-02-07 03:47:56 PST
All reviewed patches have been landed. Closing bug.
Gavin Peters
Comment 28 2013-02-07 06:26:37 PST
Reverted r142090 for reason: lots of selection expectations failures Committed r142109: <http://trac.webkit.org/changeset/142109>
jochen
Comment 29 2013-02-07 12:58:02 PST
jochen
Comment 30 2013-02-07 13:00:01 PST
Comment on attachment 187155 [details] Patch Clearing flags on attachment: 187155 Committed r142165: <http://trac.webkit.org/changeset/142165>
jochen
Comment 31 2013-02-07 13:00:09 PST
All reviewed patches have been landed. Closing bug.
Gavin Peters
Comment 32 2013-02-07 13:40:43 PST
Reverted r142165 for reason: Broke linux_aura builds. Committed r142173: <http://trac.webkit.org/changeset/142173>
jochen
Comment 33 2013-02-07 23:35:50 PST
jochen
Comment 34 2013-02-07 23:39:53 PST
Comment on attachment 187246 [details] Patch Clearing flags on attachment: 187246 Committed r142237: <http://trac.webkit.org/changeset/142237>
jochen
Comment 35 2013-02-07 23:40:00 PST
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.