[webkit-reviews] review denied: [Bug 168939] Make ImageDiff stand-alone : [Attachment 308933] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed May 3 14:53:52 PDT 2017
David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has denied 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 308933: Patch
https://bugs.webkit.org/attachment.cgi?id=308933&action=review
--- Comment #15 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 308933
--> https://bugs.webkit.org/attachment.cgi?id=308933
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=308933&action=review
r- to fix xcconfig files.
> Tools/ChangeLog:10
> + Create ImageDiff without dependencies on bmalloc and WTF so that it
exists as a
> + stand-alone project. Note that this change does not eliminate the
ImageDiff inside
> + the DumpRenderTree project.
I don't see any changes to cmake files for the moved ImageDiff sources.
Maybe we're only moving iOS and macOS first?
> Tools/ChangeLog:18
> + * ImageDiff/cg/Configurations: Added.
> + * ImageDiff/cg/Configurations/BaseTarget.xcconfig: Copied from
Tools/DumpRenderTree/mac/Configurations/BaseTarget.xcconfig.
> + * ImageDiff/cg/Configurations/ImageDiff.xcconfig: Copied from
Tools/DumpRenderTree/mac/Configurations/ImageDiff.xcconfig.
You're missing Tools/DumpRenderTree/mac/Configurations/DebugRelease.xcconfig.
DebugRelease.xcconfig is the basis for the Debug/Release configurations in the
Xcode project, which are set to ImageDiff.xcconfig now.
> Tools/ImageDiff/ImageDiff.xcodeproj/project.pbxproj:179
> + buildSettings = {
> + ALWAYS_SEARCH_USER_PATHS = NO;
> + CLANG_ANALYZER_NONNULL = YES;
> + CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION =
YES_AGGRESSIVE;
> + CLANG_CXX_LANGUAGE_STANDARD = "gnu++14";
> + CLANG_CXX_LIBRARY = "libc++";
> + CLANG_ENABLE_MODULES = YES;
> + CLANG_ENABLE_OBJC_ARC = YES;
> + CLANG_WARN_BOOL_CONVERSION = YES;
> + CLANG_WARN_CONSTANT_CONVERSION = YES;
> + CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> + CLANG_WARN_DOCUMENTATION_COMMENTS = YES;
> + CLANG_WARN_EMPTY_BODY = YES;
> + CLANG_WARN_ENUM_CONVERSION = YES;
> + CLANG_WARN_INFINITE_RECURSION = YES;
> + CLANG_WARN_INT_CONVERSION = YES;
> + CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> + CLANG_WARN_RANGE_LOOP_ANALYSIS = YES;
> + CLANG_WARN_SUSPICIOUS_MOVE = YES;
> + CLANG_WARN_UNREACHABLE_CODE = YES;
> + CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> + CODE_SIGN_IDENTITY = "-";
> + COPY_PHASE_STRIP = NO;
> + DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym";
> + ENABLE_NS_ASSERTIONS = NO;
> + ENABLE_STRICT_OBJC_MSGSEND = YES;
> + GCC_C_LANGUAGE_STANDARD = gnu11;
> + GCC_NO_COMMON_BLOCKS = YES;
> + GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> + GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> + GCC_WARN_UNDECLARED_SELECTOR = YES;
> + GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> + GCC_WARN_UNUSED_FUNCTION = YES;
> + GCC_WARN_UNUSED_VARIABLE = YES;
> + MACOSX_DEPLOYMENT_TARGET = 10.13;
> + MTL_ENABLE_DEBUG_INFO = NO;
> + SDKROOT = macosx;
> + };
This section should be blank because these settings are picked up from the
*.xcconfig files.
> Tools/ImageDiff/ImageDiff.xcodeproj/project.pbxproj:236
> + buildSettings = {
> + ALWAYS_SEARCH_USER_PATHS = NO;
> + CLANG_ANALYZER_NONNULL = YES;
> + CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION =
YES_AGGRESSIVE;
> + CLANG_CXX_LANGUAGE_STANDARD = "gnu++14";
> + CLANG_CXX_LIBRARY = "libc++";
> + CLANG_ENABLE_MODULES = YES;
> + CLANG_ENABLE_OBJC_ARC = YES;
> + CLANG_WARN_BOOL_CONVERSION = YES;
> + CLANG_WARN_CONSTANT_CONVERSION = YES;
> + CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> + CLANG_WARN_DOCUMENTATION_COMMENTS = YES;
> + CLANG_WARN_EMPTY_BODY = YES;
> + CLANG_WARN_ENUM_CONVERSION = YES;
> + CLANG_WARN_INFINITE_RECURSION = YES;
> + CLANG_WARN_INT_CONVERSION = YES;
> + CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> + CLANG_WARN_RANGE_LOOP_ANALYSIS = YES;
> + CLANG_WARN_SUSPICIOUS_MOVE = YES;
> + CLANG_WARN_UNREACHABLE_CODE = YES;
> + CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> + CODE_SIGN_IDENTITY = "-";
> + COPY_PHASE_STRIP = NO;
> + DEBUG_INFORMATION_FORMAT = dwarf;
> + ENABLE_STRICT_OBJC_MSGSEND = YES;
> + ENABLE_TESTABILITY = YES;
> + GCC_C_LANGUAGE_STANDARD = gnu11;
> + GCC_DYNAMIC_NO_PIC = NO;
> + GCC_NO_COMMON_BLOCKS = YES;
> + GCC_OPTIMIZATION_LEVEL = 0;
> + GCC_PREPROCESSOR_DEFINITIONS = (
> + "DEBUG=1",
> + "$(inherited)",
> + );
> + GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> + GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> + GCC_WARN_UNDECLARED_SELECTOR = YES;
> + GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> + GCC_WARN_UNUSED_FUNCTION = YES;
> + GCC_WARN_UNUSED_VARIABLE = YES;
> + MACOSX_DEPLOYMENT_TARGET = 10.13;
> + MTL_ENABLE_DEBUG_INFO = YES;
> + ONLY_ACTIVE_ARCH = YES;
> + SDKROOT = macosx;
> + };
Ditto.
> Tools/ImageDiff/ImageDiff.xcodeproj/project.pbxproj:278
> + buildSettings = {
> + ALWAYS_SEARCH_USER_PATHS = NO;
> + CLANG_ANALYZER_NONNULL = YES;
> + CLANG_ANALYZER_NUMBER_OBJECT_CONVERSION =
YES_AGGRESSIVE;
> + CLANG_CXX_LANGUAGE_STANDARD = "gnu++14";
> + CLANG_CXX_LIBRARY = "libc++";
> + CLANG_ENABLE_MODULES = YES;
> + CLANG_ENABLE_OBJC_ARC = YES;
> + CLANG_WARN_BOOL_CONVERSION = YES;
> + CLANG_WARN_CONSTANT_CONVERSION = YES;
> + CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> + CLANG_WARN_DOCUMENTATION_COMMENTS = YES;
> + CLANG_WARN_EMPTY_BODY = YES;
> + CLANG_WARN_ENUM_CONVERSION = YES;
> + CLANG_WARN_INFINITE_RECURSION = YES;
> + CLANG_WARN_INT_CONVERSION = YES;
> + CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> + CLANG_WARN_RANGE_LOOP_ANALYSIS = YES;
> + CLANG_WARN_SUSPICIOUS_MOVE = YES;
> + CLANG_WARN_UNREACHABLE_CODE = YES;
> + CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> + CODE_SIGN_IDENTITY = "-";
> + COPY_PHASE_STRIP = NO;
> + DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym";
> + ENABLE_NS_ASSERTIONS = NO;
> + ENABLE_STRICT_OBJC_MSGSEND = YES;
> + GCC_C_LANGUAGE_STANDARD = gnu11;
> + GCC_NO_COMMON_BLOCKS = YES;
> + GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> + GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> + GCC_WARN_UNDECLARED_SELECTOR = YES;
> + GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> + GCC_WARN_UNUSED_FUNCTION = YES;
> + GCC_WARN_UNUSED_VARIABLE = YES;
> + MACOSX_DEPLOYMENT_TARGET = 10.13;
> + MTL_ENABLE_DEBUG_INFO = NO;
> + SDKROOT = macosx;
> + };
Ditto.
> Tools/ImageDiff/cg/ImageDiff.cpp:2
> + * Copyright (C) 2005, 2007, 2015 Apple Inc. All rights reserved.
Maybe update copyright.
Why isn't this named Tools/ImageDiff/cg/ImageDiffCG.cpp?
> Tools/ImageDiff/cg/ImageDiff.cpp:85
> + CGDataProviderRef dataProvider = CGDataProviderCreateWithCFData(data);
> + CGImageRef result = CGImageCreateWithPNGDataProvider(dataProvider, 0,
false, kCGRenderingIntentDefault);
> + CFRelease(data);
> + CFRelease(dataProvider);
Nit: Could call CFRelease(data) one line earlier.
> Tools/ImageDiff/cg/ImageDiff.cpp:151
> + diff = (unsigned char*) diffBuffer;
No space for the C-style cast here. Could use a C++ cast here:
diff = reinterpret_cast<unsigned char*>(diffBuffer);
> Tools/ImageDiff/cg/Configurations/BaseTarget.xcconfig:25
> +OTHER_CFLAGS = $(inherited) -iframework
$(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/Quartz.framework/Frameworks
-iframework
$(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/ApplicationServices.framework/Framew
orks -iframework
$(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/CoreServices.framework/Frameworks
-iframework
$(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/WebKit.framework/Frameworks;
> +OTHER_CPLUSPLUSFLAGS = $(OTHER_CFLAGS);
I'm pretty sure these only apply to [sdk=macosx*]. They probably don't do
anything on iOS, but they should be:
OTHER_CFLAGS[sdk=macosx*] = $(inherited) -iframework
$(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/Quartz.framework/Frameworks
-iframework
$(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/ApplicationServices.framework/Framew
orks -iframework
$(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/CoreServices.framework/Frameworks
-iframework
$(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/WebKit.framework/Frameworks;
OTHER_CPLUSPLUSFLAGS[sdk=macosx*] = $(OTHER_CFLAGS);
Oh, I guess we're only building ImageDiff for macOS going forward, though. If
so, we don't need the OTHER_LDFLAGS[sdk=iphone*] in ImageDiff.xcconfig then.
> Tools/ImageDiff/cg/Configurations/ImageDiff.xcconfig:24
> +#include "BaseTarget.xcconfig"
You can probably inline BaseTarget.xcconfig into this file, and remove
BaseTarget.xcconfig.
It only existed in the DumpRenderTree project to prevent duplication of
xcconfig settings. No need for it here anymore.
More information about the webkit-reviews
mailing list