[webkit-reviews] review granted: [Bug 90671] [CMake][EFL] Build and run TestWebKitAPI unit tests : [Attachment 152513] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Thiago Marcos P. Santos
<tmpsantos at gmail.com>'s request for review:
Bug 90671: [CMake][EFL] Build and run TestWebKitAPI unit tests
https://bugs.webkit.org/show_bug.cgi?id=90671

Attachment 152513: Patch
https://bugs.webkit.org/attachment.cgi?id=152513&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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.