Bug 40018

Comment on attachment 57576 [details] Added AsyncImageResizer.h/cpp, added ImageResizerThread.h/cpp Your patch fails to apply to tip of tree.

Created attachment 57596 [details] Merged with latest project.pbxproj revision

Created attachment 57604 [details] Added Changelog

Comment on attachment 57604 [details] Added Changelog One generic note: It's better to add the new files to all build files in the same patch, not only to xcodeproj. It also would be nice to add FIXME into places that will be implemented later. WebCore/html/AsyncImageResizer.h:45 + enum MimeType { This adds a MimeType enum to WebCore namespace. It's a big namespace, so it would be better to have the enum scoped inside of AsyncImageResizer class. In this case, it could be named OutputType and have values JPEG and PNG (caps since they are abbreviations). WebCore/html/AsyncImageResizer.h:52 + AsyncImageResizer(CachedImage*, MimeType, float width, float height, ScriptValue successCallback, ScriptValue errorCallback, float quality, bool preserveAspectRatio, bool rotateExif); If width and height are in pixels, then we should use IntSize instead of them both. The parameter could be named more descriptive, perhaps 'desiredBounds', which would convey that it's not necessarily the size of the resulting image, because aspect ration could dictate one of the sizes to be different. Also, the bool parameters usually are represented as enums with 2 nicely named values, for the better readability in callsites. For example, 'enum AspectRatioOption { PreserveAspectRatio, IgnoreAspectRatio }'. WebCore/html/AsyncImageResizer.h:60 + void imageChanged(CachedImage*, const IntRect* = 0); virtual overrides usually retain 'virtual' keyword. WebCore/html/AsyncImageResizer.cpp:39 + This empty line is not needed. WebCore/html/AsyncImageResizer.cpp:49 + m_thread = ImageResizerThread::create(this, mimeType, width, height, quality, preserveAspectRatio, rotateExif); This constructor gets a raw pointer to CachedImage. It seems it needs CachedImage to be alive even after constructor exits. Perhaps the code you are going to write in the future will call getBlobAsync right away, but this is not obvious from the code now. I think it is better to make it a client of CachedImage right here, this will keep CachedImage alive. WebCore/html/AsyncImageResizer.cpp:56 + void AsyncImageResizer::getBlobAsync() 'getFoo' normally should return Foo. It's hard to tell what's better name is since this method is not used and incomplete. Maybe remove it for now, moving addClient to the constructor? WebCore/html/ImageResizerThread.h:44 + void returnBlob(void* imageResizerThread); This function is not in the patch, so this could be removed. WebCore/html/ImageResizerThread.h:58 + // Threading attributes Comments should end with a '.'. 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. WebCore/html/ImageResizerThread.h:65 + bool m_error; These fields are not used in this patch, lets not to add them. WebCore/html/ImageResizerThread.h:67 + // Image attributes '.' WebCore/html/ImageResizerThread.h:62 + // Results and callback information '.' at the end. WebCore/html/ImageResizerThread.cpp:44 + void returnBlobOrError(void* /*imageResizerThread*/) It seems this patch's idea is to add a resizer class and a thread that gets started, does the job (except the job code will be added later), then posts callback to the resizer and exits. It seems the patch does all that except actually calling resizer back when thread terminates. It'd be nice to complete this piece of functionality, even if the callback on the resizer object will be empty for now.

Created attachment 57823 [details] Fixed based on Dmitry's response

