Bug 16133 - Implement pixel test support on Windows
Summary: Implement pixel test support on Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P2 Normal
Assignee: Adam Roben (:aroben)
URL:
Keywords: InRadar, PlatformOnly
Depends on:
Blocks:
 
Reported: 2007-11-25 12:41 PST by Adam Roben (:aroben)
Modified: 2007-11-25 16:04 PST (History)
3 users (show)

See Also:


Attachments
patch with changelog (54.61 KB, patch)
2007-11-25 12:43 PST, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
[1/5] Make Windows DRT stop changing LF into CRLF (1.57 KB, patch)
2007-11-25 14:26 PST, Adam Roben (:aroben)
sam: review+
Details | Formatted Diff | Diff
[2/5] Clean up Windows DRT's option parsing a little bit (2.27 KB, patch)
2007-11-25 14:26 PST, Adam Roben (:aroben)
sam: review+
Details | Formatted Diff | Diff
[3/5] Implement pixel dumping in Windows DRT (25.21 KB, patch)
2007-11-25 14:26 PST, Adam Roben (:aroben)
sam: review+
Details | Formatted Diff | Diff
[4/5] Fix image diff link generation on Windows (1.65 KB, patch)
2007-11-25 14:26 PST, Adam Roben (:aroben)
sam: review+
Details | Formatted Diff | Diff
[5/5] Fix Bug 16133: Implement pixel test support on Windows (19.89 KB, patch)
2007-11-25 14:26 PST, Adam Roben (:aroben)
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2007-11-25 12:41:39 PST
We need to implement pixel test support on Windows to flesh out our testing. I have a patch that does just that.
Comment 1 Adam Roben (:aroben) 2007-11-25 12:43:21 PST
Created attachment 17511 [details]
patch with changelog
Comment 2 Adam Roben (:aroben) 2007-11-25 13:46:37 PST
Comment on attachment 17511 [details]
patch with changelog

I'm going to break this up into some smaller pieces.
Comment 3 Adam Roben (:aroben) 2007-11-25 14:26:26 PST
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(-)
Comment 4 Adam Roben (:aroben) 2007-11-25 14:26:28 PST
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(-)
Comment 5 Adam Roben (:aroben) 2007-11-25 14:26:29 PST
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(-)
Comment 6 Adam Roben (:aroben) 2007-11-25 14:26:31 PST
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(-)
Comment 7 Adam Roben (:aroben) 2007-11-25 14:26:33 PST
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(-)
Comment 8 Sam Weinig 2007-11-25 14:57:38 PST
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
Comment 9 Sam Weinig 2007-11-25 15:06:10 PST
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
Comment 10 Adam Roben (:aroben) 2007-11-25 15:23:12 PST
(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.
Comment 11 Adam Roben (:aroben) 2007-11-25 16:01:58 PST
Comment on attachment 17516 [details]
[1/5]         Make Windows DRT stop changing LF into CRLF

Landed as r28019
Comment 12 Adam Roben (:aroben) 2007-11-25 16:02:37 PST
Comment on attachment 17517 [details]
[2/5]         Clean up Windows DRT's option parsing a little bit

Landed as r28020
Comment 13 Adam Roben (:aroben) 2007-11-25 16:03:07 PST
Comment on attachment 17518 [details]
[3/5]         Implement pixel dumping in Windows DRT

Laned as r28021
Comment 14 Adam Roben (:aroben) 2007-11-25 16:03:25 PST
Comment on attachment 17519 [details]
[4/5]         Fix image diff link generation on Windows

Landed as r28022
Comment 15 Adam Roben (:aroben) 2007-11-25 16:03:46 PST
Comment on attachment 17520 [details]
[5/5] Fix Bug 16133: Implement pixel test support on Windows

Landed as r28023
Comment 16 Adam Roben (:aroben) 2007-11-25 16:04:24 PST
<rdar://5071708>