[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 #57604|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #4 from Dmitry Titov <dimich at chromium.org>  2010-06-03 09:01:16 PST ---
(From update of attachment 57604)
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.

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