[webkit-reviews] review denied: [Bug 25709] Skia/Cairo image decoders should be merged : [Attachment 30210] Part one - v1

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Peter Kasting <pkasting at google.com>'s
request for review:
Bug 25709: Skia/Cairo image decoders should be merged
https://bugs.webkit.org/show_bug.cgi?id=25709

Attachment 30210: Part one - v1
https://bugs.webkit.org/attachment.cgi?id=30210&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Yay!

This patch is pretty big, making it difficult to review.

For example, all the places where you're only re-indenting code (like
ImageDecoder.h) would be easier to review in a smaller patch...

Why is it OK to remove:
#if PLATFORM(CAIRO) || PLATFORM(QT) || PLATFORM(WX)
from BMPImageDecoder?  Is that file not included in the Windows build?	For a
while WinCG and WinCairo were sharing a solution file...

WK Style:
 185	     decode(GIFFullQuery, index+1); // Decode this frame.
would say index + 1 I think.

 407	     unsigned data_size = num * sizeof(unsigned);
dataSize


What's the ADDRESS_TAG_BIT stuff you're removing?  You don't mention it in the
ChangeLog...

I assume x is never used in this loop:
	  for (unsigned i = 0; i < info->output_width; ++i) {
 483	     for (unsigned x = 0; x < info->output_width; ++x) {

Style:
4 void PNGImageDecoder::decodingFailed() {
 225	 m_failed = true;
 226 }

What's the skia/BMPImageDecoder.h change?  Line endings?
skia/BMPImageReader.h ?

Why is this OK?
 bool JPEGImageDecoder::isSizeAvailable()
421421	       decode(true);
422422	   }
423423 
424	 return !m_failed && ImageDecoder::isSizeAvailable();
 424	 return ImageDecoder::isSizeAvailable();
425425 }

This really mostly need more documentation in the ChangeLog.  And I'd prefer to
land this in smaller pieces.

Otherwise, this looks fine.  I assume you ran the WebKit Windows (Apple's port)
still built fine after this?

r-, mostly due to lack of explanation in the ChangeLog and the above nits. 
Again, I'd rather land this in smaller pieces, but I'll happily review whatever
you post next.