Bug 79344

Summary: [CMake] Add FindDirectX
Product: WebKit Reporter: Patrick R. Gansterer <paroga>
Component: New BugsAssignee: Patrick R. Gansterer <paroga>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, dbates, rakuco
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 72816    
Attachments:
Description Flags
Patch
none
Patch aroben: review+, aroben: commit-queue-

Description Patrick R. Gansterer 2012-02-23 01:10:49 PST
[CMake] Add FindDirectX
Comment 1 Patrick R. Gansterer 2012-02-23 01:11:46 PST
Created attachment 128427 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-02-23 05:53:36 PST
Comment on attachment 128427 [details]
Patch

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

> Source/cmake/FindDirectX.cmake:12
> +    "$ENV{DXSDK_DIR}/Include"
> +    "C:/Program Files (x86)/Microsoft DirectX SDK*/Include"
> +    "C:/Program Files/Microsoft DirectX SDK*/Include"

I'd be more explicit and add the PATHS keyword before this hardcoded path list.

> Source/cmake/FindDirectX.cmake:15
> +SET(DirectX_ROOT_DIR "${DirectX_INCLUDE_DIRS}/..")

Isn't it cleaner to use GET_FILENAME_COMPONENT(DirectX_ROOT_DIR "${DirectX_INCLUDE_DIRS}/.." ABSOLUTE)?

> Source/cmake/FindDirectX.cmake:29
> +FIND_PACKAGE_HANDLE_STANDARD_ARGS(DirectX DEFAULT_MSG DirectX_LIBRARIES DirectX_INCLUDE_DIRS)

Shouldn't you also mark_as_advanced the user-visible variables?
Comment 3 Patrick R. Gansterer 2012-02-23 10:13:55 PST
Created attachment 128492 [details]
Patch
Comment 4 Raphael Kubo da Costa (:rakuco) 2012-02-23 10:26:49 PST
Comment on attachment 128492 [details]
Patch

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

Looks OK to me.

> Source/cmake/FindDirectX.cmake:21
> +IF (CMAKE_CL_64)
> +    SET(DirectX_LIBRARY_PATHS "${DirectX_ROOT_DIR}/Lib/x64")
> +ELSE ()
> +    SET(DirectX_LIBRARY_PATHS "${DirectX_ROOT_DIR}/Lib/x86" "${DirectX_ROOT_DIR}/Lib")
> +ENDIF ()

Minor: I don't really remember if we were suppose to use IF() or IF () in our agreed CMake coding style.
Comment 5 Patrick R. Gansterer 2012-02-23 10:30:29 PST
Comment on attachment 128492 [details]
Patch

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

>> Source/cmake/FindDirectX.cmake:21
>> +ENDIF ()
> 
> Minor: I don't really remember if we were suppose to use IF() or IF () in our agreed CMake coding style.

we have a space after an if in the c++ style too, so this matches WebKit style more
Comment 6 Adam Roben (:aroben) 2012-02-23 10:38:34 PST
Comment on attachment 128492 [details]
Patch

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

> Source/cmake/FindDirectX.cmake:12
> +    "C:/Program Files (x86)/Microsoft DirectX SDK*/Include"
> +    "C:/Program Files/Microsoft DirectX SDK*/Include"

Should we use $ENV{PROGRAMFILES} and $ENV{PROGRAMFILES(X86)} here? Hard-coding "C:" doesn't seem so great.
Comment 7 Patrick R. Gansterer 2012-02-23 11:16:06 PST
Committed r108647: <http://trac.webkit.org/changeset/108647>