Created attachment 186974 [details] Patch

After this patch, I will refactor some static methods (e.g. normalizedAnimationValue, normalizedAnimationValueForFillsForwards ..) as GraphicsLayerAnimation::Animation has them.

Comment on attachment 186974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186974&action=review Hmm... looking at what it takes to move away from WebCore::Animation I'm not sure it's very succinct. I originally thought it would be. Is there any other value in doing this? Seems like the code duplication makes it to not be the best idea :) > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:35 > + class Animation { > + public: I don't see the point in the inner class; why not just have those be members of GraphicsLayerAnimation? > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:45 > + enum AnimationDirection { > + AnimationDirectionNormal, > + AnimationDirectionAlternate, > + AnimationDirectionReverse, > + AnimationDirectionAlternateReverse > + }; I think we should move AnimationDirection to

(In reply to comment #3) > (From update of attachment 186974 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186974&action=review > > Hmm... looking at what it takes to move away from WebCore::Animation I'm not sure it's very succinct. I originally thought it would be. > Is there any other value in doing this? Seems like the code duplication makes it to not be the best idea :) Thanks for your review, noam. I tried to remove unnecessary data passing. After CoordinatedGraphicsLayer passes WebCore::Animation to its GraphicsLayerAnimation, only five members(m_direction, m_duration, m_iterationCount, m_fillMode and m_timingFunction) are ever used. You can see encoding/decoding of WebCore::Animation in CoordinatedGraphicsArgumentCoders.cpp. > > > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:35 > > + class Animation { > > + public: > > I don't see the point in the inner class; why not just have those be members of GraphicsLayerAnimation? > I wanted to show that the inner class is originated from Animation, but I don't want it to be mixed up with GraphicsLayerAnimation. Please suggest a better name for this class if you are okay with the inner class. :) > > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:45 > > + enum AnimationDirection { > > + AnimationDirectionNormal, > > + AnimationDirectionAlternate, > > + AnimationDirectionReverse, > > + AnimationDirectionAlternateReverse > > + }; > > I think we should move AnimationDirection to How about RenderStyleConstants.h?

Comment on attachment 186974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186974&action=review OK, let's move forward but remove the inner class, move AnimationDirection to RenderStyleConstants (need to see if dhyatt/smfr are ok with that), and use setters instead of a huge constructor. >>> Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:35 >>> + public: >> >> I don't see the point in the inner class; why not just have those be members of GraphicsLayerAnimation? > > I wanted to show that the inner class is originated from Animation, but I don't want it to be mixed up with GraphicsLayerAnimation. > Please suggest a better name for this class if you are okay with the inner class. :) I don't see why it matters that it came from WebCore::Animation. Maybe in the future we'd want to set those directly... Let's get rid of the inner class :) >>> Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:45 >>> + }; >> >> I think we should move AnimationDirection to > > How about RenderStyleConstants.h? Yes, completed my sentence :) > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:754 > - animation = GraphicsLayerAnimation(name, keyframes, boxSize, animationObject.get(), startTime, listsMatch); > + animation = GraphicsLayerAnimation(name, keyframes, boxSize, animationObject, startTime, listsMatch); I think it would be better here if instead of passing the "animationObject" in the constructor, we pass that information as set*** functions, e.g. setFillMode().

Thanks for your reply! > OK, let's move forward but remove the inner class, move AnimationDirection to RenderStyleConstants (need to see if dhyatt/smfr are ok with that), and use setters instead of a huge constructor. Got it :) > >>> Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:35 > >>> + public: > >> > >> I don't see the point in the inner class; why not just have those be members of GraphicsLayerAnimation? > > > > I wanted to show that the inner class is originated from Animation, but I don't want it to be mixed up with GraphicsLayerAnimation. > > Please suggest a better name for this class if you are okay with the inner class. :) > > I don't see why it matters that it came from WebCore::Animation. Maybe in the future we'd want to set those directly... > Let's get rid of the inner class :) Firstly, I'm not good at writing english, so I was late when you ask me on IRC. Sorry for waiting me. :( I think it again, there are no reason to know it came from WebCore::Animation. I agree with you. > > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:754 > > - animation = GraphicsLayerAnimation(name, keyframes, boxSize, animationObject.get(), startTime, listsMatch); > > + animation = GraphicsLayerAnimation(name, keyframes, boxSize, animationObject, startTime, listsMatch); > > I think it would be better here if instead of passing the "animationObject" in the constructor, we pass that information as set*** functions, e.g. setFillMode(). Got it.

