Bug 108610 - [TexMap] Remove Animation in GraphicsLayerAnimation.
Summary: [TexMap] Remove Animation in GraphicsLayerAnimation.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Changwan Hong
URL:
Keywords:
Depends on:
Blocks: 103854
  Show dependency treegraph
 
Reported: 2013-02-01 03:45 PST by Dongseong Hwang
Modified: 2013-02-14 00:34 PST (History)
9 users (show)

See Also:


Attachments
Patch (17.50 KB, patch)
2013-02-06 18:41 PST, Changwan Hong
no flags Details | Formatted Diff | Diff
Patch (20.15 KB, patch)
2013-02-07 18:04 PST, Changwan Hong
no flags Details | Formatted Diff | Diff
Patch (20.04 KB, patch)
2013-02-12 17:50 PST, Changwan Hong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2013-02-01 03:45:36 PST
GraphicsLayerAnimation uses Animation just for some members.
It is more succinct to have the internal type for this purpose.
Comment 1 Changwan Hong 2013-02-06 18:41:48 PST
Created attachment 186974 [details]
Patch
Comment 2 Changwan Hong 2013-02-06 18:42:48 PST
After this patch, I will refactor some static methods (e.g. normalizedAnimationValue, normalizedAnimationValueForFillsForwards ..) as GraphicsLayerAnimation::Animation has them.
Comment 3 Noam Rosenthal 2013-02-06 22:57:13 PST
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
Comment 4 Changwan Hong 2013-02-07 00:39:33 PST
(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 5 Noam Rosenthal 2013-02-07 00:48:51 PST
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().
Comment 6 Changwan Hong 2013-02-07 01:25:34 PST
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.
Comment 7 Changwan Hong 2013-02-07 02:02:03 PST
> > 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.
Comment 8 Noam Rosenthal 2013-02-07 04:15:37 PST
(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.
Comment 9 Changwan Hong 2013-02-07 04:39:59 PST
> > 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.
Comment 10 Changwan Hong 2013-02-07 04:52:21 PST
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!
Comment 11 Changwan Hong 2013-02-07 18:04:54 PST
Created attachment 187211 [details]
Patch
Comment 12 Changwan Hong 2013-02-07 18:10:40 PST
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 13 Noam Rosenthal 2013-02-09 08:26:07 PST
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.
Comment 14 Changwan Hong 2013-02-12 17:50:22 PST
Created attachment 187980 [details]
Patch
Comment 15 Changwan Hong 2013-02-12 17:53:02 PST
(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.
Comment 16 Changwan Hong 2013-02-12 20:02:33 PST
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
Comment 17 Changwan Hong 2013-02-13 23:29:19 PST
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.