In bug #85299 there's a patch to move GTK+ port to use a cairo only image diff. While working on it I realized we have 3 implementations with a lot of duplicated code, so as part of the patch I refactored the code to share all the common code. On top of that patch adding the CG implementations is easy.
Created attachment 306522 [details] Patch
Created attachment 306683 [details] Rebased patch
Created attachment 306687 [details] Rebased patch
hmm, the debug failure is because of the ASSERT, doesn't image diff link to WTF already? or am I missing a header include? I can easily fix the build by removing the ASSERT, I guess.
Created attachment 306807 [details] Fix debug build I have no idea why it fails to build so I've just removed the assert.
Created attachment 306812 [details] Include win changes I've realized that the patch would break windows, because it removes the ImageDiffCG from DRT. So, this new patch also includes the windows changed that was going to do in a follow up patch. I have no way to test windows changes, so I don't know whether it works or not.
Attachment 306812 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Ping reviewers
This builds and seems to work locally.
Comment on attachment 306812 [details] Include win changes Looks good, and works properly on Windows (at least locally). Let's land it.
(In reply to Brent Fulgham from comment #10) > Comment on attachment 306812 [details] > Include win changes > > Looks good, and works properly on Windows (at least locally). Let's land it. Thanks! I hope it also works in WinCairo . . .
Comment on attachment 306812 [details] Include win changes Rejecting attachment 306812 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: eC /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff.build/Objects-normal/x86_64/PlatformImage.o /Volumes/Data/EWS/WebKit/Tools/ImageDiff/PlatformImage.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff.build/Objects-normal/x86_64/ImageDiff.o /Volumes/Data/EWS/WebKit/Tools/ImageDiff/ImageDiff.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (2 failures) Full output: http://webkit-queues.webkit.org/results/3582981
(In reply to WebKit Commit Bot from comment #12) > Comment on attachment 306812 [details] > Include win changes > > Rejecting attachment 306812 [details] from commit-queue. > > Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', > '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', > '--no-clean', '--no-update', '--build-style=release', '--port=mac']" > exit_code: 2 cwd: /Volumes/Data/EWS/WebKit > > Last 500 characters of output: > eC > /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff. > build/Objects-normal/x86_64/PlatformImage.o > /Volumes/Data/EWS/WebKit/Tools/ImageDiff/PlatformImage.cpp normal x86_64 c++ > com.apple.compilers.llvm.clang.1_0.compiler > CompileC > /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff. > build/Objects-normal/x86_64/ImageDiff.o > /Volumes/Data/EWS/WebKit/Tools/ImageDiff/ImageDiff.cpp normal x86_64 c++ > com.apple.compilers.llvm.clang.1_0.compiler > (2 failures) > > Full output: http://webkit-queues.webkit.org/results/3582981 #include <WebCore/PlatformExportMacros.h> ^ 1 error generated. I have no idea why this is failing to build now.
(In reply to Carlos Garcia Campos from comment #13) > (In reply to WebKit Commit Bot from comment #12) > > Comment on attachment 306812 [details] > > Include win changes > > > > Rejecting attachment 306812 [details] from commit-queue. > > > > Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', > > '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', > > '--no-clean', '--no-update', '--build-style=release', '--port=mac']" > > exit_code: 2 cwd: /Volumes/Data/EWS/WebKit > > > > Last 500 characters of output: > > eC > > /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff. > > build/Objects-normal/x86_64/PlatformImage.o > > /Volumes/Data/EWS/WebKit/Tools/ImageDiff/PlatformImage.cpp normal x86_64 c++ > > com.apple.compilers.llvm.clang.1_0.compiler > > CompileC > > /Volumes/Data/EWS/WebKit/WebKitBuild/DumpRenderTree.build/Release/ImageDiff. > > build/Objects-normal/x86_64/ImageDiff.o > > /Volumes/Data/EWS/WebKit/Tools/ImageDiff/ImageDiff.cpp normal x86_64 c++ > > com.apple.compilers.llvm.clang.1_0.compiler > > (2 failures) > > > > Full output: http://webkit-queues.webkit.org/results/3582981 > > #include <WebCore/PlatformExportMacros.h> > ^ > 1 error generated. > > I have no idea why this is failing to build now. Could someone help me with this? I don't know why this is failing now. It worked for me locally and in EWS, but now it's failing, and I don't know what have changed since then.
It builds fine for me locally, and EWS liked it. I'd say commit it and see what happens. I'm not sure why the commit queue didn't like it.
(In reply to Alex Christensen from comment #15) > It builds fine for me locally, and EWS liked it. I'd say commit it and see > what happens. I'm not sure why the commit queue didn't like it. The thing is that I tried yesterday in a mac and got the same error than EWS.
I'm trying to rebase this patch now that ImageDiff has been moved from DRT to its own project, but I don't see the ImageDiff now in Xcode to do the changes. Is there any magic I need to do in Xcode for the new project to show up?
Manually adding a new group to WebKit.xcworkspace/contents.xcworkspacedata made it appear in XCode.
Created attachment 309491 [details] Rebased patch
Attachment 309491 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:27: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309491 [details] Rebased patch Attachment 309491 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3705722 New failing tests: compositing/absolute-inside-out-of-view-fixed.html fast/forms/file/intrinsic-min-width-overrides-width.html
Created attachment 309498 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Created attachment 309500 [details] Rebase again after r216504
Attachment 309500 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:27: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 309501 [details] Try to fix win build
Attachment 309501 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:27: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309491 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=309491&action=review r=me. There's room for improvement, but this is an improvement because it unites the code from the different platforms. > Tools/ImageDiff/cg/PlatformImageCG.cpp:116 > +PlatformImage::PlatformImage(CGImageRef image) > + : m_image(image) > +{ > +} > + > +PlatformImage::~PlatformImage() > +{ > + CFRelease(m_image); > + free(m_buffer); > +} I think it would be nicer to have the constructor call CFRetain and have the callers properly release their images. We shouldn't rely on the caller knowing that this object adopts it without any annotation. I guess it's just a small tool that nobody looks at often, but it should still make sense. > Tools/ImageDiff/cg/PlatformImageCG.cpp:145 > + m_buffer = calloc(height, rowBytes); It would be nice if we could use std::unique_ptr<uint8_t[]> or something so we could avoid all this manual memory management. Just glancing at this makes it hard to tell if we should check m_buffer for something already there and free it if there is.
(In reply to Alex Christensen from comment #27) > Comment on attachment 309491 [details] > Rebased patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=309491&action=review > > r=me. There's room for improvement, but this is an improvement because it > unites the code from the different platforms. Yes, I tried to keep existing code as mush as possible, just removing all the duplications. > > Tools/ImageDiff/cg/PlatformImageCG.cpp:116 > > +PlatformImage::PlatformImage(CGImageRef image) > > + : m_image(image) > > +{ > > +} > > + > > +PlatformImage::~PlatformImage() > > +{ > > + CFRelease(m_image); > > + free(m_buffer); > > +} > > I think it would be nicer to have the constructor call CFRetain and have the > callers properly release their images. We shouldn't rely on the caller > knowing that this object adopts it without any annotation. I guess it's > just a small tool that nobody looks at often, but it should still make sense. My previous patch used RetainPtr everywhere and the constructor received a &&, but then ImageDiff was made standalone and WTF dependency removed, so I had to go back to manually release everything as existing code does. Note that I'm not familiar with CG APIs. > > Tools/ImageDiff/cg/PlatformImageCG.cpp:145 > > + m_buffer = calloc(height, rowBytes); > > It would be nice if we could use std::unique_ptr<uint8_t[]> or something so > we could avoid all this manual memory management. Just glancing at this > makes it hard to tell if we should check m_buffer for something already > there and free it if there is. I agree, but again this is just moving existing code. We can add further improvements in follow up patches. I don't think we need to null check m_buffer since free is null safe and m_buffer is initialized to nullptr in the header.
Created attachment 309510 [details] Try to fix win build
Attachment 309510 [details] did not pass style-queue: ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:27: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:36: Alphabetical sorting problem. [build/include_order] [4] ERROR: Tools/ImageDiff/cg/PlatformImageCG.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 3 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 309510 [details] Try to fix win build Clearing flags on attachment: 309510 Committed r216576: <http://trac.webkit.org/changeset/216576>
All reviewed patches have been landed. Closing bug.