[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

--- Comment #28 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Alex Christensen from comment #27)
> Comment on attachment 309491 [details]
> 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.

Yes, I tried to keep existing code as mush as possible, just removing all the duplications.

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

My previous patch used RetainPtr everywhere and the constructor received a &&, but then ImageDiff was made standalone and WTF dependency removed, so I had to go back to manually release everything as existing code does. Note that I'm not familiar with CG APIs.

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

I agree, but again this is just moving existing code. We can add further improvements in follow up patches. I don't think we need to null check m_buffer since free is null safe and m_buffer is initialized to nullptr in the header.

-- 
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/cf8e3a73/attachment-0001.html>