RESOLVED FIXED 79345
[CMake] Add FindQuickTimeSDK 
https://bugs.webkit.org/show_bug.cgi?id=79345
Summary [CMake] Add FindQuickTimeSDK
Patrick R. Gansterer
Reported 2012-02-23 01:13:09 PST
[CMake] Add FindQuickTimeSDK
Attachments
Patch (1.91 KB, patch)
2012-02-23 01:15 PST, Patrick R. Gansterer 		no flags
DetailsFormatted DiffDiff
Patch (2.02 KB, patch)
2012-02-23 10:18 PST, Patrick R. Gansterer 		no flags
DetailsFormatted DiffDiff
Patch (2.02 KB, patch)
2012-02-23 10:36 PST, Patrick R. Gansterer 		aroben: review+
aroben: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Patrick R. Gansterer
Comment 1 2012-02-23 01:15:48 PST
Created attachment 128428 [details] Patch
Raphael Kubo da Costa (:rakuco)
Comment 2 2012-02-23 05:54:53 PST
Comment on attachment 128428 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128428&action=review > Source/cmake/FindQuickTimeSDK.cmake:10 > + "C:/Program Files (x86)/QuickTime SDK/CIncludes" > + "C:/Program Files/QuickTime SDK/CIncludes" I'd more explicit and prefix this list of hardcoded paths with PATHS. > Source/cmake/FindQuickTimeSDK.cmake:13 > +SET(QuickTimeSDK_LIBRARY_PATH "${QuickTimeSDK_INCLUDE_DIRS}/../Libraries") Isn't GET_FILENAME_COMPONENT(.. .. ABSOLUTE) cleaner? > Source/cmake/FindQuickTimeSDK.cmake:20 > +FIND_PACKAGE_HANDLE_STANDARD_ARGS(QuickTimeSDK DEFAULT_MSG QuickTimeSDK_LIBRARIES QuickTimeSDK_INCLUDE_DIRS) I'd mark_as_advanced the user-visible variables.
Patrick R. Gansterer
Comment 3 2012-02-23 10:18:23 PST
Created attachment 128493 [details] Patch
Raphael Kubo da Costa (:rakuco)
Comment 4 2012-02-23 10:28:40 PST
Comment on attachment 128493 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128493&action=review > Source/cmake/FindQuickTimeSDK.cmake:13 > +SET(QuickTimeSDK_LIBRARY_PATH "${QuickTimeSDK_INCLUDE_DIRS}/../Libraries") GET_FILENAME_COMPONENT? > Source/cmake/FindQuickTimeSDK.cmake:18 > +# handle the QUIETLY and REQUIRED arguments and set DirectX_FOUND to TRUE if all listed variables are TRUE s/DirectX/QuickTimeSDK/?
Patrick R. Gansterer
Comment 5 2012-02-23 10:34:52 PST
(In reply to comment #4) > (From update of attachment 128493 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128493&action=review > > > Source/cmake/FindQuickTimeSDK.cmake:13 > > +SET(QuickTimeSDK_LIBRARY_PATH "${QuickTimeSDK_INCLUDE_DIRS}/../Libraries") > > GET_FILENAME_COMPONENT? IMHO overkill here, since the LIBRARY_PATH is no "public" variable and is resolved in FIND_LIBRARY anyway... > > Source/cmake/FindQuickTimeSDK.cmake:18 > > +# handle the QUIETLY and REQUIRED arguments and set DirectX_FOUND to TRUE if all listed variables are TRUE > > s/DirectX/QuickTimeSDK/? thx
Patrick R. Gansterer
Comment 6 2012-02-23 10:36:31 PST
Created attachment 128498 [details] Patch
Raphael Kubo da Costa (:rakuco)
Comment 7 2012-02-23 10:39:34 PST
Comment on attachment 128498 [details] Patch Looks OK.
Adam Roben (:aroben)
Comment 8 2012-02-23 10:40:08 PST
Comment on attachment 128498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128498&action=review > Source/cmake/FindQuickTimeSDK.cmake:10 > + "C:/Program Files (x86)/QuickTime SDK/CIncludes" > + "C:/Program Files/QuickTime SDK/CIncludes" Should we use $ENV{PROGRAMFILES} and $ENV{PROGRAMFILES(X86)} here?
Patrick R. Gansterer
Comment 9 2012-02-23 11:07:03 PST
Committed r108644: <http://trac.webkit.org/changeset/108644>
Note You need to log in before you can comment on or make changes to this bug.