[webkit-reviews] review granted: [Bug 115766] really fixed the memory leak : [Attachment 200988] Patch

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 115766: really fixed the memory leak
https://bugs.webkit.org/show_bug.cgi?id=115766

Attachment 200988: Patch
https://bugs.webkit.org/attachment.cgi?id=200988&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=200988&action=review


r=me, but I think the ChangeLog should be improved before landing.

> Tools/ChangeLog:6
> +	   I needed to adopt the pointer to fix the memory leak.
> +	   https://bugs.webkit.org/show_bug.cgi?id=115766
> +
> +	   Reviewed by NOBODY (OOPS!).

This would be better if the title stated what it's doing, and then a
descriptive sentence later on mentioned the SVN revision in which the leak was
introduced, the SVN revision in which you attempted to fix it earlier, and why
that fix was incorrect. Something like:

<http://webkit.org/b/115766> Fix a memory leak introduced in r149692

Reviewed by NOBODY (OOPS!).

In r149692, the fix for <http://webkit.org/b/42324>, a call to
WKBundleFrameCopyWebArchive was added without any matching call to WKRelease.
An earlier attempted fix in r149697 introduced a RetainPtr but failed to adopt
the object.

> Tools/ChangeLog:10
> +	   really fixed memory leak

This should be a complete sentence. It wouldn't hurt to describe the fix
either. "Fix the memory leak by switching to WKRetainPtr and adopting the
returned object."