Bug 109007 – Fix code style violations in GIFImageReader.{cc|h}

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Created attachment 186756 [details]
Patch

[Hin-Chung Lam]

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]

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 on attachment 186756 [details]
Patch

Attachment 186756 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16393094

[Hin-Chung Lam]

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 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 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]

Created attachment 187644 [details]
Patch

[WebKit Review Bot]

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 on attachment 187644 [details]
Patch

OK.  r=me

[Hin-Chung Lam]

Created attachment 187672 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 187672 [details]
Patch for landing

Clearing flags on attachment: 187672

Committed r142528: <http://trac.webkit.org/changeset/142528>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.