[webkit-reviews] review granted: [Bug 35411] Clean up m_failed usage in open-source image decoders : [Attachment 54817] Part 3, v1

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Peter Kasting
<pkasting at google.com>'s request for review:
Bug 35411: Clean up m_failed usage in open-source image decoders
https://bugs.webkit.org/show_bug.cgi?id=35411

Attachment 54817: Part 3, v1
https://bugs.webkit.org/attachment.cgi?id=54817&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
This patch has been up for review for a long time.  Like many patches to the
image decoders, Peter is the only one who really understands them, consequently
my review here is largely based on my faith in Peter.  I was tempted to give
this patch an R- because it explain the problem it's solving and has some
oddities (such as m_doNothingOnFailure), but on balance, I'm willing to believe
this is an improvement.

Peter, I'm sorry contributing to the image decoders is such a pain.  I don't
really have much of a solution to over you though.  I suspect the net result is
that you're going to end up working on other things and the decoders will sit
around in their current state.	In the end, we'll muddle through.

Thanks for the patch.

WebCore/ChangeLog:13
 +	    No layout tests because this does not change the visible output of
What's the consequence of failing to clean up these objects?

WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp: 
 +		close();
What happened to this call to close()?

WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:232
 +	m_doNothingOnFailure = true;
:(

WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:209
 +	    return false;
We don't even call the parent class setFailed method in this case?