[Webkit-unassigned] [Bug 40018] Image Resizer Patch 1: Adding basic classes to perform the resizing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40018


Dmitry Titov <dimich at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57835|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #12 from Dmitry Titov <dimich at chromium.org>  2010-06-04 02:38:41 PST ---
(From update of attachment 57835)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.