> > OK, let's move forward but remove the inner class, move AnimationDirection to RenderStyleConstants (need to see if dhyatt/smfr are ok with that), and use setters instead of a huge constructor. > > Got it :) > Sorry, I didn't catch your last sentence. ("use setters instead of a huge constructor.") If we use setters instead of a some huge constructor, GraphicsLayerAnimation must have many setters name, boxSize, keyFrames.. even they will not be used after GraphicsLayerAnimation is constructed. I think GraphicsLayerAnimation should have only setState() as setter. Because state is only one that will be changed. I prefer the huge constructor instead of many setters.

(In reply to comment #7) > > > OK, let's move forward but remove the inner class, move AnimationDirection to RenderStyleConstants (need to see if dhyatt/smfr are ok with that), and use setters instead of a huge constructor. > > > > Got it :) > > > > Sorry, I didn't catch your last sentence. ("use setters instead of a huge constructor.") > If we use setters instead of a some huge constructor, GraphicsLayerAnimation must have many setters name, boxSize, keyFrames.. even they will not be used after GraphicsLayerAnimation is constructed. > I think GraphicsLayerAnimation should have only setState() as setter. Because state is only one that will be changed. I prefer the huge constructor instead of many setters. Ok, how about a struct GraphicsLayerAnimation::Atributes, same as GraphicsContext3D::Attributes, which is only used for construction, and later everything is a regular member.

> > OK, let's move forward but remove the inner class, move AnimationDirection to RenderStyleConstants (need to see if dhyatt/smfr are ok with that), and use setters instead of a huge constructor. > > Got it :) > Sorry, I didn't catch your last sentence. ("use setters instead of a huge constructor.") If we use setters instead of a some huge constructor, GraphicsLayerAnimation must have many setters name, boxSize, keyFrames.. even they will not be used after GraphicsLayerAnimation is constructed. I think GraphicsLayerAnimation should have only setState() as setter. Because state is only one that will be changed. I prefer the huge constructor instead of many setters.

oops. comment#9 is collision. > > > > Sorry, I didn't catch your last sentence. ("use setters instead of a huge constructor.") > > If we use setters instead of a some huge constructor, GraphicsLayerAnimation must have many setters name, boxSize, keyFrames.. even they will not be used after GraphicsLayerAnimation is constructed. > > I think GraphicsLayerAnimation should have only setState() as setter. Because state is only one that will be changed. I prefer the huge constructor instead of many setters. > > Ok, how about a struct GraphicsLayerAnimation::Atributes, same as GraphicsContext3D::Attributes, which is only used for construction, and later everything is a regular member. It makes sense completly! :) I will fix it like that. Thanks!

Created attachment 187211 [details] Patch

For the meantime, leave AnimationDirection at that first. I should changes more than 5 files if I move AnimationDirection at this patch. After this patch, I will move AnimationDirection to RenderStyleConstants.h if dhyatt/smfr accept with that.

Comment on attachment 187211 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187211&action=review > Source/WebCore/ChangeLog:10 > + GraphicsLayerAnimation uses Animation just for some members. It is more > + succinct to have the internal type for this purpose. Extracting some members > + from Animation, I create GraphicsLayerAnimation::Animation. "More succinct" is not a good enough argument for this patch... We're duplicating several things from WebCore::Animation, like fillsForwards() etc. We need a better argument for this. > Source/WebCore/platform/graphics/GraphicsLayerAnimation.cpp:156 > +GraphicsLayerAnimation::GraphicsLayerAnimation(const String& name, const KeyframeValueList& keyframes, const IntSize& boxSize, const Attributes& animAttrs, double startTime, bool listsMatch) animAttr -> attributes > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:55 > + Attributes(unsigned _direction, double _duration, double _iterationCount, double _fillMode, PassRefPtr<TimingFunction> _timingFunction) Don't use _ please... : direction(direction) should be ok, also : direction(newDirection). > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:91 > + Attributes attributes() const { return m_attributes; } const Attributes& attributes() const > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:614 > + m_animations.add(GraphicsLayerAnimation(keyframesName, valueList, boxSize, animAttrs, WTF::currentTime() - timeOffset, listsMatch)); animAttr -> attributes > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1027 > + const GraphicsLayerAnimation::Attributes animAttrs(anim->direction(), anim->duration(), anim->iterationCount(), anim->fillMode(), anim->timingFunction()); No need for const. > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1029 > + m_animations.add(GraphicsLayerAnimation(keyframesName, valueList, boxSize, animAttrs, m_lastAnimationStartTime, listsMatch)); Ditto > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:749 > + const GraphicsLayerAnimation::Attributes animAttrs(direction, duration, iterationCount, fillMode, timingFunction); No need for const.

Created attachment 187980 [details] Patch

(In reply to comment #13) > (From update of attachment 187211 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187211&action=review > > > Source/WebCore/ChangeLog:10 > > + GraphicsLayerAnimation uses Animation just for some members. It is more > > + succinct to have the internal type for this purpose. Extracting some members > > + from Animation, I create GraphicsLayerAnimation::Animation. > > "More succinct" is not a good enough argument for this patch... We're duplicating several things from WebCore::Animation, like fillsForwards() etc. We need a better argument for this. > I changed comment. How about that argument? > > Source/WebCore/platform/graphics/GraphicsLayerAnimation.cpp:156 > > +GraphicsLayerAnimation::GraphicsLayerAnimation(const String& name, const KeyframeValueList& keyframes, const IntSize& boxSize, const Attributes& animAttrs, double startTime, bool listsMatch) > > animAttr -> attributes > Done. > > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:55 > > + Attributes(unsigned _direction, double _duration, double _iterationCount, double _fillMode, PassRefPtr<TimingFunction> _timingFunction) > > Don't use _ please... Done. > : direction(direction) should be ok, also : direction(newDirection). > > > Source/WebCore/platform/graphics/GraphicsLayerAnimation.h:91 > > + Attributes attributes() const { return m_attributes; } > > const Attributes& attributes() const > > > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:614 > > + m_animations.add(GraphicsLayerAnimation(keyframesName, valueList, boxSize, animAttrs, WTF::currentTime() - timeOffset, listsMatch)); > > animAttr -> attributes > Done. > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1027 > > + const GraphicsLayerAnimation::Attributes animAttrs(anim->direction(), anim->duration(), anim->iterationCount(), anim->fillMode(), anim->timingFunction()); > > No need for const. > Done. > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:1029 > > + m_animations.add(GraphicsLayerAnimation(keyframesName, valueList, boxSize, animAttrs, m_lastAnimationStartTime, listsMatch)); > > Ditto > Done. > > Source/WebKit2/Shared/CoordinatedGraphics/CoordinatedGraphicsArgumentCoders.cpp:749 > > + const GraphicsLayerAnimation::Attributes animAttrs(direction, duration, iterationCount, fillMode, timingFunction); > > No need for const. Done. Thanks for kind review.

I discussed with smfr and jamesr about moving enum AnimationDirection. They said that platform/ files include files in rendering/ is a layering violation. I filed this bug https://bugs.webkit.org/show_bug.cgi?id=109649

I think it again, leaving GraphicsLayerAnimation as it is would be fine. Because we use almost everything out of WebCore::Animation and https://bugs.webkit.org/show_bug.cgi?id=103854 will remove WebCore::Animation.