[Webkit-unassigned] [Bug 21336] ImageDiff tool improvements

[bugzilla-daemon at webkit.org]

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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24057|review?                     |review-
               Flag|                            |




------- Comment #4 from darin at apple.com  2008-10-03 13:34 PDT -------
(From update of attachment 24057)
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.


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