[webkit-reviews] review denied: [Bug 37117] Add platform-independent JPEG/PNG image encoders and toDataURL : [Attachment 52646] fix style errors

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Yong Li <yong.li.webkit at gmail.com>'s
request for review:
Bug 37117: Add platform-independent JPEG/PNG image encoders and toDataURL
https://bugs.webkit.org/show_bug.cgi?id=37117

Attachment 52646: fix style errors
https://bugs.webkit.org/attachment.cgi?id=52646&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
This code is nutty, but I think the image decoder libraries force you into some
of this nuttiness.  Some nits below.  Please get pkasting to give your revised
patch an informal review.

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:39
 +	    jpeg_destination_mgr* base = this;
Woah, crazy.  Is this what we're supposed to do here?

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:50
 +	    // Zero memory
I like the static cast trick you did above.  Can we do that here?  In
principle, we can get in trouble with this code if we add virtual functions to
this class.

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:61
 +	dest->next_output_byte	=
reinterpret_cast<JOCTET*>(dest->m_buffer.data());
We don't usually align the = sign.  We just have once space.

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:59
 +	JPEGDestinationManager* dest =
(JPEGDestinationManager*)compressData->dest;
Please use a C++ style cast like you do below.

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:83
 +	longjmp(err->m_setjmpBuffer, -1);
Crazy!

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:88
 +	struct jpeg_compress_struct compressData= { 0 };
Missing space before =

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:109
 +	Vector<JSAMPLE, 600 * 3> rowBuffer;
No space around *

WebCore/platform/image-encoders/JPEGImageEncoder.cpp:108
 +	// rowBuffer must be defined here so that its destructor is always
called even when "setjmp" catches an error.
setjmp/longjmp are insane, especially when mixed with C++ constructs.

WebCore/platform/image-encoders/JPEGImageEncoder.h:21
 +  
Extra blank line here.

WebCore/platform/image-encoders/PNGImageEncoder.cpp:51
 +	Vector<char>* m_out;
Why is this called m_out but the corresponding variable for JPEG is called
m_dump?

WebCore/platform/image-encoders/PNGImageEncoder.cpp:62
 +	memcpy(&(*state->m_out)[oldSize], data, size);
&(*state->m_out)[oldSize] is too hard to understand.  Please use temporary
variables to explain what this insanity does.

WebCore/platform/image-encoders/PNGImageEncoder.cpp:87
 +	png_struct* pngPtr = png_create_write_struct(PNG_LIBPNG_VER_STRING,
We usually put these sorts of things on one line.