[webkit-reviews] review denied: [Bug 50905] [chromium] Simplify the PNG encoder. : [Attachment 76832] patch update changelog description
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Dec 17 10:57:29 PST 2010
David Levin <levin at chromium.org> has denied noel gordon
<noel.gordon at gmail.com>'s request for review:
Bug 50905: [chromium] Simplify the PNG encoder.
https://bugs.webkit.org/show_bug.cgi?id=50905
Attachment 76832: patch update changelog description
https://bugs.webkit.org/attachment.cgi?id=76832&action=review
------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76832&action=review
Looks good. It could possibly be checked in as is but I have a concern about
passing 0 to png_create_info_struct.
A few things to consider or respond to and then I'd be happy to r+ this.
> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:69
> + png_struct* png = png_create_write_struct(PNG_LIBPNG_VER_STRING, 0, 0,
0);
Why not just do
if (!png)
return false;
as before?
This way NULL will never get passed to png_create_info_struct and it simplifies
the call to png_destroy_write_struct (by getting rid of the check for png).
> WebCore/platform/image-encoders/skia/PNGImageEncoder.cpp:82
> + row.resize(imageSize.width() * bitmap.bytesPerPixel());
Why use bitmap.bytesPerPixel() when preMultipliedBGRAtoRGBA will always use 4
bytes per pixel (4 unsigned char)?
Also why not move Vector<unsigned char> row; to here? (You could even pass this
size to the constructor.)
More information about the webkit-reviews
mailing list