[webkit-reviews] review denied: [Bug 40018] Image Resizer Patch 1: Adding basic classes to perform the resizing : [Attachment 57604] Added Changelog

[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 57604: Added Changelog
https://bugs.webkit.org/attachment.cgi?id=57604&action=review

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
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.