Bug 99904 – [chromium] introduce a public API for the TestRunner library

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[chromium] introduce a public API for the TestRunner library

Created attachment 169751 [details]
Patch

[Adam Barth]

Comment on attachment 169751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169751&action=review

> Tools/DumpRenderTree/chromium/TestRunner/public/WebAccessibilityController.h:40
> +class WebAccessibilityController {

How is this going to work in the component build?  I assume we're not going to link this into webkit.dll.  Does that means we're going to link it into content.dll?  Do we need a new TestRunner.dll ?

(In reply to comment #2)
> (From update of attachment 169751 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169751&action=review
> 
> > Tools/DumpRenderTree/chromium/TestRunner/public/WebAccessibilityController.h:40
> > +class WebAccessibilityController {
> 
> How is this going to work in the component build?  I assume we're not going to link this into webkit.dll.  Does that means we're going to link it into content.dll?  Do we need a new TestRunner.dll ?

Since it only depends on symbols exported from webkit.dll, I guess it can be directly linked into the content_shell

[Adam Barth]

> Since it only depends on symbols exported from webkit.dll, I guess it can be directly linked into the content_shell

I wonder if it would be better as a separate DLL.  That will help make sure the dependencies are clean.

[Adam Barth]

Comment on attachment 169751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169751&action=review

> Tools/DumpRenderTree/chromium/TestRunner/public/WebAccessibilityController.h:36
> +class AccessibilityController;
> +
> +namespace WebKit {

You might want to take this opportunity to introduce a separate namespace for this library.  For example, we could use the namespace TestRunner or WebTestRunner.  Using the WebKit namespace isn't quite right because this is outside webkit.dll.

Do we want to put the implementation and the API into different namespaces?  My gut says that's overkill.

(In reply to comment #4)
> > Since it only depends on symbols exported from webkit.dll, I guess it can be directly linked into the content_shell
> 
> I wonder if it would be better as a separate DLL.  That will help make sure the dependencies are clean.

I think long term that's definitely desirable.

I decided against going there immediately because of Task.h and TestRunner.h that are still directly consumed by DRT.

[Adam Barth]

Makes sense.  However, I think we should set up the API in a way that makes that transition easy.  For the time being, I think that mostly means using a new namespace and deciding which functions can only be called from "inside" the library (e.g., the WebAccessibilityController(AccessibilityController*) constructor).

Created attachment 169789 [details]
Patch

I've moved the public API into the namespace WebTestRunner (but of the implementation only the required pieces yet), added a WEBTESTRUNNER_IMPLEMENTATION define, and removed a WTF header ref (wtf/Vector)

I also verified that this builds with a component build in chromium

[Adam Barth]

Comment on attachment 169789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169789&action=review

> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestInterfaces.cpp:128
> +    m_internal = new Internal;

We should use WebPrivateOwnPtr here to help us avoid leaking.  We still need to call reset manually, but at least there will be an ASSERT if we screw up.

Comment on attachment 169789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=169789&action=review

>> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestInterfaces.cpp:128
>> +    m_internal = new Internal;
> 
> We should use WebPrivateOwnPtr here to help us avoid leaking.  We still need to call reset manually, but at least there will be an ASSERT if we screw up.

WebPrivateOwnPtr requires WEBKIT_IMPLEMENTATION to be defined in the .cpp file which we don't want.

[Adam Barth]

Good point.  We could generalize it, but it's probably not worth it.

[WebKit Review Bot]

Comment on attachment 169789 [details]
Patch

Clearing flags on attachment: 169789

Committed r132029: <http://trac.webkit.org/changeset/132029>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.