[webkit-reviews] review denied: [Bug 108610] [TexMap] Remove Animation in GraphicsLayerAnimation. : [Attachment 186974] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam at webkit.org> has denied Chang Wan Hong
<jourmoon at company100.net>'s request for review:
Bug 108610: [TexMap] Remove Animation in GraphicsLayerAnimation.
https://bugs.webkit.org/show_bug.cgi?id=108610

Attachment 186974: Patch
https://bugs.webkit.org/attachment.cgi?id=186974&action=review

------- Additional Comments from Noam Rosenthal <noam at webkit.org>
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().