ImageDiff should not depend on WTF and bmalloc. Remove these dependencies and make the project a stand-alone project on Mac.
<rdar://problem/30743805>
Created attachment 308910 [details] Patch
Attachment 308910 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:152: Missing space before ( in for( [whitespace/parens] [5] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:161: An else should appear on the same line as the preceding } [whitespace/newline] [4] Total errors found: 4 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 308913 [details] Patch
Attachment 308913 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:35: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:152: Missing space before ( in for( [whitespace/parens] [5] Total errors found: 3 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Build Bot from comment #5) > Attachment 308913 [details] did not pass style-queue: > > > ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:34: Alphabetical sorting problem. > [build/include_order] [4] > ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:35: Alphabetical sorting problem. > [build/include_order] [4] > ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:152: Missing space before ( in for( > [whitespace/parens] [5] > Total errors found: 3 in 7 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. Oddly, these do not show up locally when I run check-webkit-style. The first 2 are not issues that should be fixed, if I recall correctly, winsock2.h and windows.h are a pair of headers which ware order-sensitive when imported. The second is an error which will be fixed in the next iteration of this patch.
Comment on attachment 308913 [details] Patch Attachment 308913 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3665618 New failing tests: fast/borders/border-image-fill-no-border.html
Created attachment 308924 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 308913 [details] Patch Attachment 308913 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3665626 New failing tests: fast/borders/border-image-fill-no-border.html
Created attachment 308925 [details] Archive of layout-test-results from ews116 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 308913 [details] Patch Attachment 308913 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3665824 New failing tests: compositing/overlap-blending/children-opacity-no-overlap.html
Created attachment 308931 [details] Archive of layout-test-results from ews102 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 308933 [details] Patch
Attachment 308933 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 308933 [details] 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/Frameworks -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/Frameworks -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.
Created attachment 308978 [details] Patch
Attachment 308978 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 308978 [details] Patch A few comments on the changes. Generally, yes, this is meant to be built only on Mac. But, I'm not going to specifically disable building it for other platforms (namely iOS simulator) so that we don't break anything while I'm making these changes. First, I moved over some of the CMake stuff. That does need to be done since the next patch will move ImageDiff out of DumpRenderTree entirely. Second, along with DebugRelease.xcconfig, Base.xcconfig needed to be moved as well. Both had some WebKit references in them, those were removed. Lastly, I don't think your comment about OTHER_CPLUSPLUSFLAGS is correct. Or at least, reading through our other config files, it doesn't seem to be correct. Looking at something like WebKitTestRunnerApp.xcconfig, it seems that a variable without [platform*] applies to all platforms. Otherwise, to point back to the WebKitTestRunnerApp example, that configuration file would not link WebKitTestRunnerApp against the required frameworks.
Created attachment 309049 [details] Patch
Attachment 309049 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 309050 [details] Patch
Attachment 309050 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309050 [details] Patch Needed to get the GTK build working.
Comment on attachment 309050 [details] 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.
Created attachment 309094 [details] Patch
Comment on attachment 309094 [details] Patch CQ-ing this because I will need to land it locally since it also requires that we ignore some files.
Attachment 309094 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:34: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/ImageDiff.cpp:35: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed in <https://trac.webkit.org/changeset/216207/webkit>.
Comment on attachment 309094 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=309094&action=review > 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. So, now both ImageDiff are built at the same time, and to the same output? I was trying to remove all duplicated implementations of Imagediff in bug #170608 and this si adding yet another one. Should I rebase that patch on top of this new imageDiff?
(In reply to Carlos Garcia Campos from comment #29) > Comment on attachment 309094 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309094&action=review > > > 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. > > So, now both ImageDiff are built at the same time, and to the same output? I > was trying to remove all duplicated implementations of Imagediff in bug > #170608 and this si adding yet another one. Should I rebase that patch on > top of this new imageDiff? https://bugs.webkit.org/show_bug.cgi?id=168945 will be removing the ImageDiff inside DumpRenderTree. That should be landing shortly and leave all ImageDiff code inside Tools/ImageDiff. The reason for this, by the way, is so that iOS can use a Mac ImageDiff instead of an iOS image.
(In reply to Carlos Garcia Campos from comment #29) > Comment on attachment 309094 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309094&action=review > > > 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. > > So, now both ImageDiff are built at the same time, and to the same output? I > was trying to remove all duplicated implementations of Imagediff in bug > #170608 and this si adding yet another one. Should I rebase that patch on > top of this new imageDiff? Just looked at #170608. Yes, I would rebase that patch on top of this new ImageDiff. Don't worry about the old one in DumpRenderTree, I'm about to remove it. We're building double for the time being so that we don't break any infrastructure.
(In reply to Jonathan Bedard from comment #30) > (In reply to Carlos Garcia Campos from comment #29) > > Comment on attachment 309094 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=309094&action=review > > > > > 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. > > > > So, now both ImageDiff are built at the same time, and to the same output? I > > was trying to remove all duplicated implementations of Imagediff in bug > > #170608 and this si adding yet another one. Should I rebase that patch on > > top of this new imageDiff? > > https://bugs.webkit.org/show_bug.cgi?id=168945 will be removing the > ImageDiff inside DumpRenderTree. That should be landing shortly and leave > all ImageDiff code inside Tools/ImageDiff. Ah, perfect, thanks! > The reason for this, by the way, is so that iOS can use a Mac ImageDiff > instead of an iOS image.