Bug 98098

Summary:

Parse GIF images without full decoding

Product:

WebKit

Reporter:

Hin-Chung Lam <hclam>

Component:

Images

Assignee:

Hin-Chung Lam <hclam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, cc-bugs, dongseong.hwang, enne, jamesr, koivisto, nduca, nick, pkasting, psolanki, qinmin, senorblanco, simon.fraser, skyul, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

109007, 109662, 110776, 111137, 111144

Bug Blocks:

98285, 99784, 111395

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
hacked gif decoder for local testing	none

Hin-Chung Lam

Reported 2012-10-01 17:09:47 PDT

This is a mild blocker for deferred image decoding. There are several things that should be improved with GIF image decoder: 1. GIFImageDecoder::frameCount() shouldn't be a O(n^2) operation frameCount() constructs a GIFImageReader every time there is new bytes coming this is a very inefficient implementation. This is related to deferred image decoding because frameCount() is progressive and we don't know precisely the frame count from just the header. This means we will erroneously try to defer a GIF image and later find out it is animated. 2. GIFImageDecoder should allow random access to frame header GIFImageDecoder decodes the image serially and doesn't allow peaking into next frame's header. This prohibits checking if the next frame is complete and query its duration. This is the reason why we don't defer image decoding on an animated GIF. If we solve these two problems we can support deferred image decoding for *ALL* GIF images or not at all.

Attachments
Patch (79.56 KB, patch) 2012-10-10 21:53 PDT, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (79.89 KB, patch) 2013-02-04 01:59 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (87.26 KB, patch) 2013-02-04 23:06 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (77.12 KB, patch) 2013-02-05 21:47 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Patch (77.12 KB, patch) 2013-02-05 21:49 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
hacked gif decoder for local testing (9.49 KB, patch) 2013-02-20 11:45 PST, Hin-Chung Lam	no flags	Details Formatted Diff Diff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Hin-Chung Lam

Comment 1 2012-10-10 21:53:00 PDT

Created attachment 168141 [details] Patch

Hin-Chung Lam

Comment 2 2012-10-10 22:05:50 PDT

I've upload a patch to demonstrate how to solve the problem for GIFs to enable deferred image decoding. The patch is almost code complete but there are several things remaining: 1. Haven't tested the change in BitmapImage on CG. 2. More tolerant of bad images. Because parsing goes very far, an error in a future frame means entire image cannot be displayed. 3. Unit tests. I plan to do this in Chromium's webkit_unittests. 4. Testing on a large archive of images, this is done locally to make sure results are bit exact, doesn't cause more crashes, etc. Here is how the algorithm works. The state machine for GIF parsing remains the same. During a FrameCountQuery that doesn't need full image decoding we do the following things: 1. When a complete LZW block is encountered, save the information about the block, i.e. block position, block size. 2. When end of frame is reached, save the decode context for this frame and proceed parsing next frame. When doing a FullQuery (i.e. image decoding) for a particular frame, earlier frame contexts are processed to generate all previous frames and LZW blocks before continuing.

Hin-Chung Lam

Comment 3 2012-10-11 13:51:51 PDT

I probably overdid it, shouldn't have changed the style in the file. Don't bother with the style changes I'll upload a slimmed down patch.

Hin-Chung Lam

Comment 4 2012-10-18 18:19:50 PDT

This is also blocking 99874 because deferred image decoding cannot handle caching and decoding of animated GIFs.

Hin-Chung Lam

Comment 5 2013-02-04 01:59:05 PST

Created attachment 186326 [details] Patch

WebKit Review Bot

Comment 6 2013-02-04 02:01:05 PST

