[webkit-reviews] review requested: [Bug 90468] [chromium] Use WebAnimation and related classes in GraphicsLayerChromium and AnimTranslationUtil : [Attachment 154089] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 90468: [chromium] Use WebAnimation and related classes in
GraphicsLayerChromium and AnimTranslationUtil
https://bugs.webkit.org/show_bug.cgi?id=90468

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

------- Additional Comments from vollick at chromium.org
(In reply to comment #4)
> (From update of attachment 152311 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=152311&action=review
>
> This is all a pretty straightforward conversion, and looks good in general. 
I have one small comment:
>
> > Source/Platform/chromium/public/WebAnimation.h:49
> > -	     WebAnimationTransform = 1,
> > -	     WebAnimationOpacity
> > +	     TargetPropertyTransform = 1,
> > +	     TargetPropertyOpacity
>
> Were these just not used for anything previously?
Yup, and I'd just noticed with this patch that their names didn't conform with
the API standards.
>
> > Source/WebCore/platform/graphics/chromium/AnimationTranslationUtil.cpp:172
> > -bool appendKeyframe<TransformAnimationValue, CCTransformKeyframe,
CCKeyframedTransformAnimationCurve>(CCKeyframedTransformAnimationCurve& curve,
double keyTime, const TransformAnimationValue* value, const
TransformAnimationValue* lastValue, PassOwnPtr<CCTimingFunction>
timingFunction, const FloatSize& boxSize)
> > +bool appendKeyframe<TransformAnimationValue, WebTransformKeyframe,
WebTransformAnimationCurve>(WebTransformAnimationCurve& curve, double keyTime,
const TransformAnimationValue* value, const TransformAnimationValue* lastValue,
bool isUsingCustomBezierTimingFunction,
WebKit::WebAnimationCurve::TimingFunctionType timingFunctionType, double x1,
double y1, double x2, double y2, const FloatSize& boxSize)
>
> I have a personal style nit against functions that take boolean values and a
lot of optional parameters that are only used when going down one path.  This
kind of feels like you should have two functions, e.g. addCustomBezierKeyFrame
and addWhateverNormalKeyFrame.
Cool, fixed.