WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
159819
Rename FrameData to ImageFrame, move it to a separate file and use it for all ports
https://bugs.webkit.org/show_bug.cgi?id=159819
Summary
Rename FrameData to ImageFrame, move it to a separate file and use it for all...
Said Abou-Hallawa
Reported
2016-07-15 11:05:14 PDT
Currently ImageFrame is a subset of ImageFrameData so the later can replace the former.
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
Show Obsolete
(30)
View All
Add attachment
proposed patch, testcase, etc.
Said Abou-Hallawa
Comment 1
2016-07-15 11:06:29 PDT
Created
attachment 283771
[details]
Patch
Said Abou-Hallawa
Comment 2
2016-07-15 12:55:56 PDT
Created
attachment 283784
[details]
Patch
Said Abou-Hallawa
Comment 3
2016-07-15 15:05:31 PDT
Created
attachment 283797
[details]
Patch
Said Abou-Hallawa
Comment 4
2016-07-15 15:12:27 PDT
Created
attachment 283799
[details]
Patch
Said Abou-Hallawa
Comment 5
2016-07-15 17:33:18 PDT
Created
attachment 283827
[details]
Patch for review
Darin Adler
Comment 6
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.
Said Abou-Hallawa
Comment 7
2016-09-14 11:13:29 PDT
Created
attachment 288833
[details]
Patch
Said Abou-Hallawa
Comment 8
2016-09-14 11:44:19 PDT
Created
attachment 288840
[details]
Patch
Simon Fraser (smfr)
Comment 9
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?
Said Abou-Hallawa
Comment 10
2016-09-14 12:02:32 PDT
Created
attachment 288843
[details]
Patch
Said Abou-Hallawa
Comment 11
2016-09-14 12:06:50 PDT
Created
attachment 288844
[details]
Patch
Build Bot
Comment 12
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
Build Bot
Comment 13
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
Build Bot
Comment 14
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
Build Bot
Comment 15
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
Build Bot
Comment 16
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
Build Bot
Comment 17
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
Said Abou-Hallawa
Comment 18
2016-09-14 13:44:34 PDT
Created
attachment 288853
[details]
Patch
Build Bot
Comment 19
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
Build Bot
Comment 20
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
Build Bot
Comment 21
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
Build Bot
Comment 22
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
Said Abou-Hallawa
Comment 23
2016-09-14 15:38:51 PDT
Created
attachment 288874
[details]
Patch
Build Bot
Comment 24
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
Build Bot
Comment 25
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
Build Bot
Comment 26
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
Build Bot
Comment 27
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
Said Abou-Hallawa
Comment 28
2016-09-14 17:32:41 PDT
Created
attachment 288903
[details]
Patch
Said Abou-Hallawa
Comment 29
2016-09-14 18:39:28 PDT
Created
attachment 288909
[details]
Patch
Build Bot
Comment 30
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
Build Bot
Comment 31
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
Build Bot
Comment 32
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
Build Bot
Comment 33
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
Build Bot
Comment 34
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
Build Bot
Comment 35
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
Said Abou-Hallawa
Comment 36
2016-09-15 09:44:26 PDT
Created
attachment 288958
[details]
Patch
Build Bot
Comment 37
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
Build Bot
Comment 38
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
Build Bot
Comment 39
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
Build Bot
Comment 40
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
Build Bot
Comment 41
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
Build Bot
Comment 42
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
Said Abou-Hallawa
Comment 43
2016-09-15 11:05:06 PDT
Created
attachment 288975
[details]
Patch
Said Abou-Hallawa
Comment 44
2016-09-15 14:02:06 PDT
Created
attachment 288997
[details]
Patch
Said Abou-Hallawa
Comment 45
2016-09-15 14:28:31 PDT
***
Bug 159795
has been marked as a duplicate of this bug. ***
Said Abou-Hallawa
Comment 46
2016-09-15 14:29:19 PDT
***
Bug 155444
has been marked as a duplicate of this bug. ***
Said Abou-Hallawa
Comment 47
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.
Tim Horton
Comment 48
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?
Tim Horton
Comment 49
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
Said Abou-Hallawa
Comment 50
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.
Said Abou-Hallawa
Comment 51
2016-09-16 11:23:59 PDT
Created
attachment 289081
[details]
Patch
Said Abou-Hallawa
Comment 52
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().
Said Abou-Hallawa
Comment 53
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.
Simon Fraser (smfr)
Comment 54
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.
Said Abou-Hallawa
Comment 55
2016-09-20 09:57:01 PDT
Created
attachment 289365
[details]
Patch
Said Abou-Hallawa
Comment 56
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.
WebKit Commit Bot
Comment 57
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
>
WebKit Commit Bot
Comment 58
2016-09-20 11:27:03 PDT
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.
Top of Page
Format For Printing
XML
Clone This Bug