Bug 90671

Created attachment 151084 [details] Patch The current WIP is enough for running WTF tests.

Created attachment 151960 [details] Patch

Comment on attachment 151960 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151960&action=review > Tools/TestWebKitAPI/CMakeLists.txt:55 > + -DTEST_WEBKIT2_RESOURCES_DIR=\"${TESTWEBKITAPI_DIR}/Tests/WebKit2\" Nit : Place this line as below. http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/efl/CMakeLists.txt#L116 > Tools/TestWebKitAPI/CMakeLists.txt:56 > + -DTEST_INJECTED_BUNDLE_PATH=\"${TestWebKitAPIInjectedBundle_PATH}\" ditto.

Created attachment 152513 [details] Patch

Comment on attachment 152513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152513&action=review Looks sane to me. I had some minor nits. > Tools/TestWebKitAPI/CMakeLists.txt:32 > + ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DocumentStartUserScriptAlertCrash_Bundle.cpp > + ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp > + ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp These lines aren't in sorted order according to the Unix sort command. In particular, ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DocumentStartUserScriptAlertCrash_Bundle.cpp should be after ${TESTWEBKITAPI_DIR}/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp. > Tools/TestWebKitAPI/CMakeLists.txt:77 > + ${TESTWEBKITAPI_DIR}/Tests/WTF/VectorBasic.cpp > + ${TESTWEBKITAPI_DIR}/Tests/WTF/Vector.cpp These lines should be swapped such that ${TESTWEBKITAPI_DIR}/Tests/WTF/Vector.cpp comes before ${TESTWEBKITAPI_DIR}/Tests/WTF/VectorBasic.cpp so that this list is in sorted order according to the Unix sort command. > Tools/TestWebKitAPI/PlatformEfl.cmake:33 > +# The list bellow works like a test expectations. Tests at the bellow => below expectations => expectation at => in > Tools/TestWebKitAPI/PlatformEfl.cmake:35 > +# Tests at test_webkit2_api_fail_BINARIES are compiled and suffixed "at test_webkit2_api_fail_BINARIES" => "in the test_webkit2_api_fail_BINARIES list" I'm unclear what the lists test_webkit2_api_BINARIES and test_webkit2_api_fail_BINARIES represent. I take it that they represents the binaries corresponding to the tests that pass and fail, respectively? I suggest that we elaborate on the purpose of these lists. > Tools/TestWebKitAPI/PlatformEfl.cmake:40 > +# Release builds before adding it to the list. Which list? I take it that we should add passing tests to the list test_webkit2_api_BINARIES? > Tools/TestWebKitAPI/PlatformEfl.cmake:45 > + DocumentStartUserScriptAlertCrash > + DOMWindowExtensionNoCache These lines should be swapped such that DOMWindowExtensionNoCache comes before DocumentStartUserScriptAlertCrash so that this list is in sorted order according to the Unix sort command. > Tools/TestWebKitAPI/PlatformEfl.cmake:78 > + CanHandleRequest > + NewFirstVisuallyNonEmptyLayoutFrames > + RestoreSessionStateContainingFormData > + DownloadDecideDestinationCrash > + NewFirstVisuallyNonEmptyLayoutForImages > + WKPageGetScaleFactorNotZero > + DOMWindowExtensionBasic > + ShouldGoToBackForwardListItem Please sort these according to the Unix sort command. > Tools/TestWebKitAPI/efl/main.cpp:35 > +static bool parseArguments(int argc, char** argv) Currently this function only parses and returns the value for the useX11Window command line option. I take it that you envision accepting additional command line options in the future given the generalness of the name of this function. Unless you plan to accept additional command line options in the short term, you may want to consider using a more specific name for this function, say checkForUseX11WindowArgument. > Tools/TestWebKitAPI/efl/main.cpp:37 > + int ret = 0; Can we come up with a more descriptive name for this? Maybe shouldUseX11Window? hasUseX11WindowOption? value? > Tools/TestWebKitAPI/efl/main.cpp:59 > + int ret = TestWebKitAPI::TestsController::shared().run(argc, argv) ? EXIT_SUCCESS : EXIT_FAILURE; I suggest that we write out "return code" as returnCode out, for clarity, instead of abbreviating it as ret.

Created attachment 153005 [details] Patch Thanks for reviewing. I was piping from vim to the command line sort on Linux. I'm now using the built-in vim sort which seens to be aligned with Unix sort. Now I see why the style checker always complains about the sorting order of my include headers.

Comment on attachment 153005 [details] Patch Clearing flags on attachment: 153005 Committed r122973: <http://trac.webkit.org/changeset/122973>

All reviewed patches have been landed. Closing bug.