[Webkit-unassigned] [Bug 170608] ImageDiff: Add CG implementation for new ImageDiff

[bugzilla-daemon at webkit.org]

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

Alex Christensen <achristensen at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #309491|                            |review+
              Flags|                            |

--- Comment #27 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 309491
  --> https://bugs.webkit.org/attachment.cgi?id=309491
Rebased patch

View in context: https://bugs.webkit.org/attachment.cgi?id=309491&action=review

r=me.  There's room for improvement, but this is an improvement because it unites the code from the different platforms.

> Tools/ImageDiff/cg/PlatformImageCG.cpp:116
> +PlatformImage::PlatformImage(CGImageRef image)
> +    : m_image(image)
> +{
> +}
> +
> +PlatformImage::~PlatformImage()
> +{
> +    CFRelease(m_image);
> +    free(m_buffer);
> +}

I think it would be nicer to have the constructor call CFRetain and have the callers properly release their images.  We shouldn't rely on the caller knowing that this object adopts it without any annotation.  I guess it's just a small tool that nobody looks at often, but it should still make sense.

> Tools/ImageDiff/cg/PlatformImageCG.cpp:145
> +    m_buffer = calloc(height, rowBytes);

It would be nice if we could use std::unique_ptr<uint8_t[]> or something so we could avoid all this manual memory management.  Just glancing at this makes it hard to tell if we should check m_buffer for something already there and free it if there is.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20170509/e52e4526/attachment.html>