Bug 108466 – [chromium] turn TestRunner library into a component build

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

that includes gyp changes and adding TESTRUNNER_EXPORT macros

Created attachment 186697 [details]
Patch

Created attachment 186701 [details]
Patch

I verified that it builds in a chromium component build.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] 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

[Adam Barth]

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 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.

[Adam Barth]

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]

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 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 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.

[WebKit Review Bot]

Comment on attachment 186819 [details]
Patch for landing

Clearing flags on attachment: 186819

Committed r141991: <http://trac.webkit.org/changeset/141991>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 109047

Created attachment 186907 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186907 [details]
Patch

Clearing flags on attachment: 186907

Committed r142032: <http://trac.webkit.org/changeset/142032>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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.

[Gavin Peters]

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.

[Gavin Peters]

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.