Bug 28751

Created attachment 38641 [details] Use Vector<OwnPtr<> > in ICO decoder

Comment on attachment 38641 [details] Use Vector<OwnPtr<> > in ICO decoder LGTM.

Comment on attachment 38641 [details] Use Vector<OwnPtr<> > in ICO decoder Landed in r47800, clearing flags.

Created attachment 38684 [details] Shorten #ifdef section of ImageSource.h

Created attachment 38688 [details] Clean up ImageSource.cpp a little

Created attachment 38689 [details] Clean up ImageDecoder.cpp a little

Created attachment 38693 [details] Clean up ImageDecoder*.cpp in preparation for fixing bug 28785 Added in the other ImageDecoder*.cpp files since I'm looking at them to figure out how to merge them together.

Created attachment 38694 [details] Clean up ImageSource.* in preparation for more fixes on bug 27965 Collapsed the two ImageSource.* cleanup patches together for ease of landing.

Comment on attachment 38693 [details] Clean up ImageDecoder*.cpp in preparation for fixing bug 28785 Why no "static" on: 41 inline void fillScaledValues(Vector<int>& scaledValues, double scaleRate, int length) I guess inline also prevents it from ever becoming a symbol? And here? 55 template <MatchType type> int getScaledValue(const Vector<int>& scaledValues, int valueToMatch, int searchStart) Does static not work on templates? Otherwise this looks fine.

Comment on attachment 38694 [details] Clean up ImageSource.* in preparation for more fixes on bug 27965 + return m_decoder ? m_decoder->isSizeAvailable() : false; return m_decoder && m_decoder->isSizeAvailable(); is how I would write that. Looks OK.

