[webkit-reviews] review granted: [Bug 170608] ImageDiff: Add CG implementation for new ImageDiff : [Attachment 309491] Rebased patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 9 09:58:28 PDT 2017


Alex Christensen <achristensen at apple.com> has granted  review:
Bug 170608: ImageDiff: Add CG implementation for new ImageDiff
https://bugs.webkit.org/show_bug.cgi?id=170608

Attachment 309491: Rebased patch

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




--- 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.


More information about the webkit-reviews mailing list