[Webkit-unassigned] [Bug 58242] [GTK] Implement pixel dump support for WebKitTestRunner

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=58242





--- Comment #3 from Adam Roben (:aroben) <aroben at apple.com>  2011-04-22 08:18:08 PST ---
(From update of attachment 89035)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.