[webkit-reviews] review denied: [Bug 21336] ImageDiff tool improvements : [Attachment 24057] Patch v2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Oct 3 13:34:30 PDT 2008


Darin Adler <darin at apple.com> has denied Pierre-Olivier Latour
<pol at apple.com>'s request for review:
Bug 21336: ImageDiff tool improvements
https://bugs.webkit.org/show_bug.cgi?id=21336

Attachment 24057: Patch v2
https://bugs.webkit.org/attachment.cgi?id=24057&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The ChangeLog isn't good. It only lists a bug number and doesn't mention what
the change is. Also, it has "set EMAIL_ADDRESS environment variable" in it,
rather than your email address.

+ /**
+  * Generates an RGBA8 bitmap in the reference image colorspace containing the
differences between the 2 images
+  */

We would use "//" comments for this, not /* ones, and we don't do the "/**"
thing ever.

-    RetainPtr<CGContextRef> context(AdoptCF,
CGBitmapContextCreate(CFDataGetMutableBytePtr(data),
CGImageGetWidth(testBitmap), CGImageGetHeight(testBitmap),
-	 CGImageGetBitsPerComponent(testBitmap),
CGImageGetBytesPerRow(testBitmap), colorSpace.get(), kCGBitmapByteOrder32Little
| kCGImageAlphaPremultipliedFirst));

+    CGContextRef context =
CGBitmapContextCreate(CFDataGetMutableBytePtr(data),
CGImageGetWidth(referenceImage), CGImageGetHeight(referenceImage), 8, rowBytes,
CGImageGetColorSpace(referenceImage), kCGImageAlphaPremultipliedLast |
kCGBitmapByteOrder32Big);

It's not a good idea to change the use of RetainPtr here. It makes the patch
bigger than it needs to be, for no real benefit.

But also, WebKit project coding style guide is to put the result of a "create"
function directly into a RetainPtr rather than waiting -- this minimizes the
chance of programming mistakes leading to memory leaks.

+    if (context == 0)
+	 return 0;

WebKit project coding style is to say "if (!context)" in a case like this.

-
-    // NOTE: This may not be safe when switching between ENDIAN types
+    

Removing the comment is fine. But you should just change the whitespace back.

-    float totalPixels = static_cast<float>(pixelsHigh * pixelsWide);
-    return (differences * 100.f) / totalPixels;
+    return static_cast<float>(differences) / static_cast<float>(pixelsHigh *
pixelsWide) * 100.f;

Why this change?

I don't understand when the diffBitmap can be 0. Would that be due to a
colorspace problem perhaps? Is the output from the tool clear enough?

Overall, these changes look fine, assuming that the existing test results still
work as-is. If they don't, then the patch should include both the changes to
ImageDiff *and* the other changes needed to keep tests passing, not just the
ImageDiff part.

review- because of the ChangeLog issues at least. Please consider my other
comments as well.


More information about the webkit-reviews mailing list