After #57068 closes, WebKitTestRunner will run, but not be able to dump test pixels. This bug tracks adding that feature.
Created attachment 89035 [details] Patch
Comment on attachment 89035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89035&action=review This change looks great! I've added a few comments for future thought. I don't think anything I mention is worth addressing in this change, except perhaps the "ref" call. > Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:42 > + Note: I'm not at all sure that this ref() is needed here. In CoreGraphics, the allocation of the image surface includes a callback routine that performs the cleanup. We might need to either omit this ref() here, or make sure that the destructor for the PassOwnPtr<GraphicsContext> somehow decrements the reference count here. > Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:61 > + Note that I cribbed most (all?) of this routine from ImageCairo.cpp. It would be nice if this could be shared someplace common for both implementations.
Comment on attachment 89035 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=89035&action=review > Source/WebKit2/Shared/ShareableBitmap.h:116 > + // This creates a copied BitmapImage (most likely a copy-on-write) of the shareable bitmap. > + cairo_surface_t* makeImageCopy(); > + > + // This creates a BitmapImage that directly references the shared bitmap data. > + // This is only safe to use when we know that the contents of the shareable bitmap won't change. > + cairo_surface_t* makeImage(); Seems like these should return PassRefPtr, given that you use RefPtr<cairo_surface_t> in the implementation. (I didn't know that was possible!) Is makeImageCopy() really doing a copy-on-write? CG does that behind-the-scenes on Mac. >> Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp:42 >> + ref(); // Balanced by deref in releaseBitmapContextData. >> + > > Note: I'm not at all sure that this ref() is needed here. In CoreGraphics, the allocation of the image surface includes a callback routine that performs the cleanup. We might need to either omit this ref() here, or make sure that the destructor for the PassOwnPtr<GraphicsContext> somehow decrements the reference count here. I agree with Brent. This looks like a leak to me. But if you remove this ref(), what will guarantee that the ShareableBitmap stays alive as long as the cairo_surface_t might access its data? > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:110 > +void TestInvocation::dumpPixelsAndCompareWithExpected(WKImageRef wkImage) > +{ > + cairo_surface_t* surface = WKImageCreateCairoSurface(wkImage); > + > + char actualHash[33]; > + computeMD5HashStringForCairoContext(surface, actualHash); > + fprintf(stdout, "\nActualHash: %s\n", actualHash); > + > + // Check the computed hash against the expected one and dump image on mismatch > + bool hashesMatch = false; > + if (m_expectedPixelHash.length() > 0) { > + ASSERT(m_expectedPixelHash.length() == 32); > + > + fprintf(stdout, "\nExpectedHash: %s\n", m_expectedPixelHash.c_str()); > + > + // FIXME: Do case insensitive compare. > + if (m_expectedPixelHash == actualHash) > + hashesMatch = true; > + } > + > + if (!hashesMatch) > + dumpBitmap(surface); > } Seems like we should refactor the code so that all ports can share the logic in the latter part of this function.
Comment on attachment 89035 [details] Patch Removing r? in response to aroben's comments.
Created attachment 91520 [details] Proposed update to ShareableBitmap implementation
Created attachment 97162 [details] Patch
I've updated the patch here with Adam's suggestions.
Comment on attachment 97162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97162&action=review > Source/WebKit2/ChangeLog:12 > + * Shared/API/c/cairo/WKImageCairo.h: Copied from Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp. This file seems to be missing from your patch. > Tools/WebKitTestRunner/TestInvocation.h:50 > + bool compareActualHashToExpectedAndDumpResults(char[33]); Can the argument be a const char[33]? > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:49 > + for (unsigned row = 0; row < pixelsHigh; row++) { Might as well use size_t and preincrement here. > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:50 > + md5Context.addBytes(bitmapData, 4 * pixelsWide); Is it worth checking the bits-per-pixel of the surface instead of assuming 32? > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:92 > + cairo_surface_t* surface = WKImageCreateCairoSurface(wkImage); Can you use a RefPtr here? > Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:-145 > - // FIXME: Do case insensitive compare. You lost this FIXME when moving this code.
Created attachment 98079 [details] Patch wth Adam's suggestions
(In reply to comment #8) Thanks for the review. I've attached a new patch, which I hope addresses your comments. > (From update of attachment 97162 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97162&action=review > > > Source/WebKit2/ChangeLog:12 > > + * Shared/API/c/cairo/WKImageCairo.h: Copied from Source/WebKit2/Shared/cairo/ShareableBitmapCairo.cpp. > > This file seems to be missing from your patch. Wow. For some reason webkit-patch isn't picking it up. :( > > Tools/WebKitTestRunner/TestInvocation.h:50 > > + bool compareActualHashToExpectedAndDumpResults(char[33]); > > Can the argument be a const char[33]? Fixed! > > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:49 > > + for (unsigned row = 0; row < pixelsHigh; row++) { > > Might as well use size_t and preincrement here. Fixed! > > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:50 > > + md5Context.addBytes(bitmapData, 4 * pixelsWide); > > Is it worth checking the bits-per-pixel of the surface instead of assuming 32? I assert above that the format matches what we expect here. Is that enough? I believe that getting any other image format at this point represents an error. > > Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:92 > > + cairo_surface_t* surface = WKImageCreateCairoSurface(wkImage); > > Can you use a RefPtr here? I had hoped so originally, but the Cairo RefPtr specializations are not header files and are not exported publically from libwebkitetetc.so. > > Tools/WebKitTestRunner/cg/TestInvocationCG.cpp:-145 > > - // FIXME: Do case insensitive compare. > > You lost this FIXME when moving this code. Fixed!
Comment on attachment 97162 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97162&action=review >>> Tools/WebKitTestRunner/cairo/TestInvocationCairo.cpp:50 >>> + md5Context.addBytes(bitmapData, 4 * pixelsWide); >> >> Is it worth checking the bits-per-pixel of the surface instead of assuming 32? > > I assert above that the format matches what we expect here. Is that enough? I believe that getting any other image format at this point represents an error. Sounds fine to me.
Comment on attachment 98079 [details] Patch wth Adam's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=98079&action=review > Source/WebKit2/Shared/API/c/cairo/WKImageCairo.h:33 > +typedef _cairo_surface cairo_surface_t; Seems like you're missing a "struct" in here.
Comment on attachment 98079 [details] Patch wth Adam's suggestions Clearing flags on attachment: 98079 Committed r89426: <http://trac.webkit.org/changeset/89426>
All reviewed patches have been landed. Closing bug.