Bug 16133 – Implement pixel test support on Windows

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

We need to implement pixel test support on Windows to flesh out our testing. I have a patch that does just that.

[Adam Roben (:aroben)]

Created attachment 17511 [details]
patch with changelog

[Adam Roben (:aroben)]

Comment on attachment 17511 [details]
patch with changelog

I'm going to break this up into some smaller pieces.

[Adam Roben (:aroben)]

Created attachment 17516 [details]
[1/5]         Make Windows DRT stop changing LF into CRLF


        Reviewed by NOBODY (OOPS!).

        * DumpRenderTree/win/DumpRenderTree.cpp:
        (main): Put stdout in binary mode.
        * Scripts/run-webkit-tests: Remove the CRLF hack.
---
 WebKitTools/ChangeLog                             |   10 ++++++++++
 WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp |    4 ++++
 WebKitTools/Scripts/run-webkit-tests              |    1 -
 3 files changed, 14 insertions(+), 1 deletions(-)

[Adam Roben (:aroben)]

Created attachment 17517 [details]
[2/5]         Clean up Windows DRT's option parsing a little bit


        Reviewed by NOBODY (OOPS!).

        * DumpRenderTree/win/DumpRenderTree.cpp:
        (main): Put non-option arguments into a Vector.
---
 WebKitTools/ChangeLog                             |    9 +++++++
 WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp |   27 +++++++++++----------
 2 files changed, 23 insertions(+), 13 deletions(-)

[Adam Roben (:aroben)]

Created attachment 17518 [details]
[3/5]         Implement pixel dumping in Windows DRT


        Part of http://bugs.webkit.org/show_bug.cgi?id=16133
        <rdar://5071708>

        Reviewed by NOBODY (OOPS!).

        * DumpRenderTree/cg/PixelDumpSupportCG.cpp: Added.
        (printPNG): Dumps a CGImageRef as a PNG to stdout, along with a
        Content-Length header.
        (getMD5HashStringForBitmap):
        (dumpWebViewAsPixelsAndCompareWithExpected):
        * DumpRenderTree/cg/PixelDumpSupportCG.h: Copied from WebKitTools/DumpRenderTree/mac/DumpRenderTreePasteboard.h.
        * DumpRenderTree/win/DumpRenderTree.cpp:
        (dump): Do a pixel dump if requested.
        (main): Parse pixel test options.
        * DumpRenderTree/win/DumpRenderTree.vcproj: Added new files and added
        the cg/ subdirectory to the include path.
        * DumpRenderTree/win/MD5.cpp: Added. Windows MD5 functions aren't
        available in a header or import library, so we have to go through this
        LoadLibrary/GetProcAddress dance to use them.
        (cryptDLL):
        (init):
        (update):
        (final):
        (MD5_Init):
        (MD5_Update):
        (MD5_Final):
        * DumpRenderTree/win/MD5.h: Added.
        * DumpRenderTree/win/PixelDumpSupport.h: Added. This file should be
        moved up to the top level to share it with Mac eventually.
        * DumpRenderTree/win/PixelDumpSupportWin.cpp: Added.
        (getBitmapContextFromWebView): Forces the WebView to paint using a
        WM_PRINTCLIENT message, and puts the result in a CGBitmapContext.
---
 WebKitTools/ChangeLog                              |   37 +++++++
 .../DumpRenderTree/cg/PixelDumpSupportCG.cpp       |  105 ++++++++++++++++++++
 WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.h |   41 ++++++++
 WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp  |   21 ++++
 .../DumpRenderTree/win/DumpRenderTree.vcproj       |   34 ++++++-
 WebKitTools/DumpRenderTree/win/MD5.cpp             |   77 ++++++++++++++
 WebKitTools/DumpRenderTree/win/MD5.h               |   45 +++++++++
 WebKitTools/DumpRenderTree/win/PixelDumpSupport.h  |   36 +++++++
 .../DumpRenderTree/win/PixelDumpSupportWin.cpp     |   67 +++++++++++++
 9 files changed, 458 insertions(+), 5 deletions(-)

[Adam Roben (:aroben)]

Created attachment 17519 [details]
[4/5]         Fix image diff link generation on Windows


        Reviewed by NOBODY (OOPS!).

        * Scripts/run-webkit-tests: Removed unnecessary and incorrect calls
        to toURL.
