[webkit-reviews] review denied: [Bug 22191] logic error in CachedImage.cpp : [Attachment 25081] Swaps lines.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ojan Vafai <ojan at chromium.org>'s
request for review:
Bug 22191: logic error in CachedImage.cpp
https://bugs.webkit.org/show_bug.cgi?id=22191

Attachment 25081: Swaps lines.
https://bugs.webkit.org/attachment.cgi?id=25081&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
You say that the mistake in this function is obvious. But it's not obvious to
me. It seems to me that the clear() function needs to be called before
m_errorOccurred, because we want it to actually discard any decoded data in the
partially created image.

In fact, I'm not sure why the check for m_errorOccurred in the
destroyDecodedData is helpful. It seems that we do want to destroy the decoded
data even if an error occurred.

You say that this causes a crash in Chromium, but you don't supply any details.


I'm not convinced this is the correct fix.

> +	Fixes logic of error() to first set
> +	m_errorOccurred, then call clear(),
> +	which depends on m_errorOccurred having the
> +	correct value.

Please don't use tabs in ChangeLog -- our repository is configured so it
doesn't allow tabs.

I think it's likely that if this is really a bug there's some way to reproduce
a crash in ports other than Chromium and therefore we can create a regression
test.

If you remain convinced that this patch is 1) correct and 2) not testable in
other ports, then feel free to submit the patch again, but I don't think it's
obvious that this is a correct change.