[webkit-reviews] review denied: [Bug 40018] Image Resizer Patch 1: Adding basic classes to perform the resizing : [Attachment 57927] Corrected 2nd set of Dmitry's comments

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Sterling Swigart
<sswigart at google.com>'s request for review:
Bug 40018: Image Resizer Patch 1: Adding basic classes to perform the resizing
https://bugs.webkit.org/show_bug.cgi?id=40018

Attachment 57927: Corrected 2nd set of Dmitry's comments
https://bugs.webkit.org/attachment.cgi?id=57927&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
Getting closer and hotter :-) There is one more note about lifetime, see below.


WebCore/html/AsyncImageResizer.cpp:68
 +	if (!cachedImage->stillNeedsLoad() && cachedImage->isLoaded()) {
I wonder if using notifyFinished() callback of CachedResourceClient w/o this
check is a better way to go here, because the check is not needed then.

WebCore/html/AsyncImageResizer.h:80
 +	ImageResizerThread* m_thread;
This variable seems to be not used.

WebCore/html/ImageResizerThread.cpp:51
 +	delete callbackInfo->imageResizerThread;
Now you have 2 'delete' operations on the ImageResizerThread - one here and one
in thread function. Not sure this will work.

One way to fix this could be to not keep CallbackInfo as part of the thread
object (so you need to keep tread object alive until callback is fired) but
rather allocate an independent CalbackInfo instance when thread is done
resizing, fill it in and schedule callback on the main thread - and then
detach/delete the thread, since the callback call is now independent. Keeping
AsyncImageResizer in RefPtr along the way is a good thing, it'll keep it alive
until calback is actually fired (whether or not it'l reach the javascript is a
separate issue for future). At the end of this static callback function, the
CallbackInfo can be deleted.

r- because of 2 'delete's, but it's close.