---
 WebKitTools/ChangeLog                |    9 +++++++++
 WebKitTools/Scripts/run-webkit-tests |    6 +++---
 2 files changed, 12 insertions(+), 3 deletions(-)

[Adam Roben (:aroben)]

Created attachment 17520 [details]
[5/5] Fix Bug 16133: Implement pixel test support on Windows


WebKit/win:

        Add ImageDiff.vcproj to WebKit.sln

        Reviewed by NOBODY (OOPS!).

        * WebKit.vcproj/WebKit.sln:

WebKitTools:

        Port ImageDiff to CG and C++

        Final part of http://bugs.webkit.org/show_bug.cgi?id=16133
        <rdar://5071708>

        Reviewed by NOBODY (OOPS!).

        * DumpRenderTree/DumpRenderTree.sln: Added ImageDiff.vcproj.
        * DumpRenderTree/cg/ImageDiff.cpp: Added.
        (main):
        (createImageFromStdin):
        (compareImages):
        (getDifferenceBitmap):
        (computePercentageDifferent):
        * DumpRenderTree/win/ImageDiff.vcproj: Added.
---
 WebKit/win/ChangeLog                            |    8 +
 WebKit/win/WebKit.vcproj/WebKit.sln             |   21 ++-
 WebKitTools/ChangeLog                           |   18 ++
 WebKitTools/DumpRenderTree/DumpRenderTree.sln   |    8 +
 WebKitTools/DumpRenderTree/cg/ImageDiff.cpp     |  203 +++++++++++++++++++++++
 WebKitTools/DumpRenderTree/win/ImageDiff.vcproj |  182 ++++++++++++++++++++
 6 files changed, 435 insertions(+), 5 deletions(-)

[Sam Weinig]

Comment on attachment 17518 [details]
[3/5]         Implement pixel dumping in Windows DRT

Style wise, this should be the first #include (we should really add a config.h).
+#include "PixelDumpSupportCG.h"

You should include <wtf/StringExtras.h> instead of doing this #define.
+#define snprintf _snprintf


Please use the WTF ASSERT
+    assert(bitsPerPixel == 32); // ImageDiff assumes 32 bit RGBA, we must as well.

+    assert(bytesPerRow >= (pixelsWide * bytesPerPixel));

In PixelDumpSupportCG.h it doesn't look like you need to #include <wtf/Platform.h>.  Also, are you sure it's a good idea to return a RetainPtr?  That feels weird to me.

None of these are show-stoppers.  r=me

[Sam Weinig]

Comment on attachment 17520 [details]
[5/5] Fix Bug 16133: Implement pixel test support on Windows

I can't image that forwarding the main function is helpful.
+/* prototypes */
+int main(int argc, const char* argv[]);

Please use c++ style casts here.
+    percentage = (float)((int)(percentage * 100.0f)) / 100.0f; // round to 2 decimal places

Please use ASSERT.
+    assert(!(bitmapInfo & kCGImageAlphaFirst));
+    assert(!(bitmapInfo & kCGBitmapFloatComponents));


r=me

[Adam Roben (:aroben)]

(In reply to comment #8)
> (From update of attachment 17518 [details] [edit])
> Also, are you sure it's a good idea to return a RetainPtr?  That feels weird to me.

It probably feels weird to you because we don't have a "PassRetainPtr" (the equivalent of PassRefPtr for CF types). All this means is that we'll do an extra CFRetain/CFRelease when the function returns. Since these are not performance critical functions, I think the extra retain count churn is worth the lifetime guarantees that come along with using RetainPtr as the return type.

[Adam Roben (:aroben)]

Comment on attachment 17516 [details]
[1/5]         Make Windows DRT stop changing LF into CRLF

Landed as r28019

[Adam Roben (:aroben)]

Comment on attachment 17517 [details]
[2/5]         Clean up Windows DRT's option parsing a little bit

Landed as r28020

[Adam Roben (:aroben)]

Comment on attachment 17518 [details]
[3/5]         Implement pixel dumping in Windows DRT

Laned as r28021

[Adam Roben (:aroben)]

Comment on attachment 17519 [details]
[4/5]         Fix image diff link generation on Windows

Landed as r28022

[Adam Roben (:aroben)]

Comment on attachment 17520 [details]
[5/5] Fix Bug 16133: Implement pixel test support on Windows

Landed as r28023

[Adam Roben (:aroben)]

<rdar://5071708>