[webkit-reviews] review denied: [Bug 40018] Image Resizer Patch 1: Adding basic classes to perform the resizing : [Attachment 57835] Fixed style, added reference in Android.mk

[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 57835: Fixed style, added reference in Android.mk
https://bugs.webkit.org/attachment.cgi?id=57835&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
Thanks for iterating! It's getting closer.

A few things:

WebCore/ChangeLog does not include latest additions to the build files.

WebCore/html/ImageResizerThread.cpp:44
 +  void returnBlobOrError(void* callbackInformation)
This can be just a static function, in which case it does not need to be
mentioned in .h file.

WebCore/html/ImageResizerThread.cpp:48
 +	    callbackInfo->asyncImageResizer->resizeError("Unexpected error
while resizing image");
Do we need this string here? If you plan on having multiple detailed text
messages, I'd worry about localization. If it's just an "error happened" then
it could be moved to the palce that actually creates parameters for JS error
callback. In other words, resizeError may not have any parameters now.

WebCore/html/AsyncImageResizer.cpp:43
 +  AsyncImageResizer::AsyncImageResizer(CachedImage* cachedImage, OutputType
outputType, IntSize desiredBounds, ScriptValue successCallback, ScriptValue
errorCallback, float quality, AspectRatioOption aspectRatioOption,
OrientationOption orientationOption)
I'd ASSERT in the constructor that at least one of callbacks isObject().
Otherwise, the bailout should have happened before creating AIR.

WebCore/html/ImageResizerThread.h:50
 +	    AsyncImageResizer* asyncImageResizer;
What guarantees that this pointer still points to a live AsyncImageResizer when
it's time to invoke the callback? If the document is closed while image is
resized on the thread, the AsyncImageResizer could be destroyed. Either AIR
could be refcounted and you would have RefPtr here (ok w/o being threadsafe
since it's only used on main thread), or the destruction of AIR should somehow
prevent the ImageResizerThread callback from attempt to use this pointer, for
example calling some 'requestTermination()' method on the ImageResizeThread.

Same question about ImageResizerThread as well - once ImageResizerThread posted
the callback to be executed on the main thread, it could take a while.
Meanwhile the main thread can unload the document and destroy
AsyncImageResizer, which will release ImageResizerThread and m_callbackInfo.
Then the main thread will try to use it and crash. You can AddRef the
ImageResizerThread in its constructor and Release right after detachThread to
make sure it is not destroyed underneath the physical thread because of
document unload.

>> 
>> WebCore/html/ImageResizerThread.h:59
>>  +	   Mutex m_threadCreationMutex;
>> If we don't really use this mutex now, lets remove it and add when actual
code that uses it is added.
>
>This is used to protect references to m_threadID. The other threading classes
do the same thing. You don't think I need it?

The actual thread function is not executed until threadID is established (see
WTF::createThread). A comment in WorkerThread is a bit confusing. I think you
might need some mutex in the future if you want to do termination of the thread
and if the call to terminate comes after start() but before the thread function
actually initializes new thread enough to do correct termination. In other
words, I'd remove it now because the code you have now does not need it. You'll
likely need to add it back later, but it's better to do it this way because
then it'll be clear why you need it.