Attachment 186326 [details] did not pass style-queue: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:43: Alphabetical sorting problem. [build/include_order] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:90: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:91: block_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:97: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:100: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:101: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:101: rows_remaining is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:102: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:103: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:106: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:106: x_offset is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:106: y_offset is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:108: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:109: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:109: disposal_method is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:110: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:110: local_colormap_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:111: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:111: local_colormap_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:112: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:112: frame_index is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:114: is_local_colormap_defined is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:115: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:115: progressive_display is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:116: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:117: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:117: is_transparent is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:118: is_decode_started is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:120: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:120: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:120: delay_time is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:122: previous_blocks is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:185: The parameter name "query" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:197: The parameter name "query" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:213: The parameter name "query" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:221: bytes_to_consume is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:222: bytes_read is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:229: global_colormap_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:230: global_colormap_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:231: images_data_complete is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:235: image_complete is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:238: raw_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:241: current_frame is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:244: previous_frames is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:246: lzw_context is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:105: output_row is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:156: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:156: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:162: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:163: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:218: do_lzw is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:218: bytes_to_decode is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:231: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:232: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:234: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:235: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:236: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:237: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:237: clear_code is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:238: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:239: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:241: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:242: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:243: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:244: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:245: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:246: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:249: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:327: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:352: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:359: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:360: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:361: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:362: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:363: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:364: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:365: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:366: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:376: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:439: data_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:441: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:442: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:445: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:447: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:448: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:454: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:455: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:456: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:457: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:458: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:459: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:460: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:462: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:471: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:471: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:472: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:474: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:472: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:475: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:477: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:477: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:484: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:485: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:528: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:576: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:636: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:639: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:642: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:646: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:647: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:648: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:648: disposal_method is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:649: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:650: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:651: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:652: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:654: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:735: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:735: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:735: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:755: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:758: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:766: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:767: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:769: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:772: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:773: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:774: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:775: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:780: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:783: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:784: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:786: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:789: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:792: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:795: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:797: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:797: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:799: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:799: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:810: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:811: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:812: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:817: num_colors is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:826: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:834: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:849: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:850: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:855: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:868: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:869: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:871: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:874: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:875: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:876: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:877: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:880: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:881: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:890: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:892: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:893: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:896: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:905: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:918: clear_code is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:945: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:945: bytes_to_decode is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:952: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:953: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:954: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCoreFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/BitmapImage.cpp', u'Source/WebCore/platform/graphics/ImageSource.cpp', u'Source/WebCore/platform/graphics/ImageSource.h', u'Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp', u'Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.h', u'Source/WebCore/platform/image-decoders/ImageDecoder.cpp', u'Source/WebCore/platform/image-decoders/ImageDecoder.h', u'Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp', u'Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/WebKit.gyp', u'Source/WebKit/chromium/WebKit.gypi', u'Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp', u'Source/WebKit/chromium/tests/data/broken.gif']" exit_code: 1 /platform/image-decoders/gif/GIFImageReader.cpp:955: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:966: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:967: block_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:977: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:977: bytes_to_decode is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:982: bytes_left is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:63: The parameter name "disposalMethod" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h:80: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 179 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Hin-Chung Lam

Comment 7 2013-02-04 23:06:49 PST

Created attachment 186553 [details] Patch

Hin-Chung Lam

Comment 8 2013-02-04 23:10:17 PST

Comment on attachment 186553 [details] Patch Phew the code is ready for review now, after sitting in dust for a few months. Please ignore obvious style problems. GIFImageReader was written in hacker style and didn't follow WebKit style. Fixing style would make it much harder for review. I'll submit a patch for style fix after this patch hopefully lands.

Hin-Chung Lam

Comment 9 2013-02-04 23:12:37 PST

pkasting: You had experience with this GIF decoder code, can you review this? WebKit reviewer is still needed but we can do it afterwards.

WebKit Review Bot

Comment 10 2013-02-04 23:14:01 PST

