CSSValue Immediates: Change CSSValue to an object instead of a pointer

Source/core/animation/animatable/AnimatableUnknownTest.cpp

 /*
  * Copyright (c) 2013, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 23 matching lines @@
 #include "core/animation/animatable/AnimatableNeutral.h"
 #include "core/css/CSSValuePool.h"
 
 #include <gtest/gtest.h>
 
 using namespace blink;
 
 namespace {
 
 class AnimationAnimatableUnknownTest : public ::testing::Test {
+public:
+    AnimationAnimatableUnknownTest()
+        : cssValue(cssValuePool().createIdentifierValue(CSSValueYellow)),
+        otherCSSValue(cssValuePool().createIdentifierValue(CSSValueOrange)) { };
 protected:
     virtual void SetUp()
     {
         cssValue = cssValuePool().createIdentifierValue(CSSValueYellow);
         animatableUnknown = AnimatableUnknown::create(cssValue);
 
         otherCSSValue = cssValuePool().createIdentifierValue(CSSValueOrange);
         otherAnimatableUnknown = AnimatableUnknown::create(otherCSSValue);
     }
 
-    RefPtrWillBePersistent<CSSValue> cssValue;
+    CSSValue cssValue;
     RefPtrWillBePersistent<AnimatableValue> animatableUnknown;
-    RefPtrWillBePersistent<CSSValue> otherCSSValue;
+    CSSValue otherCSSValue;

[haraken, 2015/06/11 02:48:53]

This looks wrong. What you're doing here is:

class A {  // Not garbage collected
  Persistent<CSSValue> cssValue;
};

to:

class A {  // Not garbage collected
  CSSValueObject* m_cssValueObject; 
};

This is unsafe because the m_cssValueObject is not traced. In general:

- it is safe to replace Member<CSSValue> to CSSValue.
- but it is unsafe to replace Persistent<CSSValue> to CSSValue.

As far as I scan the CL, this is the only place where you replaced
Persistent<CSSValue> with CSSValue, so I think we can work around the issue here
instead of inventing some general solution. Specifically, you can do something
like:

class PersistentCSSValue : public GarbageCollected<PersistentCSSValue> {
  DEFINE_INLINE_TRACE()
  {
    visitor->trace(cssValue);
  }
  CSSValue cssValue;
};

class AnimationAnimatableUnknownTest {
  ...;
  PersistentCSSValue persistentCSSValue;
};

[haraken, 2015/06/11 02:48:53]

oilpan-reviews@: The GC plugin must detect this error.

[sashab, 2015/06/11 23:05:34]

In this case I could just remove the test fixture altogether; the values are
only really needed in the last test anyway. Good to know about Persistent
though.

[sof, 2015/06/12 05:10:04]

This is an anonymous namespace, where the GC plugin isn't activated.

[sof, 2015/06/12 10:48:08]

That quirk has now been removed, http://crbug.com/499738

     RefPtrWillBePersistent<AnimatableValue> otherAnimatableUnknown;
 };
 
 TEST_F(AnimationAnimatableUnknownTest, Create)
 {
     EXPECT_TRUE(animatableUnknown);
 }
 
 TEST_F(AnimationAnimatableUnknownTest, ToCSSValue)
 {
     EXPECT_EQ(cssValue, toAnimatableUnknown(animatableUnknown.get())->toCSSValue());
 }
 
 TEST_F(AnimationAnimatableUnknownTest, Interpolate)
 {
     EXPECT_EQ(cssValue, toAnimatableUnknown(AnimatableValue::interpolate(animatableUnknown.get(), otherAnimatableUnknown.get(), 0).get())->toCSSValue());
     EXPECT_EQ(cssValue, toAnimatableUnknown(AnimatableValue::interpolate(animatableUnknown.get(), otherAnimatableUnknown.get(), 0.4).get())->toCSSValue());
     EXPECT_EQ(otherCSSValue, toAnimatableUnknown(AnimatableValue::interpolate(animatableUnknown.get(), otherAnimatableUnknown.get(), 0.5).get())->toCSSValue());
     EXPECT_EQ(otherCSSValue, toAnimatableUnknown(AnimatableValue::interpolate(animatableUnknown.get(), otherAnimatableUnknown.get(), 0.6).get())->toCSSValue());
     EXPECT_EQ(otherCSSValue, toAnimatableUnknown(AnimatableValue::interpolate(animatableUnknown.get(), otherAnimatableUnknown.get(), 1).get())->toCSSValue());
 
     EXPECT_EQ(otherCSSValue, toAnimatableUnknown(AnimatableValue::interpolate(otherAnimatableUnknown.get(), animatableUnknown.get(), 0).get())->toCSSValue());
     EXPECT_EQ(otherCSSValue, toAnimatableUnknown(AnimatableValue::interpolate(otherAnimatableUnknown.get(), animatableUnknown.get(), 0.4).get())->toCSSValue());
     EXPECT_EQ(cssValue, toAnimatableUnknown(AnimatableValue::interpolate(otherAnimatableUnknown.get(), animatableUnknown.get(), 0.5).get())->toCSSValue());
     EXPECT_EQ(cssValue, toAnimatableUnknown(AnimatableValue::interpolate(otherAnimatableUnknown.get(), animatableUnknown.get(), 0.6).get())->toCSSValue());
     EXPECT_EQ(cssValue, toAnimatableUnknown(AnimatableValue::interpolate(otherAnimatableUnknown.get(), animatableUnknown.get(), 1).get())->toCSSValue());
 }
 
 }