[webkit-reviews] review granted: [Bug 168939] Make ImageDiff stand-alone : [Attachment 309050] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 168939: Make ImageDiff stand-alone
https://bugs.webkit.org/show_bug.cgi?id=168939

Attachment 309050: Patch

https://bugs.webkit.org/attachment.cgi?id=309050&action=review




--- Comment #24 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 309050
  --> https://bugs.webkit.org/attachment.cgi?id=309050
Patch

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

r=me if you fix the cmake issues and Xcode project file issue.	Get Alex
Christensen to look at the cmake stuff if you want a second opinion.

> Tools/ImageDiff/PlatformGTK.cmake:2
> +set(PLATFORM_WIN_CAIRO 1)
> +include(PlatformWin.cmake)

This seems a bit dodgy.  GTK port != WinCairo port.  Need something more
specific like PLATFORM_CAIRO that's set for both GTK and WIN_CAIRO to do this.

I really think you should keep Cairo.cmake, and then just include(Cairo.cmake)
both here and in PlatformWin.cmake.

> Tools/ImageDiff/PlatformWin.cmake:26
> +    list(APPEND IMAGE_DIFF_LIBRARIES
> +	   CFNetwork
> +	   CoreGraphics
> +	   CoreText
> +    )
> +    set(IMAGE_DIFF_SOURCES
> +	   ${IMAGE_DIFF_DIR}/cg/ImageDiff.cpp
> +    )
> +    list(APPEND ImageDiff_LIBRARIES
> +	   CoreFoundation
> +	   CoreGraphics
> +	   CoreText
> +    )

This is identical to PlatformMac.cmake.  Can we just include(PlatformMac.cmake)
here?

> Tools/ImageDiff/ImageDiff.xcodeproj/project.pbxproj:152
> +		318E7A281EB939AD00DAE9FC /* Debug */ = {
> +			isa = XCBuildConfiguration;
> +			baseConfigurationReference = 317C0C271EB9543900439F71
/* ImageDiff.xcconfig */;

This baseConfigurationReference should be DebugRelease.xcconfig.

> Tools/ImageDiff/ImageDiff.xcodeproj/project.pbxproj:160
> +		318E7A291EB939AD00DAE9FC /* Release */ = {
> +			isa = XCBuildConfiguration;
> +			baseConfigurationReference = 317C0C271EB9543900439F71
/* ImageDiff.xcconfig */;

This baseConfigurationReference should be DebugRelease.xcconfig.