Attachment 186553 [details] did not pass style-queue: Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:90: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:91: block_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:97: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:100: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:101: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:101: rows_remaining is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:102: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:103: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:106: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:106: x_offset is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:106: y_offset is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:108: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:109: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:109: disposal_method is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:110: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:110: local_colormap_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:111: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:111: local_colormap_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:112: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:112: frame_index is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:114: is_local_colormap_defined is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:115: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:115: progressive_display is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:116: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:117: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:117: is_transparent is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:118: is_decode_started is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:120: One space before end of line comments [whitespace/comments] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:120: Should have a space between // and comment [whitespace/comments] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:120: delay_time is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:122: previous_blocks is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:221: bytes_to_consume is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:222: bytes_read is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:229: global_colormap_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:230: global_colormap_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:231: images_data_complete is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:235: image_complete is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:238: raw_data is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:241: current_frame is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:244: previous_frames is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:246: lzw_context is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:156: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:156: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:157: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:162: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:163: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:218: bytes_to_decode is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:231: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:232: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:233: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:234: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:235: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:236: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:237: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:237: clear_code is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:238: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:239: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:241: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:242: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:243: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:244: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:245: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:246: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:249: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:327: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:352: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:359: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:360: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:361: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:362: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:363: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:364: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:365: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:366: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:439: data_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:441: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:442: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:445: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:447: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:448: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:454: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:455: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:456: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:457: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:458: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:459: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:460: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:462: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:471: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:471: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:472: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:474: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:472: An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement. [readability/control_flow] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:475: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:477: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:477: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:484: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:485: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:528: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:576: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:636: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:639: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:642: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:646: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:647: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:648: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:648: disposal_method is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:649: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:650: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:651: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:652: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:654: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:735: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:735: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:735: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:755: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:758: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:766: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:767: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:769: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:772: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:773: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:774: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:775: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:780: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:783: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:784: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:786: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:789: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:792: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:795: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:797: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:797: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:799: One line control clauses should not use braces. [whitespace/braces] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:799: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:810: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:811: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:812: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:817: num_colors is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:826: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:834: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:849: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:850: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:855: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:868: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:869: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:871: Weird number of spaces aFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/BitmapImage.cpp', u'Source/WebCore/platform/graphics/ImageSource.cpp', u'Source/WebCore/platform/graphics/ImageSource.h', u'Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp', u'Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.h', u'Source/WebCore/platform/image-decoders/ImageDecoder.cpp', u'Source/WebCore/platform/image-decoders/ImageDecoder.h', u'Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp', u'Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.h', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.h', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/WebKit.gyp', u'Source/WebKit/chromium/WebKit.gypi', u'Source/WebKit/chromium/tests/GIFImageDecoderTest.cpp', u'Source/WebKit/chromium/tests/data/broken.gif']" exit_code: 1 t line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:874: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:875: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:876: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:877: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:880: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:881: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:890: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:892: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:893: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:896: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:905: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:918: clear_code is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:945: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:945: bytes_to_decode is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:952: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:953: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:954: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:955: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:966: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:967: block_size is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:977: block_position is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:977: bytes_to_decode is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:982: bytes_left is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 170 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Peter Kasting

Comment 11 2013-02-05 16:18:43 PST

This is a really large patch. Is there no way to break it into a series of smaller patches to implement individual pieces? It looks like you're implementing multiple new behaviors, adding a couple of different structures, making no-functional-change cleanups to the GIF decoder along with the functional ones, etc. It'd be easier if each bit was its own small piece. This is especially important in the GIF decoder because it is so hairy and has been the source of so many subtle bugs in the past.

Hin-Chung Lam

Comment 12 2013-02-05 16:28:29 PST

Sorry I did this a while ago and it accumulated into a giant patch. I'll try to break it down into some logic changes.

Hin-Chung Lam

Comment 13 2013-02-05 16:47:42 PST

I'll try to break it down into these patches: 1. Abide to WebKit style. This eliminates a lot of noise in later reviews. 2. Get rid of the 'hold' pattern in the code + unit tests. 3. Use vectors instead of raw pointers. 4. Define new data structures. 5. Implement new logic.

Hin-Chung Lam

Comment 14 2013-02-05 21:47:51 PST

Created attachment 186754 [details] Patch

Hin-Chung Lam

Comment 15 2013-02-05 21:49:59 PST

Created attachment 186755 [details] Patch

WebKit Review Bot

Comment 16 2013-02-05 21:52:55 PST

Attachment 186755 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/image-decoders/gif/GIFImageDecoder.cpp', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp', u'Source/WebCore/platform/image-decoders/gif/GIFImageReader.h']" exit_code: 1 Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:62: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Hin-Chung Lam

Comment 17 2013-02-20 11:45:27 PST

Created attachment 189349 [details] hacked gif decoder for local testing For a number of subsequent changes to GIFImageDecoder/GIFImageReader it is necessary to run it over a large corpus to verify that new code produces the same results. I hacked up a command line tool to decode all frames in a GIF file and produces SHA1 hashes for this purpose. This patch is not review and in case anyone is interested in how I do testing locally.

Hin-Chung Lam

Comment 18 2013-03-19 12:52:14 PDT

This is landed woohoo!

Note You need to log in before you can comment on or make changes to this bug.