Bug 68206 – Ensure TestWebKitAPI works on mac, win, chromium-mac and chromium-linux

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Xianzhu Wang]

The patch to TestWebKitAPI may break some platforms or configurations. First, there were some changes after the patch was uploaded, and the new code after the patch may break. Second, I didn't verified the patch on all platforms and configurations. I'm working on making TestWebKitAPI work on mac, win, chromium-mac, chromium-win and chromium-linux.

[Xianzhu Wang]

Specifically, should fix the break on chromium's shared windows build (http://www.chromium.org/developers/how-tos/component-build).

[Dirk Pranke]

Okay, I spent a half hour or so staring at this on the win shared build and I think you've got a couple of things to fix before this'll work.

this TestWebKitAPI target is new to me ... it looks like it is intended to be the unit test driver for c++ tests in webkit/webcore/etc.?

At any rate, it appears to be linking in both webkit and wtf, and it looks like the existing files for TestWebKitAPI are unit tests for WTF ...

Here's where the problems come up ... Chromium is only building wtf as a static library; it gets included into webkit.dll, but the WTF symbols aren't (and shouldn't be) exported from that. 

the WTF library has internal dependencies on a function called currentTime; Chromium overrides the default WTF implementation with one found in ChromiumCurrentTime. Which, unfortunately, is not built as part of wtf.lib, it's built as part of webkit.dll. 

So, wtf can't link without webkit.dll. However, there's no good way to export the CurrentTime symbol out of webkit.dll on windows in a way where the __declspec(dllexport) makes sense, without creating a compile time dependency from wtf onto webkit.

You can hack around this by including ChromiumCurrentTime.cpp and ChromiumThreading.cpp (apparently also needed) directly into the TestWebKitAPI sources list, but of course you can't call the currentTime function() without initializing webkit_support and including that as a dependency.

[Xianzhu Wang]

(In reply to comment #2)

Thanks Dirk for detailed and helpful explanation.

I just tried component=shared_library in a WebKit source tree with chromium dependencies on Linux, and encountered linking errors:

/usr/bin/ld: out/Release/obj.target/DumpRenderTree/Tools/DumpRenderTree/chromium/TestShell.o: in function TestShell::bindJSObjectsToWindow(WebKit::WebFrame*):TestShell.cpp(.text._ZN9TestShell21bindJSObjectsToWindowEPN6WebKit8WebFrameE+0x17): error: undefined reference to 'WebKit::WebTestingSupport::injectInternalsObject(WebKit::WebFrame*)'
/usr/bin/ld: out/Release/obj.target/DumpRenderTree/Tools/DumpRenderTree/chromium/TestShell.o: in function TestShell::resetTestController():TestShell.cpp(.text._ZN9TestShell19resetTestControllerEv+0x132): error: undefined reference to 'WebKit::WebTestingSupport::resetInternalsObject(WebKit::WebFrame*)'

I tried to fix it by moving the 'component=="shared_library"' condition out of 'inside_chromium_build==1' condition (WebKit.gyp about line 619), and I can successfully build all targets.

Is it intended that component=shared_library doesn't work for 'build-wekit --chromium'?

BTW do you know what Source/WebCore/gyp/WebCore.gyp (not .../WebCore.gyp/WebCore.gyp) and Source/JavaScriptCore/gyp/JavaScriptCore.gyp are for?

> Okay, I spent a half hour or so staring at this on the win shared build and I think you've got a couple of things to fix before this'll work.
> 
> this TestWebKitAPI target is new to me ... it looks like it is intended to be the unit test driver for c++ tests in webkit/webcore/etc.?
> 

I think so. However, for now it contains only tests for WTF and WebKit2. I touched this because I'm making some changes to WTF::StringBuilder and want unit tests for it.

> At any rate, it appears to be linking in both webkit and wtf, and it looks like the existing files for TestWebKitAPI are unit tests for WTF ...
> 
> Here's where the problems come up ... Chromium is only building wtf as a static library; it gets included into webkit.dll, but the WTF symbols aren't (and shouldn't be) exported from that. 
> 
> the WTF library has internal dependencies on a function called currentTime; Chromium overrides the default WTF implementation with one found in ChromiumCurrentTime. Which, unfortunately, is not built as part of wtf.lib, it's built as part of webkit.dll. 
> 
> So, wtf can't link without webkit.dll. However, there's no good way to export the CurrentTime symbol out of webkit.dll on windows in a way where the __declspec(dllexport) makes sense, without creating a compile time dependency from wtf onto webkit.
> 
> You can hack around this by including ChromiumCurrentTime.cpp and ChromiumThreading.cpp (apparently also needed) directly into the TestWebKitAPI sources list, but of course you can't call the currentTime function() without initializing webkit_support and including that as a dependency.

[Xianzhu Wang]

(In reply to comment #3)
> 
> Is it intended that component=shared_library doesn't work for 'build-wekit --chromium'?
>
Never mind. I've found the reason and will fix it.
 
> BTW do you know what Source/WebCore/gyp/WebCore.gyp (not .../WebCore.gyp/WebCore.gyp) and Source/JavaScriptCore/gyp/JavaScriptCore.gyp are for?
>

[Xianzhu Wang]

Created attachment 107629 [details]
patch

[Tony Chang]

Comment on attachment 107629 [details]
patch

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

> Is it intended that component=shared_library doesn't work for 'build-wekit --chromium'?

We don't have a bot for this, so I'm not surprised it doesn't work.

I don't really understand exactly how TestWebKitAPI works on win/mac and I don't have access to a win/mac right now.  I'll look again tomorrow.

> Source/WebKit/chromium/WebKit.gyp:1265
> +                'public',

Is this include only needed for Chromium{CurrentTime,Threading}.cpp?  If so, maybe it should be in the condition below?

> Source/WebKit/chromium/WebKit.gyp:1270
> +                ''

Nit: Remove this blank line

> Source/WebKit/chromium/WebKit.gyp:1276
> +                        'src/ChromiumCurrentTime.cpp',
> +                        'src/ChromiumThreading.cpp',

In the shared_library build on windows, do we call currentTime() or ChromiumThreading::callOnMainThread()?  Should we include separate stub implementations when linking TestWebKitAPI rather than using the definitions in webkit?  For example, we could add stubs in WebKit/Tools/TestWebKitAPI/chromium/.

> Tools/TestWebKitAPI/Tests/WTF/MetaAllocator.cpp:32
> -#include <JavaScriptCore/MetaAllocator.h>
> -#include <JavaScriptCore/Vector.h>
> +#include <stdarg.h>
> +#include <wtf/MetaAllocator.h>
> +#include <wtf/Vector.h>

I don't understand this include/ForwardingHeader change.  Can you describe why this is needed in the ChangeLog?  Why does it work in the Apple Mac build?

[Xianzhu Wang]

Comment on attachment 107629 [details]
patch

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

>> Source/WebKit/chromium/WebKit.gyp:1265
>> +                'public',
> 
> Is this include only needed for Chromium{CurrentTime,Threading}.cpp?  If so, maybe it should be in the condition below?

tests/RunAllTests.cpp also needs it.

>> Source/WebKit/chromium/WebKit.gyp:1270
>> +                ''
> 
> Nit: Remove this blank line

Will change it.

>> Source/WebKit/chromium/WebKit.gyp:1276
>> +                        'src/ChromiumThreading.cpp',
> 
> In the shared_library build on windows, do we call currentTime() or ChromiumThreading::callOnMainThread()?  Should we include separate stub implementations when linking TestWebKitAPI rather than using the definitions in webkit?  For example, we could add stubs in WebKit/Tools/TestWebKitAPI/chromium/.

I reused the tests/RunAllTests.cpp which uses webkit_support. As webkit_support is the testing support of Chromium, I think it's reasonable for TestWebKitAPI on Chromium to reuse the same facility, and also reuse other stuffs of Chromium WebKit as much as possible. What's your opinion?

>> Tools/TestWebKitAPI/Tests/WTF/MetaAllocator.cpp:32
>> +#include <wtf/Vector.h>
> 
> I don't understand this include/ForwardingHeader change.  Can you describe why this is needed in the ChangeLog?  Why does it work in the Apple Mac build?

Originally in Apple Mac the code uses #include <JavaScriptCore/...> to include WTF headers in a magic way (I don't know the details of how it works). However, the path <JavaScriptCore/Vector.h> doesn't exist on some other platforms, so we should change it to <wtf/Vector.h>, but because Mac still expects <JavaScriptCore/Vector.h>, to resolve this problem, each header that is included in <wtf/...> way should have a forwarding header file to include <JavaScriptCore/...>.

[Tony Chang]

Comment on attachment 107629 [details]
patch

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

>>> Source/WebKit/chromium/WebKit.gyp:1276

>> 
>> In the shared_library build on windows, do we call currentTime() or ChromiumThreading::callOnMainThread()?  Should we include separate stub implementations when linking TestWebKitAPI rather than using the definitions in webkit?  For example, we could add stubs in WebKit/Tools/TestWebKitAPI/chromium/.
> 
> I reused the tests/RunAllTests.cpp which uses webkit_support. As webkit_support is the testing support of Chromium, I think it's reasonable for TestWebKitAPI on Chromium to reuse the same facility, and also reuse other stuffs of Chromium WebKit as much as possible. What's your opinion?

On further inspection, this seems fine.  I was a confused by how this works in the shared_library case, but have convinced myself that this is OK.  Please add a comment here explaining why this is needed.

[Xianzhu Wang]

Created attachment 108100 [details]
patch v2

[WebKit Review Bot]

Comment on attachment 108100 [details]
patch v2

Clearing flags on attachment: 108100

Committed r95647: <http://trac.webkit.org/changeset/95647>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.