Fixing reflectivity of operator== for timing functions.

Fixing reflectivity of operator== for timing functions.

(Also added a bunch of equality tests to make sure no other bugs exist.)

Previously, ((A == B) == (B == A)) was not true in all cases.

If A was a preset and B was a Custom with identical internal values to the
preset, then the reflectivity would not hold.

Before this patch, the test output looked like the following:
----

TEST_F(TimingFunctionTest, CubicOperatorEqReflectivityBug)
{
    RefPtr<TimingFunction> cubicA = CubicBezierTimingFunction::preset(CubicBezierTimingFunction::EaseIn);
    RefPtr<TimingFunction> cubicB = CubicBezierTimingFunction::create(0.42, 0.0, 1.0, 1.0);
    EXPECT_REFV_NE(cubicA, cubicB);
    EXPECT_REFV_NE(cubicB, cubicA);
}
[ RUN      ] TimingFunctionTest.CubicOperatorEqReflectivityBug
../../third_party/WebKit/Source/core/platform/animation/TimingFunctionTest.cpp:111: Failure
Expected: (*(cubicB.get())) != (*(cubicA.get())), actual: CubicBezierTimingFunction@0x16d1b01f2ec0(Custom, 0.42, 0, 1, 1) vs CubicBezierTimingFunction@0x16d1b01f2020(EaseIn, 0.42, 0, 1, 1)
[  FAILED  ] TimingFunctionTest.CubicOperatorEqReflectivityBug (0 ms)

----

TEST_F(TimingFunctionTest, StepsOperatorEqReflectivityBug)
{
    RefPtr<TimingFunction> stepsA = StepsTimingFunction::preset(StepsTimingFunction::Start);
    RefPtr<TimingFunction> stepsB = StepsTimingFunction::create(1, true);
    EXPECT_REFV_NE(stepsA, stepsB);
    EXPECT_REFV_NE(stepsB, stepsA);
}
[ RUN      ] TimingFunctionTest.StepsOperatorEqReflectivityBug
../../third_party/WebKit/Source/core/platform/animation/TimingFunctionTest.cpp:180: Failure
Expected: (*(stepsB.get())) != (*(stepsA.get())), actual: StepsTimingFunction@0x16d1b01ea800(Custom, 1, true) vs StepsTimingFunction@0x16d1b01e9f20(Start, 1, true)
[  FAILED  ] TimingFunctionTest.StepsOperatorEqReflectivityBug (0 ms)

----

DEPENDS=https://codereview.chromium.org/39223002/
BUG=

[mithro-old, 2013-11-04 12:05:03]

Hi Steve,

I finally figured out exactly what was going on last week and have now split the
problem into multiple patches.

This patch "Fixes the reflectivity of operator== for timing functions".
Previously, ((A == B) == (B == A)) was not true in all cases.

This patch only depends on https://codereview.chromium.org/39223002/
"TimingFunction test helper".

Tim

[Steve Block, 2013-11-04 23:19:23]

Makes sense, but what's the impact on web content? Can you add a LayoutTest to
document the change in behavior?

[mithro-old, 2013-11-05 01:23:20]

On 2013/11/04 23:19:23, Steve Block wrote:
> Makes sense, but what's the impact on web content? Can you add a LayoutTest to
> document the change in behavior?

I can't see how this is exposed to the web without some type of very complicated
set up. Suggestions?

[mithro-old, 2013-11-05 11:42:29]

On 2013/11/05 01:23:20, mithro wrote:
> On 2013/11/04 23:19:23, Steve Block wrote:
> > Makes sense, but what's the impact on web content? Can you add a LayoutTest
to
> > document the change in behavior?
> 
> I can't see how this is exposed to the web without some type of very
complicated
> set up. Suggestions?

I uses attributes to tag the operator== as depreciated and compiled all_webkit,
content_shell and webkit_unit_tests and then traced the values upwards.

 * The TimingFunction::operator== is only used by
CSSAnimationData::animationsMatch
 * The CSSAnimationData::animationsMatch is only used by it's own operator==
 * The CSSAnimationData::operator== is only used by
CSSAnimationDataList::operator==
 * The CSSAnimationDataList::operator== is only used by
CompositeAnimation::updateKeyframeAnimations

The code in CompositeAnimation::updateKeyframeAnimations reads;

    if (currentStyle && currentStyle->hasAnimations() &&
targetStyle->hasAnimations() && *(currentStyle->animations()) ==
*(targetStyle->animations())) {
        // The current and target animations are the same so we just need to
toss any
        // animation which is finished (postActive).
        for (AnimationNameMap::const_iterator it = m_keyframeAnimations.begin();
it != kfend; ++it) {
            if (it->value->postActive())
                it->value->setIndex(-1);
        }
    } else {
        // Mark all existing animations as no longer active.
        ...
        // Toss the animation order map.
        ...
        // Now mark any still active animations as active and add any new
animations.
    }

So, previously knowing how our operator== currently works.

