Summary: | [chromium] turn TestRunner library into a component build | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jochen | ||||||||||||||||
Component: | Tools / Tests | Assignee: | jochen | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, dpranke, fishd, gavinp, jamesr, tkent+wkapi, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 109047, 109095 | ||||||||||||||||||
Bug Blocks: | 91308 | ||||||||||||||||||
Attachments: |
|
Description
jochen
2013-01-31 05:19:16 PST
Created attachment 186697 [details]
Patch
Created attachment 186701 [details]
Patch
I verified that it builds in a chromium component build. 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. +dirk for component build insights 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/ 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 (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. (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? (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. 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! 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. 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 Comment on attachment 186701 [details]
Patch
build/gyp changes look fine to me.
why did you remove the OVERRIDE annotations from TestRunner.h ?
Created attachment 186819 [details]
Patch for landing
(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 (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. Comment on attachment 186819 [details] Patch for landing Clearing flags on attachment: 186819 Committed r141991: <http://trac.webkit.org/changeset/141991> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 109047 Created attachment 186907 [details]
Patch
Comment on attachment 186907 [details] Patch Clearing flags on attachment: 186907 Committed r142032: <http://trac.webkit.org/changeset/142032> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 109095 Created attachment 187008 [details]
Patch
Comment on attachment 187008 [details] Patch Clearing flags on attachment: 187008 Committed r142090: <http://trac.webkit.org/changeset/142090> All reviewed patches have been landed. Closing bug. Reverted r142090 for reason: lots of selection expectations failures Committed r142109: <http://trac.webkit.org/changeset/142109> Created attachment 187155 [details]
Patch
Comment on attachment 187155 [details] Patch Clearing flags on attachment: 187155 Committed r142165: <http://trac.webkit.org/changeset/142165> All reviewed patches have been landed. Closing bug. Reverted r142165 for reason: Broke linux_aura builds. Committed r142173: <http://trac.webkit.org/changeset/142173> Created attachment 187246 [details]
Patch
Comment on attachment 187246 [details] Patch Clearing flags on attachment: 187246 Committed r142237: <http://trac.webkit.org/changeset/142237> All reviewed patches have been landed. Closing bug. |