[chromium] Manage TestSuite lifetime explicitly, fixing crash in webkit_unit_tests in DLL build."
Created attachment 112103 [details] Patch
Attachment 112103 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/tests/WebUnitTests.h:46: WEBKIT_EXPORT should only appear in the chromium public directory. [readability/webkit_export] [5] Source/WebKit/chromium/tests/WebUnitTests.h:49: WEBKIT_EXPORT should only appear in the chromium public directory. [readability/webkit_export] [5] Source/WebKit/chromium/tests/WebUnitTests.h:52: WEBKIT_EXPORT should only appear in the chromium public directory. [readability/webkit_export] [5] Total errors found: 3 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
On component/dll builds, the existing code çreates two test suites -- main() creates a TestSuite, then WebKit::RunAllTests creates another. This leads to two nasty side effects: - webkit_unit_tests crashes after it runs. The tests show as passing, but then we get a stack dump in ~CommandLine. - The "real" testsuite thinks it doesn't have a command line. This makes things like --gtest_filter don't work This patch replaces the RunAllTests with three functions: InitTestSuite,RunAllTests,DeleteTestSuite. This allows the overall main() function to create the suite, initialize webkit support, run the tests, then tear everything down again in the right order. I'm not sure how to make the style bot happy. Should tests/WebUnitTests.h be in public?
Comment on attachment 112103 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112103&action=review > Source/WebKit/chromium/tests/RunAllTests.cpp:47 > +// and run inside awebkit.dll. Nit: awebkit typo > Source/WebKit/chromium/tests/WebUnitTests.cpp:38 > +namespace { > +static TestSuite* testSuite = 0; Nit: No namespace (it's redundant with static and WebKit style prefers static). >> Source/WebKit/chromium/tests/WebUnitTests.h:46 >> +WEBKIT_EXPORT void InitTestSuite(int argc, char** argv); > > WEBKIT_EXPORT should only appear in the chromium public directory. [readability/webkit_export] [5] We could add a WEBKIT_EXPORT_PRIVATE for these. See src/net/base/net_export.h for an example of this.
Created attachment 112208 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
LGTM, but let's let Darin review the addition of WEBKIT_EXPORT_PRIVATE.
Comment on attachment 112208 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112208&action=review > Source/WebKit/chromium/public/WebCommon.h:72 > + #define WEBKIT_EXPORT_PRIVATE __declspec(dllexport) I don't think WEBKIT_EXPORT_PRIVATE is all that helpful. I'm also don't think it is that helpful for net/ to have NET_EXPORT_PRIVATE :) The only reason we have NET_EXPORT_PRIVATE is because Ricardo took the time to carefully separate net/ entry points used only by tests from those used by Chrome. The downside of this approach is that it is really hard to maintain. I would not be surprised to see NET_EXPORT_PRIVATE being used by Chrome now since there is no mechanism to prevent it. Let's just stick with WEBKIT_EXPORT. It is easier to understand. I'm not too worried about using it outside of the public/ directory for this one case.
Created attachment 112220 [details] Patch
Comment on attachment 112220 [details] Patch Feel free to ignore the style bot. We can fix that the style checker in a follow up bug to ignore the tests directory.
Attachment 112220 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1 Source/WebKit/chromium/tests/WebUnitTests.h:46: WEBKIT_EXPORT should only appear in the chromium public directory. [readability/webkit_export] [5] Source/WebKit/chromium/tests/WebUnitTests.h:49: WEBKIT_EXPORT should only appear in the chromium public directory. [readability/webkit_export] [5] Source/WebKit/chromium/tests/WebUnitTests.h:52: WEBKIT_EXPORT should only appear in the chromium public directory. [readability/webkit_export] [5] Total errors found: 3 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r98269: <http://trac.webkit.org/changeset/98269>