Bug 79345 - [CMake] Add FindQuickTimeSDK
Summary: [CMake] Add FindQuickTimeSDK
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks: 72816
  Show dependency tree / graph
 
Reported: 2012-02-23 01:13 PST by Patrick R. Gansterer
Modified: 2012-02-23 11:07 PST (History)
3 users (show)

See Also:

Attachments
Patch (1.91 KB, patch)
2012-02-23 01:15 PST, Patrick R. Gansterer 		no flags Details | Formatted Diff | Diff
Patch (2.02 KB, patch)
2012-02-23 10:18 PST, Patrick R. Gansterer 		no flags Details | Formatted Diff | Diff
Patch (2.02 KB, patch)
2012-02-23 10:36 PST, Patrick R. Gansterer 		aroben: review+
aroben: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2012-02-23 01:13:09 PST 
[CMake] Add FindQuickTimeSDK
Comment 1 Patrick R. Gansterer 2012-02-23 01:15:48 PST 
Created attachment 128428 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 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.
Comment 3 Patrick R. Gansterer 2012-02-23 10:18:23 PST 
Created attachment 128493 [details]
Patch
Comment 4 Raphael Kubo da Costa (:rakuco) 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/?
Comment 5 Patrick R. Gansterer 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
Comment 6 Patrick R. Gansterer 2012-02-23 10:36:31 PST 
Created attachment 128498 [details]
Patch
Comment 7 Raphael Kubo da Costa (:rakuco) 2012-02-23 10:39:34 PST 
Comment on attachment 128498 [details]
Patch

Looks OK.
Comment 8 Adam Roben (:aroben) 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?
Comment 9 Patrick R. Gansterer 2012-02-23 11:07:03 PST 
Committed r108644: <http://trac.webkit.org/changeset/108644>