Bug 147519 – Get ready to build DumpRenderTree with CMake

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Alex Christensen]

There's still one symbol the linker can't find, but this is ready to upstream.

[Alex Christensen]

Created attachment 257987 [details]
Patch

[Martin Robinson]

Comment on attachment 257987 [details]
Patch

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

Looks pretty good, but I have a few suggestions...

> Source/WebKit/CMakeLists.txt:22
> +    PRIVATE JavaScriptCore
> +    PRIVATE WTF
> +    PRIVATE WebCore

Small indentation issue here.

> Tools/DumpRenderTree/CMakeLists.txt:85
> +if (WIN32)
> +    add_library(DumpRenderTreeLib SHARED ${DumpRenderTree_SOURCES})
> +    set_target_properties(DumpRenderTreeLib PROPERTIES FOLDER "Tools")
> +    set_target_properties(DumpRenderTreeLib PROPERTIES OUTPUT_NAME "DumpRenderTree")
> +    target_link_libraries(DumpRenderTreeLib ${DumpRenderTree_LIBRARIES})
> +
> +    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /ENTRY:wWinMainCRTStartup")
> +    add_executable(DumpRenderTree ${TOOLS_DIR}/win/DLLLauncher/DLLLauncherMain.cpp)
> +    target_link_libraries(DumpRenderTree shlwapi)

I wonder if you could move this to the Windows specific CMake file by using the DumpRenderTree_SOURCES variable to add the DumpRenderTreeLib library and then resetting it to only contain ${TOOLS_DIR}/win/DLLLauncher/DLLLauncherMain.cpp. I think that would allow you to remove the Windows-specific code form this file.

[Alex Christensen]

Created attachment 258308 [details]
Patch

[Brent Fulgham]

Comment on attachment 258308 [details]
Patch

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

LGTM! r=me.

> Source/WebCore/CMakeLists.txt:-3297
> -    WebCore

Is this true for other platforms (EFL/GTK?)

[Alex Christensen]

(In reply to comment #4)
> Comment on attachment 258308 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258308&action=review
> 
> LGTM! r=me.
> 
> > Source/WebCore/CMakeLists.txt:-3297
> > -    WebCore
> 
> Is this true for other platforms (EFL/GTK?)
Yes, WebCoreTestSupport is a static library on all platforms currently, and their EWS bots are ok with this change.  If someone wants to make it a static library in the future for some reason, then they can, provided they don't break anything.

[Alex Christensen]

Moved include from WebView.h to WebView.cpp
Committed to http://trac.webkit.org/changeset/187997