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.
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
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
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
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
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
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
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
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
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
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
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
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
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 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 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?
(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 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 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 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 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 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.
2016-07-15 11:06 PDT, Said Abou-Hallawa
2016-07-15 12:55 PDT, Said Abou-Hallawa
2016-07-15 15:05 PDT, Said Abou-Hallawa
2016-07-15 15:12 PDT, Said Abou-Hallawa
2016-07-15 17:33 PDT, Said Abou-Hallawa
2016-09-14 11:13 PDT, Said Abou-Hallawa
2016-09-14 11:44 PDT, Said Abou-Hallawa
2016-09-14 12:02 PDT, Said Abou-Hallawa
2016-09-14 12:06 PDT, Said Abou-Hallawa
2016-09-14 13:17 PDT, Build Bot
2016-09-14 13:21 PDT, Build Bot
2016-09-14 13:22 PDT, Build Bot
2016-09-14 13:44 PDT, Said Abou-Hallawa
2016-09-14 14:36 PDT, Build Bot
2016-09-14 14:49 PDT, Build Bot
2016-09-14 15:38 PDT, Said Abou-Hallawa
2016-09-14 16:47 PDT, Build Bot
2016-09-14 16:47 PDT, Build Bot
2016-09-14 17:32 PDT, Said Abou-Hallawa
2016-09-14 18:39 PDT, Said Abou-Hallawa
2016-09-14 19:40 PDT, Build Bot
2016-09-14 19:44 PDT, Build Bot
2016-09-14 19:48 PDT, Build Bot
2016-09-15 09:44 PDT, Said Abou-Hallawa
2016-09-15 10:37 PDT, Build Bot
2016-09-15 10:41 PDT, Build Bot
2016-09-15 10:54 PDT, Build Bot
2016-09-15 11:05 PDT, Said Abou-Hallawa
2016-09-15 14:02 PDT, Said Abou-Hallawa
2016-09-16 11:23 PDT, Said Abou-Hallawa
2016-09-20 09:57 PDT, Said Abou-Hallawa