Summary: | [CMake] Add FindQuickTimeSDK | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick R. Gansterer <paroga> | ||||||||
Component: | New Bugs | Assignee: | 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
Patrick R. Gansterer
2012-02-23 01:13:09 PST
Created attachment 128428 [details]
Patch
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. Created attachment 128493 [details]
Patch
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/? (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 Created attachment 128498 [details]
Patch
Comment on attachment 128498 [details]
Patch
Looks OK.
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? Committed r108644: <http://trac.webkit.org/changeset/108644> |