Summary: | GIFs loop one time too few | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peter Kasting <pkasting> | ||||||||||
Component: | Images | Assignee: | Peter Kasting <pkasting> | ||||||||||
Status: | CLOSED FIXED | ||||||||||||
Severity: | Normal | CC: | hausmann, hyatt, jay.tucker, mrowe, webkit.review.bot, zecke | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Peter Kasting
2010-05-27 16:53:00 PDT
Oh hey wait. Since the fix is in BitmapImage.cpp, it will actually fix things for both Safari and Chrome at the same time. OK, taking. Argh, this is really confusing. Reading http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp more: -1 means "loop infinitely". 0 means "loop infinitely". anything else means "show n + 1 total times". The obvious question then is: how would someone specify "loop once"? The answer is that you can't do it from this value, you just omit the Netscape extension block entirely, and the default is then to loop once. Created attachment 57284 [details]
patch v1
This should fix Safari, Chrome, and all other consumers besides Qt, which was already correct.
Attachment 57284 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/BitmapImage.cpp:283: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:692: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
WebCore/platform/graphics/cg/ImageSourceCG.cpp:213: 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: 3 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 57285 [details]
patch v1.1
Fixes the style errors in BitmapImage.cpp and ImageSourceCG.cpp, but not in GIFImageReader.cpp as that file is not in WebKit style but rather is copied from Mozilla.
Attachment 57285 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:692: 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 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 57285 [details]
patch v1.1
Great catch, really glad you are fixing it.
Why get rid of cAnimationLoopOnce? Changing its value to 0 would make the patch smaller, and I think having a named constant for it is still helpful even if it's only a particular special case of the general rule of "loop once more than this number". I think that changing from the constant to the magic number 0 makes the code less readable.
Please take a crack at doing this without removing cAnimationLoopOnce. The resulting patch should be a bit smaller and it will also be clearer what you are and are not changing.
Comment on attachment 57285 [details]
patch v1.1
We also need a test case for this bug fix. If we can't come up with a DumpRenderTree test, we need a manual test.
(In reply to comment #7) > Why get rid of cAnimationLoopOnce? Changing its value to 0 would make the patch smaller, and I think having a named constant for it is still helpful even if it's only a particular special case of the general rule of "loop once more than this number". I think that changing from the constant to the magic number 0 makes the code less readable. You are right that that would make the patch smaller. My reason for doing it was that I felt uncomfortable with having a named constant for a value that was handled identically to all the unnamed constants -- it seemed like it would cause readers to believe that had to be handled specially, which wasn't true. OTOH, it _is_ something of a magic number since the GIF extension can't represent that value, so in that way it makes sense. I'll change it. Manual tests go in WebCore/manual-tests/? Created attachment 57371 [details] patch v2 This restores cAnimationLoopOnce, adds more comments, and fixes + makes available for everyone an existing Qt manual test on this. I am concerned that, based on the comments on bug 36818, Qt may still be buggy with this patch. If that's true, the fix may need to happen in the Qt image decoders themselves. I will leave it to a Qt developer to address that (a few of whom are CCed on this bug). Attachment 57371 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:693: 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 in 8 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Fixed in r60375. Revision r60375 cherry-picked into qtwebkit-2.0 with commit 6a7801db0fd3f747a0306f2847d5199823cc0ed7 |