[webkit-reviews] review denied: [Bug 84446] [GTK] Build and run TestWebKitAPI WebKit2 unit tests : [Attachment 138083] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 24 08:09:26 PDT 2012
Philippe Normand <pnormand at igalia.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 84446: [GTK] Build and run TestWebKitAPI WebKit2 unit tests
https://bugs.webkit.org/show_bug.cgi?id=84446
Attachment 138083: Patch
https://bugs.webkit.org/attachment.cgi?id=138083&action=review
------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=138083&action=review
Looks good in general but I'm afraid I missed some things, maybe Martin can do
another pass? This patch is quite big :). Anyway, some questions follow:
> Tools/ChangeLog:19
> + * TestWebKitAPI/JavaScriptTest.cpp: Use
> + JavaScriptCore/JSContextRef.h instead of
> + JavaScriptCore/JavaScriptCore.h since it includes JSStringRefCF.h
> + unconditionally.
Would it make sense to fix JavaScriptCore.h instead?
> Tools/Scripts/run-gtk-tests:83
> +
SkippedTest("TestWebKitAPI/WebKit2/TestNewFirstVisuallyNonEmptyLayout",
> + "Test times out"),
> +
SkippedTest("TestWebKitAPI/WebKit2/TestNewFirstVisuallyNonEmptyLayoutForImages"
,
> + "Test times out"),
> + SkippedTest("TestWebKitAPI/WebKit2/TestWKConnectionTest",
> + "Test times out"),
> +
SkippedTest("TestWebKitAPI/WebKit2/TestRestoreSessionStateContainingFormData",
> + "Session State is not implemented in GTK+ port"),
> + SkippedTest("TestWebKitAPI/WebKit2/TestSpacebarScrolling",
> + "Test fails")
Can these failures be tracked in bugzilla if possible?
> Tools/Scripts/run-gtk-tests:201
> + self._test_env["TEST_WEBKIT_API_WEBKIT2_INJECTED_BUNDLE_PATH"] =
os.path.abspath(os.path.join(self._get_build_directory(), "Libraries"))
Damn I forgot the "get build directory" is duplicated between this script and
common.py. It'd be great to fix that at some point, in another patch.
> Tools/Scripts/run-gtk-tests:265
> + tester_command = [test, "--gtest_throw_on_failure"]
Maybe that change should go in the other patch?
> Tools/TestWebKitAPI/GNUmakefile.am:61
> +testwebkitapi_wtf_tests_cppflags = \
Ditto
> Tools/TestWebKitAPI/GNUmakefile.am:69
> +testwebkitapi_wtf_tests_ldadd = \
Ditto
> Tools/TestWebKitAPI/GNUmakefile.am:74
> +testwebkitapi_wtf_tests_ldflags = \
Ditto
> Tools/TestWebKitAPI/GNUmakefile.am:172
> -Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_CPPFLAGS =
$(testwebkitapi_tests_cppflags)
> -Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDADD =
$(testwebkitapi_tests_ldadd)
> -Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDFLAGS =
$(testwebkitapi_tests_ldflags)
> +Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_CPPFLAGS =
$(testwebkitapi_wtf_tests_cppflags)
> +Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDADD =
$(testwebkitapi_wtf_tests_ldadd)
> +Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDFLAGS =
$(testwebkitapi_wtf_tests_ldflags)
Ok I'm tired of Ditto, you get it I guess :)
> Tools/TestWebKitAPI/gtk/PlatformUtilitiesGtk.cpp:65
> + return g_filename_to_utf8(value, -1, 0, &bytesWritten, 0);
I find it a bit strange to use g_filename_to_utf8() here. Maybe some day we'll
have env variables not dealing with file paths. Or maybe it's just this
function that should be renamed :)
> Tools/TestWebKitAPI/gtk/main.cpp:37
> + bool passed = TestWebKitAPI::TestsController::shared().run(argc, argv);
> +
> + return passed ? EXIT_SUCCESS : EXIT_FAILURE;
This can be one line maybe?
More information about the webkit-reviews
mailing list