Bug 46907 - [Chromium] Enable webkit unit tests in chromium multi-dll build
Summary: [Chromium] Enable webkit unit tests in chromium multi-dll build
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows Vista
: P2 Normal
Assignee: Victor Wang
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-30 08:56 PDT by Victor Wang
Modified: 2010-10-06 19:22 PDT (History)
4 users (show)

See Also:


Attachments
Proposed Patch (11.60 KB, patch)
2010-09-30 09:31 PDT, Victor Wang
no flags Details | Formatted Diff | Diff
Proposed Patch (11.65 KB, patch)
2010-10-06 11:49 PDT, Victor Wang
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Wang 2010-09-30 08:56:54 PDT
In chromium Windows multi-dll build, webkit is built as a shared library (dll). To run the webkit unit tests in this mode, we need to compile the unit tests code in the webkit.dll and export an webkit api that launches all the tests.
Comment 1 Victor Wang 2010-09-30 09:31:12 PDT
Created attachment 69341 [details]
Proposed Patch
Comment 2 Darin Fisher (:fishd, Google) 2010-10-04 10:41:23 PDT
Comment on attachment 69341 [details]
Proposed Patch

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

> WebKit/chromium/tests/RunAllTests.cpp:44
> +#if defined(WIN32) && defined(WEBKIT_DLL_UNITTEST)

is it possible to unify these two branches so that we can avoid the
#ifdef here?
Comment 3 Victor Wang 2010-10-04 14:46:24 PDT
(In reply to comment #2)
> (From update of attachment 69341 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69341&action=review
> 
> > WebKit/chromium/tests/RunAllTests.cpp:44
> > +#if defined(WIN32) && defined(WEBKIT_DLL_UNITTEST)
> 
> is it possible to unify these two branches so that we can avoid the
> #ifdef here?

Not sure a good way to do this. The atExitManager (TestSuite) must to be created before WebKit support SetUpTestEnvironment, but it can not be created more than once in the binary.
Comment 4 Darin Fisher (:fishd, Google) 2010-10-05 14:00:58 PDT
Also, it would be nice to avoid directly using base::AtExitManager from the WebKit code base.  We need to hide such dependencies.  Otherwise, it will be very difficult to ever rename AtExitManager or change the way it works (if we wanted to do so).  Circular dependencies between Chromium and WebKit repositories add pain.

I have some ideas about how to resolve this.  Perhaps we could introduce a method to webkit_support that makes a callback to run the tests.  Then, the details of how the AtExitManager gets initialized could live in the Chromium repository.
Comment 5 Victor Wang 2010-10-06 11:49:00 PDT
Created attachment 69972 [details]
Proposed Patch

Do not see a simple way to avoid #ifdef. New patch uploaded per our discussion, it hides the AtExitManager by creating a TestSuite instance.
Comment 6 Darin Fisher (:fishd, Google) 2010-10-06 11:56:35 PDT
Comment on attachment 69972 [details]
Proposed Patch

R=me
Comment 7 Victor Wang 2010-10-06 15:35:21 PDT
Committed r69243: <http://trac.webkit.org/changeset/69243>
Comment 8 WebKit Review Bot 2010-10-06 16:55:11 PDT
http://trac.webkit.org/changeset/69243 might have broken Chromium Win Release
Comment 9 Victor Wang 2010-10-06 19:22:32 PDT
(In reply to comment #8)
> http://trac.webkit.org/changeset/69243 might have broken Chromium Win Release

Think this is false alarm. The build was broken by another patch (http://trac.webkit.org/changeset/69247) and was fixed later (http://trac.webkit.org/changeset/69251).