Bug 21124 - --pixel should be ignored for tests that use dumpAsText()
: --pixel should be ignored for tests that use dumpAsText()
Status: RESOLVED FIXED
: WebKit
Tools / Tests
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-09-25 16:59 PST by
Modified: 2008-09-30 17:43 PST (History)


Attachments
Patch, changelog (2.09 KB, patch)
2008-09-30 17:40 PST, Simon Fraser (smfr)
mitz: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-09-25 16:59:21 PST
If I understand things right, tests that use the dumpAsText() instead of dumping the render tree are supposed to be platform agnostic, so their results are in the same directory as the tests instead of in "platform".

Pixel test are by definition not platform agnostic (destination screen color profile, font rendering differences, etc...), so their result should always be in "platform".

If you combine these two assumptions, then having a test that uses dumpAsText() also have its rendering be pixel-tested does not make sense.
But today, if you run "WebKitTools/Scripts/run-webkit-tests --pixel LayoutTests/", it will generate images for such tests.

For instance, 3 out of the 6 tests in LayoutTests/transforms/2d are dumpAsText() tests and do not print the render tree:
- compound-2d-transforms.html
- transform-2d.html
- transform-accuracy.html
But since there are no "base" images in LayoutTests/platform/mac/transforms/2d for these tests, DRT always creates news ones in LayoutTests/transforms/2d and this pollutes the directory.

I think the DRT tool when running in --pixel-test mode should not compare pixel rendering of tests that use dumpAsText().
------- Comment #1 From 2008-09-25 17:05:59 PST -------
I thought that this was already the case.  Perhaps we broke this behaviour somewhere along the way.
------- Comment #2 From 2008-09-25 17:12:19 PST -------
We want to add a bunch of tests for transforms, transitions and animations, and some of them require pixel testing. Having this fixed sooner than later would help, thanks.
------- Comment #3 From 2008-09-26 13:06:09 PST -------
For some reason, you can't just bypass dumpWebViewAsPixelsAndCompareWithExpected() completely in DRT or the tests hangs.

Here's a diff that seems to fix the issue (it's a one-line change, but because of (re)tabbing a bunch of lines, it makes it look quite bigger):

Index: WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.cpp
===================================================================
--- WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.cpp    (revision 36958)
+++ WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.cpp    (working copy)
@@ -106,21 +106,25 @@
     if (gLayoutTestController->dumpSelectionRect())
         drawSelectionRect(context.get(), getSelectionRect());
 #endif
+    
+    // Only process the rendered image if have a test that is not platform-agnositc i.e. doesn't use "dumpAsText()"
+    if(!gLayoutTestController->dumpAsText()) {

-    // Compute the actual hash to compare to the expected image's hash.
-    char actualHash[33];
-    getMD5HashStringForBitmap(context.get(), actualHash);
-    printf("\nActualHash: %s\n", actualHash);
+        // Compute the actual hash to compare to the expected image's hash.
+        char actualHash[33];
+        getMD5HashStringForBitmap(context.get(), actualHash);
+        printf("\nActualHash: %s\n", actualHash);

-    // FIXME: We should compare the actualHash to the expected hash here and
-    // only set dumpImage to true if they don't match, but DRT doesn't have
-    // enough information currently to find the expected checksum file.
-    bool dumpImage = true;
+        // FIXME: We should compare the actualHash to the expected hash here and
+        // only set dumpImage to true if they don't match, but DRT doesn't have
+        // enough information currently to find the expected checksum file.
+        bool dumpImage = true;

-    if (dumpImage) {
-        RetainPtr<CGImageRef> image(AdoptCF, CGBitmapContextCreateImage(context.get()));
-        printPNG(image.get());
+        if (dumpImage) {
+            RetainPtr<CGImageRef> image(AdoptCF, CGBitmapContextCreateImage(context.get()));
+            printPNG(image.get());
+        }
     }
-
+    
     printf("#EOF\n");
 }
------- Comment #4 From 2008-09-30 17:40:25 PST -------
Created an attachment (id=23965) [details]
Patch, changelog
------- Comment #5 From 2008-09-30 17:42:16 PST -------
(From update of attachment 23965 [details])
r=me
------- Comment #6 From 2008-09-30 17:43:44 PST -------
    M    WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
    M    WebKitTools/ChangeLog
    M    WebKitTools/Scripts/run-webkit-tests
r37129 = 783a6ba5d73a582590947cb76f7859d8d42b4f9c (trunk)