[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





--- Comment #14 from Sterling Swigart <sswigart at google.com>  2010-06-04 16:25:33 PST ---
(In reply to comment #12)
> (From update of attachment 57835 [details])
> Thanks for iterating! It's getting closer.
> 
> A few things:
> 
> WebCore/ChangeLog does not include latest additions to the build files.

Woops! Got them in now.

> 
> 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.

Done. It's now static and not mentioned in the .h (I wasn't sure what convention was).

> 
> 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.

I went through the cases and realized that I would only end up calling that function with one string to return in mind. Thus, I removed the parameter.

> 
> 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.

Sure, done.

> 
> 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.

I have rethought ownership and destruction of everything and slightly reformatted it. When ImageResizerThread finishes its job and calls the static function on the main thread, after the callbacks it will destroy the ImageResizerThread (which is no longer refcounted) and subsequently the AsyncImageResizer object, because it is refcounted by ImageResizerThread. I also added a comment explaining this (and a bit more) at the top of AsyncImageResizer.h.

Let me know what you think about this new approach.

> 
> >> 
> >> 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.

I removed it for now, then.

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