Bug 159819 - Rename FrameData to ImageFrame, move it to a separate file and use it for all ports
Summary: Rename FrameData to ImageFrame, move it to a separate file and use it for all...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
: 155444 159795 (view as bug list)
Depends on: 159795
Blocks: 155322 155498
  Show dependency treegraph
 
Reported: 2016-07-15 11:05 PDT by Said Abou-Hallawa
Modified: 2018-04-09 11:25 PDT (History)
7 users (show)

See Also:


Attachments
Patch (226.61 KB, patch)
2016-07-15 11:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (206.13 KB, patch)
2016-07-15 12:55 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (206.13 KB, patch)
2016-07-15 15:05 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (206.13 KB, patch)
2016-07-15 15:12 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch for review (34.22 KB, patch)
2016-07-15 17:33 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (97.85 KB, patch)
2016-09-14 11:13 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (97.97 KB, patch)
2016-09-14 11:44 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (100.12 KB, patch)
2016-09-14 12:02 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (100.11 KB, patch)
2016-09-14 12:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.01 MB, application/zip)
2016-09-14 13:17 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1.21 MB, application/zip)
2016-09-14 13:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.52 MB, application/zip)
2016-09-14 13:22 PDT, Build Bot
no flags Details
Patch (107.98 KB, patch)
2016-09-14 13:44 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (977.36 KB, application/zip)
2016-09-14 14:36 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews113 for mac-yosemite (1.63 MB, application/zip)
2016-09-14 14:49 PDT, Build Bot
no flags Details
Patch (112.20 KB, patch)
2016-09-14 15:38 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (1.11 MB, application/zip)
2016-09-14 16:47 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (1.51 MB, application/zip)
2016-09-14 16:47 PDT, Build Bot
no flags Details
Patch (112.33 KB, patch)
2016-09-14 17:32 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (114.64 KB, patch)
2016-09-14 18:39 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1011.74 KB, application/zip)
2016-09-14 19:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (975.54 KB, application/zip)
2016-09-14 19:44 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.55 MB, application/zip)
2016-09-14 19:48 PDT, Build Bot
no flags Details
Patch (115.33 KB, patch)
2016-09-15 09:44 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-yosemite (1.03 MB, application/zip)
2016-09-15 10:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (1.21 MB, application/zip)
2016-09-15 10:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.58 MB, application/zip)
2016-09-15 10:54 PDT, Build Bot
no flags Details
Patch (115.17 KB, patch)
2016-09-15 11:05 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (123.80 KB, patch)
2016-09-15 14:02 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (123.95 KB, patch)
2016-09-16 11:23 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (123.93 KB, patch)
2016-09-20 09:57 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2016-07-15 11:05:14 PDT
Currently ImageFrame is a subset of ImageFrameData so the later can replace the former.
Comment 1 Said Abou-Hallawa 2016-07-15 11:06:29 PDT
Created attachment 283771 [details]
Patch
Comment 2 Said Abou-Hallawa 2016-07-15 12:55:56 PDT
Created attachment 283784 [details]
Patch
Comment 3 Said Abou-Hallawa 2016-07-15 15:05:31 PDT
Created attachment 283797 [details]
Patch
Comment 4 Said Abou-Hallawa 2016-07-15 15:12:27 PDT
Created attachment 283799 [details]
Patch
Comment 5 Said Abou-Hallawa 2016-07-15 17:33:18 PDT
Created attachment 283827 [details]
Patch for review
Comment 6 Darin Adler 2016-07-16 09:08:18 PDT
Comment on attachment 283827 [details]
Patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=283827&action=review

Refactoring looks OK.

> Source/WebCore/platform/graphics/ImageFrame.cpp:37
> +#include "Color.h"
> +
> +#if USE(CG)
> +#include "ImageDecoderCG.h"
> +#else
> +#include "ImageDecoder.h"
> +#endif
> +
> +#include "PlatformImage.h"

Normally we put all unconditional includes first, and then includes afterward. So PlatformImage.h should be paragraphed with Color.h.

> Source/WebCore/platform/graphics/ImageFrame.cpp:50
> +    m_size = PlatformImage(m_image).size();
> +    m_hasAlpha = PlatformImage(m_image).hasAlpha();

I know this code is just being moved, but I noticed this:

Is the construction of a PlatformImage guaranteed to be inexpensive? If not, then I suggest we put it into a local variable so we don’t have to do it twice.

> Source/WebCore/platform/graphics/ImageFrame.cpp:97
> +void ImageFrame::clearSubImages()

