There's going to be a lot of changes in GIFImageReader.cpp. This file doesn't use WebKit style and will add a lot of noise in future reviews. Fix the style violations.
Created attachment 186756 [details] Patch
senorblanco: can you review this? There's no change in logic at all. I had to fix the style violations first because my subsequent changes touch a significant portion of GIFImageReader and style checks add a lot of noise to review. I ran layout tests and ran this through a random set of 50K GIF images on the web to make sure it is bit-exact as before.
Attachment 186756 [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.
Comment on attachment 186756 [details] Patch Attachment 186756 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16393094
senorblanco, jamesr: thoughts? It seems overkill to fix the entire file but later reviews will have a lot less noise from style-check violations.
Comment on attachment 186756 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186756&action=review Generally rubber-stamp LGTM > Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:95 > + } \ Nit: Combine next line with this one? > Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:92 > + int ipass; // Interlace pass; Ranges 1-4 if interlaced. Nit: Some members, like stackp and ipass, look like maybe they should be camelCase? > Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:166 > +class GIFImageReader { Why is GIFFrameContext a struct with all-public members and no accessors, but GIFImageReader is a class with private members and accessors?
Comment on attachment 186756 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186756&action=review >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:95 >> + } \ > > Nit: Combine next line with this one? done. >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:92 >> + int ipass; // Interlace pass; Ranges 1-4 if interlaced. > > Nit: Some members, like stackp and ipass, look like maybe they should be camelCase? Those I just leave it. I'll remove / modify them in future patches anyway. >> Source/WebCore/platform/image-decoders/gif/GIFImageReader.h:166 >> +class GIFImageReader { > > Why is GIFFrameContext a struct with all-public members and no accessors, but GIFImageReader is a class with private members and accessors? I added some public methods to this class, hence I changed it from struct to class. GIFFrameContext is still a struct because we access the members directly (it will change though).
Created attachment 187644 [details] Patch
Attachment 187644 [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.
Comment on attachment 187644 [details] Patch OK. r=me
Created attachment 187672 [details] Patch for landing
Comment on attachment 187672 [details] Patch for landing Clearing flags on attachment: 187672 Committed r142528: <http://trac.webkit.org/changeset/142528>
All reviewed patches have been landed. Closing bug.