(In reply to comment #9) > (From update of attachment 38693 [details]) > Why no "static" on: > 41 inline void fillScaledValues(Vector<int>& scaledValues, double scaleRate, > int length) > > And here? > 55 template <MatchType type> int getScaledValue(const Vector<int>& > scaledValues, int valueToMatch, int searchStart) Because they have been moved into the pre-existing anonymous namespace, so they don't also need a "static" declaration. Anonymous namespaces and "static" are the two ways to locally scope functions; "static" is deprecated in C++ (Stroustrup secs. 9.2, B.2.3).

(In reply to comment #11) > (In reply to comment #9) > > (From update of attachment 38693 [details] [details]) > > Why no "static" on: > > 41 inline void fillScaledValues(Vector<int>& scaledValues, double scaleRate, > > int length) > > > > And here? > > 55 template <MatchType type> int getScaledValue(const Vector<int>& > > scaledValues, int valueToMatch, int searchStart) > > Because they have been moved into the pre-existing anonymous namespace, so they > don't also need a "static" declaration. > > Anonymous namespaces and "static" are the two ways to locally scope functions; > "static" is deprecated in C++ (Stroustrup secs. 9.2, B.2.3). Random. I didn't realize we used anonymous namespaces in WebCore, or that static is "deprecated".

Comment on attachment 38693 [details] Clean up ImageDecoder*.cpp in preparation for fixing bug 28785 OK.

Comment on attachment 38694 [details] Clean up ImageSource.* in preparation for more fixes on bug 27965 Landed in r47840, clearing flags.

Comment on attachment 38693 [details] Clean up ImageDecoder*.cpp in preparation for fixing bug 28785 Landed in r47841, clearing flags.

Created attachment 46606 [details] Condense downsampling logic This greatly reduces the amount of code in the decoders that support image downsampling. It does this by making the downsampling functions available for all builds, and having them act "as you'd expect" (e.g. scaledSize() returns the same as size() when the image has not been scaled). This allowed me to condense a lot of logic. This also contains the fixes on bug 33624 (which I discovered while writing this patch). There should be no functional change in this patch.

Attachment 46606 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:360: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1

Created attachment 46609 [details] Condense downsampling logic, v2 Fixes style nit and condenses another few lines I didn't previously realize I could shorten.

Comment on attachment 46609 [details] Condense downsampling logic, v2 > #include "ImageDecoder.h" > #endif > > -#if ENABLE(IMAGE_DECODER_DOWN_SAMPLING) > -#ifndef IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS > -#define IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS (1024 * 1024) > -#endif > -#endif > - > namespace WebCore { > > ImageSource::ImageSource() > @@ -81,6 +75,9 @@ void ImageSource::setData(SharedBuffer* > if (!m_decoder) { > m_decoder = static_cast<NativeImageSourcePtr>(ImageDecoder::create(*data)); > #if ENABLE(IMAGE_DECODER_DOWN_SAMPLING) > +#ifndef IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS > +#define IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS (1024 * 1024) > +#endif > if (m_decoder) > m_decoder->setMaxNumPixels(IMAGE_DECODER_DOWN_SAMPLING_MAX_NUMBER_OF_PIXELS); > #endif Why do you move it into the function? I think I was asked to move it out before :)

(In reply to comment #19) > Why do you move it into the function? I think I was asked to move it out before > :) For readability (to keep the constant close to where it's used) and simplicity (one fewer preprocessor block, two fewer lines of code).

Attachment 46609 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/186745

Created attachment 46614 [details] Condense downsampling logic, v3 Try to fix both build warnings and errors on gtk. I intended to reorder the ImageDecoder.h member variables in a later patch but I guess I have to do a little bit to get things in the right order now.

Comment on attachment 46614 [details] Condense downsampling logic, v3 I'm not an expert here, but the minus lines outnumber the plus lines and the plus lines are prettier. More seriously, I looked through this and the parts I understood looked good. There were some calculations that mystified me, but looks plausible. If there's a better reviewer for this code, please let me or Peter know.

Comment on attachment 46614 [details] Condense downsampling logic, v3 Landed in r53309, clearing flags.

Broke Chromium Linux: http://build.webkit.org/builders/Chromium%20Linux%20Release/builds/932/steps/compile-webkit/logs/stdio I'm not sure why the EWS bot didn't catch that...

(In reply to comment #25) > Broke Chromium Linux: > http://build.webkit.org/builders/Chromium%20Linux%20Release/builds/932/steps/compile-webkit/logs/stdio > > I'm not sure why the EWS bot didn't catch that... Attempted fix landed as r53311.

Created attachment 46709 [details] Use OwnPtrs and remove GIFImageDecoderPrivate This moves the private reader objects in several decoders from being raw pointers to OwnPtrs, to simplify the code a little. Since I'm touching this part of the GIF decoder anyway, it also removes GIFImageDecoderPrivate, since that wrapper class doesn't seem to add much value beyond simplifying the "decode" API call, and it takes up a big chunk of space.

Holger, since the latest patch touches Qt code (though trivially so), thought you might want to take a look and/or r+.

Attachment 46709 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/190557

Created attachment 46714 [details] Use OwnPtrs and remove GIFImageDecoderPrivate, v2 Try to fix Qt build error.

Comment on attachment 46714 [details] Use OwnPtrs and remove GIFImageDecoderPrivate, v2 This patch is fantastic.

Comment on attachment 46714 [details] Use OwnPtrs and remove GIFImageDecoderPrivate, v2 Landed in r53411, clearing flags.

Created attachment 48689 [details] Style/code cleanup This somewhat stretches the definition of a "small" cleanup, but it's all mechanical, with no functional change (if you find one, flag it!). I'm just trying to make the Image Decoders more compact, readable, and style-guide-compliant. In particular, this gets rid of the 80-column wrapping in code originally written by me, which Eric Seidel has frequently complained to me about :)

Attachment 48689 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 Last 3072 characters of output: itespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:70: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:72: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:84: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:152: Place brace on its own line for function definitions. [whitespace/braces] [4] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:178: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:240: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:250: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:276: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:288: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:293: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:309: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:368: term_source is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:112: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp:121: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/ImageDecoder.h:61: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/ImageDecoder.h:184: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/ImageDecoder.h:187: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:65: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:66: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:67: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:68: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:318: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 27 If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 48690 [details] Style/code cleanup, v2 This fixes the fixable style comments. There are two cases the style bot will still complain about that aren't fixed: int var; // Indenting these comments the same bool var2; // causes the style bot to complain void jpeg_decompress_struct(); // Funcs/structs used by libjpeg are named/styled to match its conventions, which makes the style bot angry I don't think fixing either of these is advantageous.

Attachment 48690 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:63: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:63: setjmp_buffer is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:67: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:70: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:72: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:87: Declaration has space between type name and * in JPEGImageReader *decoder [whitespace/declaration] [3] WebCore/platform/image-decoders/jpeg/JPEGImageDecoder.cpp:369: term_source is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/platform/image-decoders/ImageDecoder.h:58: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/ImageDecoder.h:59: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/ImageDecoder.h:60: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/ImageDecoder.h:184: One space before end of line comments [whitespace/comments] [5] WebCore/platform/image-decoders/ImageDecoder.h:187: One space before end of line comments [whitespace/comments] [5] Total errors found: 12 If any of these errors are false positives, please file a bug against check-webkit-style.

> void jpeg_decompress_struct(); // Funcs/structs used by libjpeg are > named/styled to match its conventions, which makes the style bot angry We can whitelist these files not to check for underscores in names. Do you know the full set of affected image decoder files?

Comment on attachment 48690 [details] Style/code cleanup, v2 I didn't verify everything in detail, but this patch looks great. Thanks for doing this cleanup!

Comment on attachment 48690 [details] Style/code cleanup, v2 Landed in r54823, clearing flags.

Created attachment 48960 [details] Saner PNG-in-ICO decoding This makes PNG-in-ICO decoding a little less convoluted.

Comment on attachment 48960 [details] Saner PNG-in-ICO decoding I didn't 100% understand the changes to ICOImageDecoder::decodeAtIndex, but the patch looks good. Can we retire this bug and use a new one for future patches?

Landed in r54978. Closing bug per comment 41.

Thanks Peter.