I know this code is just being moved, but "subimages" is a single coined word, not two words "sub images", so it should be spelled Subimages, not SubImages.
Comment 7 Said Abou-Hallawa 2016-09-14 11:13:29 PDT
Created attachment 288833 [details]
Patch
Comment 8 Said Abou-Hallawa 2016-09-14 11:44:19 PDT
Created attachment 288840 [details]
Patch
Comment 9 Simon Fraser (smfr) 2016-09-14 11:50:24 PDT
Comment on attachment 288840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288840&action=review

> Source/WebCore/platform/graphics/ImageFrame.h:40
> +using SubsamplingLevel = int;

Normally we typedef.

> Source/WebCore/platform/graphics/ImageFrame.h:42
> +enum : SubsamplingLevel {

enum class?

> Source/WebCore/platform/graphics/ImageFrame.h:49
> +// The animation cycles infinitely if repetitionCount() is AnimationLoopInfinite.

No need for this comment; the values are self-explanatory.

> Source/WebCore/platform/graphics/ImageFrame.h:50
> +using AnimationLoop = int;

typedef?

> Source/WebCore/platform/graphics/ImageFrame.h:52
> +enum : AnimationLoop {

enum class?
Comment 10 Said Abou-Hallawa 2016-09-14 12:02:32 PDT
Created attachment 288843 [details]
Patch
Comment 11 Said Abou-Hallawa 2016-09-14 12:06:50 PDT
Created attachment 288844 [details]
Patch
Comment 12 Build Bot 2016-09-14 13:17:00 PDT
Comment on attachment 288844 [details]
Patch

Attachment 288844 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2073837

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 13 Build Bot 2016-09-14 13:17:03 PDT
Created attachment 288847 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 14 Build Bot 2016-09-14 13:21:35 PDT
Comment on attachment 288844 [details]
Patch

Attachment 288844 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2073846

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 15 Build Bot 2016-09-14 13:21:38 PDT
Created attachment 288849 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 16 Build Bot 2016-09-14 13:22:09 PDT
Comment on attachment 288844 [details]
Patch

Attachment 288844 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2073829

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 17 Build Bot 2016-09-14 13:22:12 PDT
Created attachment 288850 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 18 Said Abou-Hallawa 2016-09-14 13:44:34 PDT
Created attachment 288853 [details]
Patch
Comment 19 Build Bot 2016-09-14 14:36:29 PDT
Comment on attachment 288853 [details]
Patch

Attachment 288853 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2074197

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 20 Build Bot 2016-09-14 14:36:33 PDT
Created attachment 288860 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 21 Build Bot 2016-09-14 14:49:18 PDT
Comment on attachment 288853 [details]
Patch

Attachment 288853 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2074198

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 22 Build Bot 2016-09-14 14:49:22 PDT
Created attachment 288865 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 23 Said Abou-Hallawa 2016-09-14 15:38:51 PDT
Created attachment 288874 [details]
Patch
Comment 24 Build Bot 2016-09-14 16:47:05 PDT
Comment on attachment 288874 [details]
Patch

Attachment 288874 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2075045

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 25 Build Bot 2016-09-14 16:47:08 PDT
Created attachment 288894 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 26 Build Bot 2016-09-14 16:47:36 PDT
Comment on attachment 288874 [details]
Patch

Attachment 288874 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2075007

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 27 Build Bot 2016-09-14 16:47:39 PDT
Created attachment 288896 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 28 Said Abou-Hallawa 2016-09-14 17:32:41 PDT
Created attachment 288903 [details]
Patch
Comment 29 Said Abou-Hallawa 2016-09-14 18:39:28 PDT
Created attachment 288909 [details]
Patch
Comment 30 Build Bot 2016-09-14 19:40:14 PDT
Comment on attachment 288909 [details]
Patch

Attachment 288909 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2076243

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 31 Build Bot 2016-09-14 19:40:19 PDT
Created attachment 288915 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 32 Build Bot 2016-09-14 19:44:25 PDT
Comment on attachment 288909 [details]
Patch

Attachment 288909 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2076249

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 33 Build Bot 2016-09-14 19:44:29 PDT
Created attachment 288916 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 34 Build Bot 2016-09-14 19:47:59 PDT
Comment on attachment 288909 [details]
Patch

Attachment 288909 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2076250

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 35 Build Bot 2016-09-14 19:48:02 PDT
Created attachment 288918 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 36 Said Abou-Hallawa 2016-09-15 09:44:26 PDT
Created attachment 288958 [details]
Patch
Comment 37 Build Bot 2016-09-15 10:37:43 PDT
Comment on attachment 288958 [details]
Patch

Attachment 288958 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2080400

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 38 Build Bot 2016-09-15 10:37:46 PDT
Created attachment 288969 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 39 Build Bot 2016-09-15 10:41:06 PDT
Comment on attachment 288958 [details]
Patch

Attachment 288958 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2080405

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 40 Build Bot 2016-09-15 10:41:09 PDT
Created attachment 288971 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 41 Build Bot 2016-09-15 10:54:27 PDT
Comment on attachment 288958 [details]
Patch

Attachment 288958 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2080439

New failing tests:
http/tests/misc/slow-loading-animated-image.html
Comment 42 Build Bot 2016-09-15 10:54:31 PDT
Created attachment 288972 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 43 Said Abou-Hallawa 2016-09-15 11:05:06 PDT
Created attachment 288975 [details]
Patch
Comment 44 Said Abou-Hallawa 2016-09-15 14:02:06 PDT
Created attachment 288997 [details]
Patch
Comment 45 Said Abou-Hallawa 2016-09-15 14:28:31 PDT
*** Bug 159795 has been marked as a duplicate of this bug. ***
Comment 46 Said Abou-Hallawa 2016-09-15 14:29:19 PDT
*** Bug 155444 has been marked as a duplicate of this bug. ***
Comment 47 Said Abou-Hallawa 2016-09-15 14:49:42 PDT
Comment on attachment 288840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288840&action=review

>> Source/WebCore/platform/graphics/ImageFrame.h:42
>> +enum : SubsamplingLevel {
> 
> enum class?

Done. And the using statement is removed since SubsamplingLevel can be strongly typed.

>> Source/WebCore/platform/graphics/ImageFrame.h:49
>> +// The animation cycles infinitely if repetitionCount() is AnimationLoopInfinite.
> 
> No need for this comment; the values are self-explanatory.

Removed.

>> Source/WebCore/platform/graphics/ImageFrame.h:50
>> +using AnimationLoop = int;
> 
> typedef?

Done.

>> Source/WebCore/platform/graphics/ImageFrame.h:52
>> +enum : AnimationLoop {
> 
> enum class?

No. Because AnimationLoop is not strongly typed. In addition to the values below, it can be any other unsigned value.
Comment 48 Tim Horton 2016-09-15 19:04:35 PDT
Comment on attachment 288997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288997&action=review

> Source/WebCore/platform/graphics/ImageFrame.h:132
> +    void fillMetaData(ImageDecoder&, size_t, SubsamplingLevel, bool animating);

Please pick one of Metadata or MetaData (probably Metadata), but not both.

> Source/WebCore/platform/graphics/ImageSource.h:-54
> -// Right now GIFs are the only recognized image format that supports animation.
> -// The animation system and the constants below are designed with this in mind.
> -// GIFs have an optional 16-bit unsigned loop count that describes how an
> -// animated GIF should be cycled.  If the loop count is absent, the animation
> -// cycles once; if it is 0, the animation cycles infinitely; otherwise the
> -// animation plays n + 1 cycles (where n is the specified loop count).  If the
> -// GIF decoder defaults to cAnimationLoopOnce in the absence of any loop count
> -// and translates an explicit "0" loop count to cAnimationLoopInfinite, then we
> -// get a couple of nice side effects:

Are you sure you got all of these behaviors right? Do we have tests? Can we? Can you at least manually test?
Comment 49 Tim Horton 2016-09-15 19:07:03 PDT
(In reply to comment #47)
> Comment on attachment 288840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288840&action=review
> 
> >> Source/WebCore/platform/graphics/ImageFrame.h:42
> >> +enum : SubsamplingLevel {
> > 
> > enum class?
> 
> Done. And the using statement is removed since SubsamplingLevel can be
> strongly typed.
> 
> >> Source/WebCore/platform/graphics/ImageFrame.h:49
> >> +// The animation cycles infinitely if repetitionCount() is AnimationLoopInfinite.
> > 
> > No need for this comment; the values are self-explanatory.
> 
> Removed.
> 
> >> Source/WebCore/platform/graphics/ImageFrame.h:50
> >> +using AnimationLoop = int;
> > 
> > typedef?
> 
> Done.
> 
> >> Source/WebCore/platform/graphics/ImageFrame.h:52
> >> +enum : AnimationLoop {
> > 
> > enum class?
> 
> No. Because AnimationLoop is not strongly typed. In addition to the values
> below, it can be any other unsigned value.

I think it should be called AnimationLoopCount or just LoopCount
Comment 50 Said Abou-Hallawa 2016-09-16 11:06:07 PDT
Comment on attachment 288997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288997&action=review

>> Source/WebCore/platform/graphics/ImageFrame.h:132
>> +    void fillMetaData(ImageDecoder&, size_t, SubsamplingLevel, bool animating);
> 
> Please pick one of Metadata or MetaData (probably Metadata), but not both.

Done.

>> Source/WebCore/platform/graphics/ImageSource.h:-54
>> -// get a couple of nice side effects:
> 
> Are you sure you got all of these behaviors right? Do we have tests? Can we? Can you at least manually test?

This comment is not correct. Here is the calculation of the repetition count according to ImageDecoder::repetitionCount() in ImageDecoderCG.cpp:

Not a gif or a png image                    repetitionCount = 0         (AnimationLoopNone)
LoopCount property is missing               repetitionCount = 1         (AnimationLoopOnce)
LoopCount property == 0                     repetitionCount = Infinite  (AnimationLoopInfinite)
LoopCount property == n (including 1)       repetitionCount = n         (including AnimationLoopOnce)

I do not think also the values of the cAnimation* have any nice side effects at least in the current state of the code. Yes they do not conflict with real loop count values. But there is no need for that except for AnimationLoopInfinite which should be a special case.

cAnimationNone was chosen to be -2. I changed that to be 0 so I could have the following initializations in BitmapImage.h
    AnimationLoop m_repetitionCount { AnimationLoopNone };
    AnimationLoop m_repetitionsComplete { AnimationLoopNone };

Instead of 

    int m_repetitionCount { cAnimationNone };
    int m_repetitionsComplete { 0 };

m_repetitionCount represents how many loops the image can animate. And m_repetitionsComplete represents how many animation loops have finished. The first one is initialized with -1 while the second initialized with zero. I think it makes more sense to start both of them with 0 (or AnimationLoopNone).

cAnimationOnce was chosen to be 0. I changed that to be 1 so I have can have the following condition in BitmapImage::startAnimation()

    bool BitmapImage::shouldAnimate()
    {
        return repetitionCount(false) && !m_animationFinished && imageObserver();
    }

Instead of

    bool BitmapImage::shouldAnimate()
    {
        return (repetitionCount(false) != AnimationLoopNone && !m_animationFinished && imageObserver());
    }

I reviewed all the use cases of these constants and I found they were special cases when checking the value of the repetitionCount. There is no math which depends on these values. So there is no harm in changing their values to reflect their their actual meanings.
Comment 51 Said Abou-Hallawa 2016-09-16 11:23:59 PDT
Created attachment 289081 [details]
Patch
Comment 52 Said Abou-Hallawa 2016-09-16 11:26:24 PDT
Comment on attachment 288840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288840&action=review

>>>> Source/WebCore/platform/graphics/ImageFrame.h:42
>>>> +enum : SubsamplingLevel {
>>> 
>>> enum class?
>> 
>> Done. And the using statement is removed since SubsamplingLevel can be strongly typed.
> 
> I think it should be called AnimationLoopCount or just LoopCount

I changed it to be RepetitionCount because the function that calculates it is named repetitionCount().
Comment 53 Said Abou-Hallawa 2016-09-16 12:55:02 PDT
Comment on attachment 288997 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288997&action=review

>>> Source/WebCore/platform/graphics/ImageSource.h:-54
>>> -// get a couple of nice side effects:
>> 
>> Are you sure you got all of these behaviors right? Do we have tests? Can we? Can you at least manually test?
> 
> This comment is not correct. Here is the calculation of the repetition count according to ImageDecoder::repetitionCount() in ImageDecoderCG.cpp:
> 
> Not a gif or a png image                    repetitionCount = 0         (AnimationLoopNone)
> LoopCount property is missing               repetitionCount = 1         (AnimationLoopOnce)
> LoopCount property == 0                     repetitionCount = Infinite  (AnimationLoopInfinite)
> LoopCount property == n (including 1)       repetitionCount = n         (including AnimationLoopOnce)
> 
> I do not think also the values of the cAnimation* have any nice side effects at least in the current state of the code. Yes they do not conflict with real loop count values. But there is no need for that except for AnimationLoopInfinite which should be a special case.
> 
> cAnimationNone was chosen to be -2. I changed that to be 0 so I could have the following initializations in BitmapImage.h
>     AnimationLoop m_repetitionCount { AnimationLoopNone };
>     AnimationLoop m_repetitionsComplete { AnimationLoopNone };
> 
> Instead of 
> 
>     int m_repetitionCount { cAnimationNone };
>     int m_repetitionsComplete { 0 };
> 
> m_repetitionCount represents how many loops the image can animate. And m_repetitionsComplete represents how many animation loops have finished. The first one is initialized with -1 while the second initialized with zero. I think it makes more sense to start both of them with 0 (or AnimationLoopNone).
> 
> cAnimationOnce was chosen to be 0. I changed that to be 1 so I have can have the following condition in BitmapImage::startAnimation()
> 
>     bool BitmapImage::shouldAnimate()
>     {
>         return repetitionCount(false) && !m_animationFinished && imageObserver();
>     }
> 
> Instead of
> 
>     bool BitmapImage::shouldAnimate()
>     {
>         return (repetitionCount(false) != AnimationLoopNone && !m_animationFinished && imageObserver());
>     }
> 
> I reviewed all the use cases of these constants and I found they were special cases when checking the value of the repetitionCount. There is no math which depends on these values. So there is no harm in changing their values to reflect their their actual meanings.

This is already covered by many tests. For example fast/images/gif-loop-count.html has an image with a missing repetition key so ImageDecoder::repetitionCount() returns RepetitionCountOnce. And http/tests/misc/slow-loading-animated-image.html has as image with repetition key equals to zero so ImageDecoder::repetitionCount() returns RepetitionCountInfinite.
Comment 54 Simon Fraser (smfr) 2016-09-19 14:52:42 PDT
Comment on attachment 289081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289081&action=review

> Source/WebCore/platform/graphics/BitmapImage.cpp:108
> +        frameBytesCleared += m_frames[i].clear(ImageFrame::Caching::Metadata);

It's confusing that clear(ImageFrame::Caching::Metadata) does NOT clear metadata. I would expect to read clear(KeepMetadata) or clear(FrameData)

> Source/WebCore/platform/graphics/BitmapImage.cpp:168
> +        NativeImagePtr image = m_source.createFrameImageAtIndex(index, subsamplingLevel);
> +        m_frames[index].initialize(WTFMove(image), *m_source.decoder(), index, subsamplingLevel, repetitionCount(false));

It seems a little odd here to get the image from the ImageSource, and then pass it to initialize(), but also pass the source's decoder. It's also sad that the native image type shows up in BitmapImage.cpp.

> Source/WebCore/platform/graphics/BitmapImage.cpp:272
> -        if (int bytes = m_frames[m_frames.size() - 1].clear(true)) {
> +        if (int bytes = m_frames[m_frames.size() - 1].clear(ImageFrame::Caching::Empty)) {

I don't know what's being cleared here. Maybe using this enum for clear() is just confusing.
Comment 55 Said Abou-Hallawa 2016-09-20 09:57:01 PDT
Created attachment 289365 [details]
Patch
Comment 56 Said Abou-Hallawa 2016-09-20 10:03:01 PDT
Comment on attachment 289081 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=289081&action=review

>> Source/WebCore/platform/graphics/BitmapImage.cpp:108
>> +        frameBytesCleared += m_frames[i].clear(ImageFrame::Caching::Metadata);
> 
> It's confusing that clear(ImageFrame::Caching::Metadata) does NOT clear metadata. I would expect to read clear(KeepMetadata) or clear(FrameData)

The caching argument was removed. Calling clear(ImageFrame::Caching::Metadata) means clearing the NativeImagePtr only but keeping the metadata. So I created another function and I named it ImageFrame::clearImage() which will be called here instead.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:168
>> +        m_frames[index].initialize(WTFMove(image), *m_source.decoder(), index, subsamplingLevel, repetitionCount(false));
> 
> It seems a little odd here to get the image from the ImageSource, and then pass it to initialize(), but also pass the source's decoder. It's also sad that the native image type shows up in BitmapImage.cpp.

This will be fixed in the fix of https://bugs.webkit.org/show_bug.cgi?id=155498 where caching the frames will be moved to the ImageSource and the ImageFrame will be hidden from BitmapImage.

>> Source/WebCore/platform/graphics/BitmapImage.cpp:272
>> +        if (int bytes = m_frames[m_frames.size() - 1].clear(ImageFrame::Caching::Empty)) {
> 
> I don't know what's being cleared here. Maybe using this enum for clear() is just confusing.

The caching argument was removed.
Comment 57 WebKit Commit Bot 2016-09-20 11:26:55 PDT
Comment on attachment 289365 [details]
Patch

Clearing flags on attachment: 289365

Committed r206156: <http://trac.webkit.org/changeset/206156>
Comment 58 WebKit Commit Bot 2016-09-20 11:27:03 PDT
All reviewed patches have been landed.  Closing bug.