Bug 109007

Summary: Fix code style violations in GIFImageReader.{cc|h}
Product: WebKit Reporter: Hin-Chung Lam <hclam>
Component: ImagesAssignee: Hin-Chung Lam <hclam>
Status: RESOLVED FIXED    
Severity: Normal CC: jamesr, nduca, pkasting, qinmin, senorblanco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 98098    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Hin-Chung Lam
Reported 2013-02-05 21:52:24 PST
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.
Attachments
Patch (77.14 KB, patch)
2013-02-05 21:56 PST, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Patch (77.14 KB, patch)
2013-02-11 13:04 PST, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Patch for landing (77.14 KB, patch)
2013-02-11 14:18 PST, Hin-Chung Lam 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Hin-Chung Lam
Comment 1 2013-02-05 21:56:12 PST
Created attachment 186756 [details] Patch
Hin-Chung Lam
Comment 2 2013-02-05 21:57:55 PST
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.
WebKit Review Bot
Comment 3 2013-02-05 22:00:04 PST
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.
Build Bot
Comment 4 2013-02-06 01:01:00 PST
Comment on attachment 186756 [details] Patch Attachment 186756 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16393094
Hin-Chung Lam
Comment 5 2013-02-08 10:36:56 PST
senorblanco, jamesr: thoughts? It seems overkill to fix the entire file but later reviews will have a lot less noise from style-check violations.
Peter Kasting
Comment 6 2013-02-11 12:25:16 PST
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?
Hin-Chung Lam
Comment 7 2013-02-11 13:01:29 PST
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).
Hin-Chung Lam
Comment 8 2013-02-11 13:04:53 PST
Created attachment 187644 [details] Patch
WebKit Review Bot
Comment 9 2013-02-11 13:10:51 PST
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.
Stephen White
Comment 10 2013-02-11 14:07:02 PST
Comment on attachment 187644 [details] Patch OK. r=me
Hin-Chung Lam
Comment 11 2013-02-11 14:18:59 PST
Created attachment 187672 [details] Patch for landing
WebKit Review Bot
Comment 12 2013-02-11 15:23:10 PST
Comment on attachment 187672 [details] Patch for landing Clearing flags on attachment: 187672 Committed r142528: <http://trac.webkit.org/changeset/142528>
WebKit Review Bot
Comment 13 2013-02-11 15:23:14 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.