Blink Compositor Animation: Make Animation and Curve methods non-virtual.

third_party/WebKit/Source/platform/testing/CompositorAnimationsTestUtils.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CompositorAnimationsTestUtils_h
+#define CompositorAnimationsTestUtils_h
+
+#include "cc/animation/animation.h"
+#include "cc/animation/animation_curve.h"
+#include "cc/animation/keyframed_animation_curve.h"
+#include "cc/animation/timing_function.h"
+
+using TimeDeltaForTesting = base::TimeDelta;

[jbroman, 2016/05/27 14:52:55]

This doesn't seem super helpful and makes the Source/platform/ abstraction super
leaky, even if only for testing. I don't know that referring to
"CcAnimationForTesting" is any better than "cc::Animation" (except it's harder
to read and adds a weird indirection that doesn't actually hide the interface).

I'd much rather you provide actual testing utilities in platform; ideally ones
that operated on a blink::CompositorAnimation (etc) rather than cc::Animation.
For example:


std::unique_ptr<CcAnimationForTesting> animation =
convertToCompositorAnimation(*effect);
EXPECT_EQ(CompositorTargetProperty::OPACITY, animation->target_property());
// ...

could instead be written as

OwnPtr<CompositorAnimation> animation = convertToCompositorAnimation(*effect);
EXPECT_EQ(CompositorTargetProperty::OPACITY, animation->targetProperty());
// ...


const CcKeyframedFloatAnimationCurveForTesting* keyframedFloatCurve =
ToFloatCurve(animation.get());
DCHECK_EQ(2UL, keyframedFloatCurve->keyframes_for_testing().size());
const CcFloatKeyframeForTesting* keyframe1 =
keyframedFloatCurve->keyframes_for_testing()[0].get();
const CcFloatKeyframeForTesting* keyframe2 =
keyframedFloatCurve->keyframes_for_testing()[1].get();
EXPECT_EQ(TimeDeltaForTesting(), keyframe1->Time());
EXPECT_EQ(2.0f, keyframe1->Value());

could instead be written as

// first two lines could be grouped into a floatCurveKeyframesForAnimation(const
CompositorAnimation&).
OwnPtr<CompositorFloatAnimationCurve> floatAnimationCurve =
animation->floatCurveForTesting();
Vector<CompositorFloatKeyframe> keyframes =
floatAnimationCurve->keyframesForTesting();
ASSERT_EQ(2UL, keyframes.size());
EXPECT_EQ(0.0, keyframes[0].time);
EXPECT_EQ(2.0f, keyframes[0].value);

or perhaps, if you prefer, you could move some of the checks into helpers using
functions (returning bools or testing::AssertionResult) or gmock matchers


If you're worried about whether these accessors faithfully represent the
underlying cc::Animation, that should be tested in platform.

Alternatively, if some of the logic of that test doesn't require core, perhaps
it can be moved to blink_platform_unittests, where accessing underlying types is
appropriate.

If we do want to use base/cc types in core for testing, I'd rather see a
discussion about allowing that than a ton of type aliases.

[loyso (OOO), 2016/05/31 02:20:39]

Done.

+using CcAnimationForTesting = cc::Animation;
+using CcTimingFunctionForTesting = cc::TimingFunction;
+using CcKeyframedFloatAnimationCurveForTesting = cc::KeyframedFloatAnimationCurve;
+using CcCubicBezierTimingFunctionForTesting = cc::CubicBezierTimingFunction;
+using CcFloatKeyframeForTesting = cc::FloatKeyframe;
+using CcAnimationCurveForTesting = cc::AnimationCurve;
+
+#endif // CompositorAnimationsTestUtils_h

[alancutter (OOO until 2018), 2016/06/01 02:20:55]

This is only included once, why have a separate file?

[loyso (OOO), 2016/06/01 02:36:47]

Irrelevant with a new patch set.

[loyso (OOO), 2016/06/01 02:38:20]

The actual reason was the folder, where is file is located. You are allowed to
include and use cc only in platform/animation.