Bug 108466

Summary: [chromium] turn TestRunner library into a component build
Product: WebKit Reporter: jochen
Component: Tools / TestsAssignee: 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 Flags
Patch
none
Patch
none
Patch for landing
none
Patch
none
Patch
none
Patch
none
Patch none

Description jochen 2013-01-31 05:19:16 PST
that includes gyp changes and adding TESTRUNNER_EXPORT macros
Comment 1 jochen 2013-02-05 14:00:37 PST
Created attachment 186697 [details]
Patch
Comment 2 jochen 2013-02-05 14:13:18 PST
Created attachment 186701 [details]
Patch
Comment 3 jochen 2013-02-05 14:14:13 PST
I verified that it builds in a chromium component build.
Comment 4 WebKit Review Bot 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.
Comment 5 jochen 2013-02-05 14:42:43 PST
+dirk for component build insights
Comment 6 Adam Barth 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/
Comment 7 Adam Barth 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
Comment 8 jochen 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.
Comment 9 jochen 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?
Comment 10 jochen 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.
Comment 11 Adam Barth 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!
Comment 12 Adam Barth 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.
Comment 13 WebKit Review Bot 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
Comment 14 Dirk Pranke 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 ?
Comment 15 jochen 2013-02-06 04:09:43 PST
Created attachment 186819 [details]
Patch for landing
Comment 16 jochen 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
Comment 17 jochen 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.
Comment 18 WebKit Review Bot 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>
Comment 19 WebKit Review Bot 2013-02-06 05:13:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 2013-02-06 06:37:59 PST
Re-opened since this is blocked by bug 109047
Comment 21 jochen 2013-02-06 13:22:58 PST
Created attachment 186907 [details]
Patch
Comment 22 WebKit Review Bot 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>
Comment 23 WebKit Review Bot 2013-02-06 14:15:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 WebKit Review Bot 2013-02-06 14:35:27 PST
Re-opened since this is blocked by bug 109095
Comment 25 jochen 2013-02-07 00:46:12 PST
Created attachment 187008 [details]
Patch
Comment 26 jochen 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>
Comment 27 jochen 2013-02-07 03:47:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Gavin Peters 2013-02-07 06:26:37 PST
Reverted r142090 for reason:

lots of selection expectations failures

Committed r142109: <http://trac.webkit.org/changeset/142109>
Comment 29 jochen 2013-02-07 12:58:02 PST
Created attachment 187155 [details]
Patch
Comment 30 jochen 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>
Comment 31 jochen 2013-02-07 13:00:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 32 Gavin Peters 2013-02-07 13:40:43 PST
Reverted r142165 for reason:

Broke linux_aura builds.

Committed r142173: <http://trac.webkit.org/changeset/142173>
Comment 33 jochen 2013-02-07 23:35:50 PST
Created attachment 187246 [details]
Patch
Comment 34 jochen 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>
Comment 35 jochen 2013-02-07 23:40:00 PST
All reviewed patches have been landed.  Closing bug.