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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu May 14 21:52:10 PDT 2009

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

Attachment 30210: Part one - v1

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

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:
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);

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

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) {

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	   }
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.

More information about the webkit-reviews mailing list