If;
 * currentStyle->animations() had a timing function of "cubic-bezier(0.42, 0.0,
1.0, 1.0)"
 * targetStyle->animations() had a timing function of "ease-in"
Then 
 * We'll hit the if part.

==== but ====

If;
 * currentStyle->animations() had a timing function of "ease-in"
 * targetStyle->animations() had a timing function of "cubic-bezier(0.42, 0.0,
1.0, 1.0)"
Then 
 * We'll hit the else part.


The only difference I can see between the two paths are that in the else path
the a KeyframeAnimation object is updated with the new CSSAnimationData.

   const CSSAnimationData* anim = targetStyle->animations()->animation(i);
   ...
      keyframeAnim->setAnimation(anim);
   ...

The only places I can find which uses the timing function contained inside the
animation object is;

   double AnimationBase::progress(double scale, double offset, const
TimingFunction* timingFunction) const
which does a
    return timingFunction->evaluate(fractionalTime,
accuracyForDuration(m_animation->duration()));

Which is going to be functionaly identical in both the new and old case.

On a side note, it appears that getComputedStyle pulls the timing function
information from somewhere other then this data. It uses a CSSAnimationDataList
and that type that is not stored in the KeyframeAnimation object.
---
static PassRefPtr<CSSValue> valueForAnimationTimingFunction(const
CSSAnimationDataList* animList)
{
    RefPtr<CSSValueList> list = CSSValueList::createCommaSeparated();
    if (animList) {
        for (size_t i = 0; i < animList->size(); ++i)
           
list->append(createTimingFunctionValue(animList->animation(i)->timingFunction()));
    } else
        // Note that initialAnimationTimingFunction() is used for both
transitions and animations
       
list->append(createTimingFunctionValue(CSSAnimationData::initialAnimationTimingFunction().get()));
    return list.release();
}
---




=================================================

Depreciated warnings I got
---

../../third_party/WebKit/Source/core/platform/animation/CSSAnimationData.cpp: In
member function ‘bool WebCore::CSSAnimationData::animationsMatch(const
WebCore::CSSAnimationData*, bool) const’:
../../third_party/WebKit/Source/core/platform/animation/CSSAnimationData.cpp:118:78:
warning: ‘virtual bool WebCore::TimingFunction::operator==(const
WebCore::TimingFunction&) const’ is deprecated (declared at
../../third_party/WebKit/Source/core/platform/animation/TimingFunction.h:54):
Base TimingFunction operator== [-Wdeprecated-declarations]

----

../../third_party/WebKit/Source/core/platform/animation/CSSAnimationDataList.cpp:
In member function ‘bool WebCore::CSSAnimationDataList::operator==(const
WebCore::CSSAnimationDataList&) const’:
../../third_party/WebKit/Source/core/platform/animation/CSSAnimationDataList.cpp:59:44:
warning: ‘bool WebCore::CSSAnimationData::operator!=(const
WebCore::CSSAnimationData&) const’ is deprecated (declared at
../../third_party/WebKit/Source/core/platform/animation/CSSAnimationData.h:141):
CSSAnimationData operator!= [-Wdeprecated-declarations]

---

../../third_party/WebKit/Source/core/frame/animation/CompositeAnimation.cpp: In
member function ‘void
WebCore::CompositeAnimation::updateKeyframeAnimations(WebCore::RenderObject*,
WebCore::RenderStyle*, WebCore::RenderStyle*)’:
../../third_party/WebKit/Source/core/frame/animation/CompositeAnimation.cpp:198:150:
warning: ‘bool WebCore::CSSAnimationDataList::operator==(const
WebCore::CSSAnimationDataList&) const’ is deprecated (declared at
../../third_party/WebKit/Source/core/platform/animation/CSSAnimationDataList.h:42):
CSSAnimationData operator== [-Wdeprecated-declarations]

[Steve Block, 2013-11-06 00:31:48]

>  * The CSSAnimationDataList::operator== is only used by
> CompositeAnimation::updateKeyframeAnimations

It's also used by StyleRareNonInheritedData::operator==(), which is used by
RenderStyle::operator==().

However, I think that in both of these cases, we shouldn't be comparing timing
functions at all. See https://codereview.chromium.org/60033004. So you could
modify TimingFunction::operator==() as you see fit for testing.

[mithro-old, 2013-11-06 01:09:20]

On 2013/11/06 00:31:48, Steve Block wrote:
> >  * The CSSAnimationDataList::operator== is only used by
> > CompositeAnimation::updateKeyframeAnimations
> 
> It's also used by StyleRareNonInheritedData::operator==(), which is used by
> RenderStyle::operator==().
> 
> However, I think that in both of these cases, we shouldn't be comparing timing
> functions at all. See https://codereview.chromium.org/60033004. So you could
> modify TimingFunction::operator==() as you see fit for testing.

After https://codereview.chromium.org/60033004 lands, I will move all operator==
stuff to the test helper, this making this CL obsolete. Closing.