(In reply to comment #4) > (From update of attachment 57604 [details]) > One generic note: It's better to add the new files to all build files in the same patch, not only to xcodeproj. Oh, I didn't know about the other ones, but I followed 39083 and added references to the new files in (hopefully) all of the necessary places. > It also would be nice to add FIXME into places that will be implemented later. I added these where applicable. > > WebCore/html/AsyncImageResizer.h:45 > + enum MimeType { > This adds a MimeType enum to WebCore namespace. It's a big namespace, so it would be better to have the enum scoped inside of AsyncImageResizer class. In this case, it could be named OutputType and have values JPEG and PNG (caps since they are abbreviations). The enums are now part of the AsyncImageResizer class. Referring to them from outside the class is quite a mouthful now, but I can deal with that. > > WebCore/html/AsyncImageResizer.h:52 > + AsyncImageResizer(CachedImage*, MimeType, float width, float height, ScriptValue successCallback, ScriptValue errorCallback, float quality, bool preserveAspectRatio, bool rotateExif); > If width and height are in pixels, then we should use IntSize instead of them both. The parameter could be named more descriptive, perhaps 'desiredBounds', which would convey that it's not necessarily the size of the resulting image, because aspect ration could dictate one of the sizes to be different. Good call. Done. > > Also, the bool parameters usually are represented as enums with 2 nicely named values, for the better readability in callsites. For example, 'enum AspectRatioOption { PreserveAspectRatio, IgnoreAspectRatio }'. Done. > > WebCore/html/AsyncImageResizer.h:60 > + void imageChanged(CachedImage*, const IntRect* = 0); > virtual overrides usually retain 'virtual' keyword. Done. > > WebCore/html/AsyncImageResizer.cpp:39 > + > This empty line is not needed. Deleted. > > WebCore/html/AsyncImageResizer.cpp:49 > + m_thread = ImageResizerThread::create(this, mimeType, width, height, quality, preserveAspectRatio, rotateExif); > This constructor gets a raw pointer to CachedImage. It seems it needs CachedImage to be alive even after constructor exits. Perhaps the code you are going to write in the future will call getBlobAsync right away, but this is not obvious from the code now. I think it is better to make it a client of CachedImage right here, this will keep CachedImage alive. That's a good point. Now, constructing AsyncImageResizer is all that needs to be done to do the resizing. > > WebCore/html/AsyncImageResizer.cpp:56 > + void AsyncImageResizer::getBlobAsync() > 'getFoo' normally should return Foo. It's hard to tell what's better name is since this method is not used and incomplete. Maybe remove it for now, moving addClient to the constructor? I removed this function based on your last comment. > > WebCore/html/ImageResizerThread.h:44 > + void returnBlob(void* imageResizerThread); > This function is not in the patch, so this could be removed. It is now called returnBlobOrError, and it is now functional based on your request below. It will call the correct callback function in AsyncImageResizer. > > WebCore/html/ImageResizerThread.h:58 > + // Threading attributes > Comments should end with a '.'. Check. > > 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? > > WebCore/html/ImageResizerThread.h:65 > + bool m_error; > These fields are not used in this patch, lets not to add them. Re-added in the struct CallbackInfo, which is now used and filled. > > WebCore/html/ImageResizerThread.h:67 > + // Image attributes > '.' Check. > > WebCore/html/ImageResizerThread.h:62 > + // Results and callback information > '.' at the end. Check. > > WebCore/html/ImageResizerThread.cpp:44 > + void returnBlobOrError(void* /*imageResizerThread*/) > It seems this patch's idea is to add a resizer class and a thread that gets started, does the job (except the job code will be added later), then posts callback to the resizer and exits. It seems the patch does all that except actually calling resizer back when thread terminates. It'd be nice to complete this piece of functionality, even if the callback on the resizer object will be empty for now. I completed this part. Since this function isn't in the ImageResizerThread class, it can't access its fields unless I expose a number of getters, so I store all the callback information in the new struct CallbackInfo.

Created attachment 57829 [details] Fixed based on Dmitry's response

Created attachment 57830 [details] Forgot to commit my newest changes before diffing

Created attachment 57834 [details] Took steps to avoid previous .pbxproj conflicts

Attachment 57834 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/html/ImageResizerThread.cpp:44: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/html/ImageResizerThread.cpp:48: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 57835 [details] Fixed style, added reference in Android.mk

Comment on attachment 57835 [details] Fixed style, added reference in Android.mk 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.

Created attachment 57927 [details] Corrected 2nd set of Dmitry's comments

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

I forgot to add in my last comment that I didn't finish handling one case, which is if the document is destroyed/unloaded while I am resizing the objects. In that case, I would want to be able to sense that and block the callbacks from occurring. I was thinking there would be some sort of document observer that would allow me to receive notifications of document unloading, but I haven't been able to find anything that can do that for non-Element objects. If you have any ideas, let me know! This case is what the FIXME in AsyncImageResizer.h is for.

Comment on attachment 57927 [details] Corrected 2nd set of Dmitry's comments 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.

Created attachment 58175 [details] Fixed Dmitry's 3rd set of comments

(In reply to comment #16) > (From update of attachment 57927 [details]) > 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. Yep, this works better. I tested it out on my branch connected to JS. > > WebCore/html/AsyncImageResizer.h:80 > + ImageResizerThread* m_thread; > This variable seems to be not used. Took it out, was an artifact. > > 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. I did exactly that. Lifetime is much more clear now anyway.

Created attachment 58256 [details] Attempt to fix Win build error

Created attachment 58259 [details] Fixing Win build error...

Created attachment 58265 [details] Fixing build errors

Comment on attachment 58265 [details] Fixing build errors I believe those can be final ones: WebCore/html/AsyncImageResizer.h:51 + class AsyncImageResizer : public CachedResourceClient, public RefCounted<AsyncImageResizer> { I think AsyncImageResizer should be ThreadSafeShared rather the RefCounted since its refcounted from 2 threads and the code is not structured specifically to provide safe RefCounted operation. WebCore/html/ImageResizerThread.h:46 + struct CallbackInfo { For this struct, I'd have a constructor to make sure all fields are initialized. Also, it seems 'bool error' is not necessary since blob==0 could do the same. Also, it's unclear what error==true, blob!=0 would mean. Could we just remove error and use !blob as indicator of error? At least for now, before we actually need it. WebCore/html/ImageResizerThread.h:52 + static bool start(RefPtr<SharedBuffer> imageData, RefPtr<AsyncImageResizer>, AsyncImageResizer::OutputType, IntSize desiredBounds, float quality, AsyncImageResizer::AspectRatioOption, AsyncImageResizer::OrientationOption); usually parameters are PassRefPtr<T> to avoi creating RefPtrs and churning refcount. WebCore/html/ImageResizerThread.h:56 + ImageResizerThread(RefPtr<AsyncImageResizer>, AsyncImageResizer::OutputType, IntSize desiredBounds, float quality, AsyncImageResizer::AspectRatioOption, AsyncImageResizer::OrientationOption); Ditto about PassRefPtr

Created attachment 58303 [details] Patch 1

Comment on attachment 58303 [details] Patch 1 r=me

Comment on attachment 58303 [details] Patch 1 Rejecting patch 58303 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Dmitry Titov', u'--force']" exit_code: 1 Last 500 characters of output: Hunk #3 FAILED at 13402. Hunk #4 succeeded at 16927 (offset 12 lines). Hunk #5 succeeded at 18146 (offset 24 lines). Hunk #6 succeeded at 19775 (offset 14 lines). Hunk #7 succeeded at 20469 (offset 23 lines). 1 out of 7 hunks FAILED -- saving rejects to file WebCore/WebCore.xcodeproj/project.pbxproj.rej patching file WebCore/html/AsyncImageResizer.cpp patching file WebCore/html/AsyncImageResizer.h patching file WebCore/html/ImageResizerThread.cpp patching file WebCore/html/ImageResizerThread.h Full output: http://webkit-commit-queue.appspot.com/results/3212130

Created attachment 58385 [details] Patch 1 - Fixed commit-queue error

Comment on attachment 58385 [details] Patch 1 - Fixed commit-queue error thanks for resolving against fresh sources, lets try it again...

Comment on attachment 58385 [details] Patch 1 - Fixed commit-queue error Clearing flags on attachment: 58385 Committed r61000: <http://trac.webkit.org/changeset/61000>

All reviewed patches have been landed. Closing bug.