Web Animations: use WTF::Optional for Keyframe::offset_

Previously nullable values in the animations code were tracked by
storing nulls as quiet_NaN() and using std::isnan as a null-detector.
It is much more explicit to store such values using WTF::Optional, which
forces code to consider whether or not the offset exists.

Bug: 791086
Change-Id: I4db0457b593d6be61dedb5c13a9daf8b671206b0
Reviewed-on: https://chromium-review.googlesource.com/804116Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522021}

third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/Animatable/animate-expected.txt

 This is a testharness.js-based test.
-Found 121 tests; 68 PASS, 53 FAIL, 0 TIMEOUT, 0 NOTRUN.
+Found 121 tests; 75 PASS, 46 FAIL, 0 TIMEOUT, 0 NOTRUN.
 PASS Element.animate() creates an Animation object
 PASS Element.animate() creates an Animation object in the relevant realm of the target element
 PASS Element.animate() creates an Animation object with a KeyframeEffect
@@ -53,18 +53,18 @@ FAIL Element.animate() accepts a two property (a shorthand and one of its compon
 PASS Element.animate() accepts a two property keyframe sequence where one property is missing from the first keyframe
 PASS Element.animate() accepts a two property keyframe sequence where one property is missing from the last keyframe
 PASS Element.animate() accepts a one property two keyframe sequence that needs to stringify its values
-FAIL Element.animate() accepts a keyframe sequence with a CSS variable reference assert_equals: value for 'offset' on ComputedKeyframe #0 expected (object) null but got (number) NaN
+PASS Element.animate() accepts a keyframe sequence with a CSS variable reference
 FAIL Element.animate() accepts a keyframe sequence with a CSS variable reference in a shorthand property assert_equals: properties on ComputedKeyframe #0 should match expected "computedOffset,easing,margin,offset" but got "computedOffset,easing,marginBottom,marginLeft,marginRight,marginTop,offset"
 PASS Element.animate() accepts a keyframe sequence with duplicate values for a given interior offset
 PASS Element.animate() accepts a keyframe sequence with duplicate values for offsets 0 and 1
 PASS Element.animate() accepts a two property four keyframe sequence
-FAIL Element.animate() accepts a single keyframe sequence with omitted offset assert_equals: value for 'offset' on ComputedKeyframe #0 expected (object) null but got (number) NaN
-FAIL Element.animate() accepts a single keyframe sequence with null offset assert_equals: value for 'offset' on ComputedKeyframe #0 expected (object) null but got (number) NaN
+PASS Element.animate() accepts a single keyframe sequence with omitted offset
+PASS Element.animate() accepts a single keyframe sequence with null offset
 PASS Element.animate() accepts a single keyframe sequence with string offset
-FAIL Element.animate() accepts a one property keyframe sequence with some omitted offsets assert_equals: value for 'offset' on ComputedKeyframe #2 expected (object) null but got (number) NaN
-FAIL Element.animate() accepts a one property keyframe sequence with some null offsets assert_equals: value for 'offset' on ComputedKeyframe #2 expected (object) null but got (number) NaN
-FAIL Element.animate() accepts a two property keyframe sequence with some omitted offsets assert_equals: value for 'offset' on ComputedKeyframe #2 expected (object) null but got (number) NaN
-FAIL Element.animate() accepts a one property keyframe sequence with all omitted offsets assert_equals: value for 'offset' on ComputedKeyframe #0 expected (object) null but got (number) NaN
+PASS Element.animate() accepts a one property keyframe sequence with some omitted offsets
+PASS Element.animate() accepts a one property keyframe sequence with some null offsets
+PASS Element.animate() accepts a two property keyframe sequence with some omitted offsets
+PASS Element.animate() accepts a one property keyframe sequence with all omitted offsets
 PASS Element.animate() accepts a keyframe sequence with different easing values, but the same easing value for a given offset
 PASS Element.animate() accepts a keyframe sequence with different composite values, but the same composite value for a given offset
 PASS Element.animate() does not accept keyframes with an out-of-bounded positive offset

third_party/WebKit/LayoutTests/external/wpt/web-animations/interfaces/KeyframeEffect/constructor-expected.txt

 This is a testharness.js-based test.
-Found 167 tests; 89 PASS, 78 FAIL, 0 TIMEOUT, 0 NOTRUN.
+Found 167 tests; 103 PASS, 64 FAIL, 0 TIMEOUT, 0 NOTRUN.
 PASS A KeyframeEffectReadOnly can be constructed with no frames
 PASS easing values are parsed correctly when passed to the KeyframeEffectReadOnly constructor in KeyframeEffectOptions
 PASS Invalid easing values are correctly rejected when passed to the KeyframeEffectReadOnly constructor in KeyframeEffectOptions
@@ -96,30 +96,30 @@ PASS A KeyframeEffectReadOnly can be constructed with a two property keyframe se
 PASS A KeyframeEffectReadOnly constructed with a two property keyframe sequence where one property is missing from the last keyframe roundtrips
 PASS A KeyframeEffectReadOnly can be constructed with a one property two keyframe sequence that needs to stringify its values
 PASS A KeyframeEffectReadOnly constructed with a one property two keyframe sequence that needs to stringify its values roundtrips
-FAIL A KeyframeEffectReadOnly can be constructed with a keyframe sequence with a CSS variable reference assert_equals: value for 'offset' on ComputedKeyframe #0 expected (object) null but got (number) NaN
-FAIL A KeyframeEffectReadOnly constructed with a keyframe sequence with a CSS variable reference roundtrips Failed to construct 'KeyframeEffectReadOnly': Non numeric offset provided
+PASS A KeyframeEffectReadOnly can be constructed with a keyframe sequence with a CSS variable reference
+PASS A KeyframeEffectReadOnly constructed with a keyframe sequence with a CSS variable reference roundtrips
 FAIL A KeyframeEffectReadOnly can be constructed with a keyframe sequence with a CSS variable reference in a shorthand property assert_equals: properties on ComputedKeyframe #0 should match expected "computedOffset,easing,margin,offset" but got "computedOffset,easing,marginBottom,marginLeft,marginRight,marginTop,offset"
-FAIL A KeyframeEffectReadOnly constructed with a keyframe sequence with a CSS variable reference in a shorthand property roundtrips Failed to construct 'KeyframeEffectReadOnly': Non numeric offset provided
+FAIL A KeyframeEffectReadOnly constructed with a keyframe sequence with a CSS variable reference in a shorthand property roundtrips assert_equals: properties on ComputedKeyframe #0 should match expected "computedOffset,easing,marginBottom,marginLeft,marginRight,marginTop,offset" but got "computedOffset,easing,offset"
 PASS A KeyframeEffectReadOnly can be constructed with a keyframe sequence with duplicate values for a given interior offset
 PASS A KeyframeEffectReadOnly constructed with a keyframe sequence with duplicate values for a given interior offset roundtrips
 PASS A KeyframeEffectReadOnly can be constructed with a keyframe sequence with duplicate values for offsets 0 and 1
 PASS A KeyframeEffectReadOnly constructed with a keyframe sequence with duplicate values for offsets 0 and 1 roundtrips
 PASS A KeyframeEffectReadOnly can be constructed with a two property four keyframe sequence
 PASS A KeyframeEffectReadOnly constructed with a two property four keyframe sequence roundtrips
-FAIL A KeyframeEffectReadOnly can be constructed with a single keyframe sequence with omitted offset assert_equals: value for 'offset' on ComputedKeyframe #0 expected (object) null but got (number) NaN
-FAIL A KeyframeEffectReadOnly constructed with a single keyframe sequence with omitted offset roundtrips Failed to construct 'KeyframeEffectReadOnly': Non numeric offset provided
-FAIL A KeyframeEffectReadOnly can be constructed with a single keyframe sequence with null offset assert_equals: value for 'offset' on ComputedKeyframe #0 expected (object) null but got (number) NaN
-FAIL A KeyframeEffectReadOnly constructed with a single keyframe sequence with null offset roundtrips Failed to construct 'KeyframeEffectReadOnly': Non numeric offset provided
+PASS A KeyframeEffectReadOnly can be constructed with a single keyframe sequence with omitted offset
+PASS A KeyframeEffectReadOnly constructed with a single keyframe sequence with omitted offset roundtrips
+PASS A KeyframeEffectReadOnly can be constructed with a single keyframe sequence with null offset
+PASS A KeyframeEffectReadOnly constructed with a single keyframe sequence with null offset roundtrips
 PASS A KeyframeEffectReadOnly can be constructed with a single keyframe sequence with string offset
 PASS A KeyframeEffectReadOnly constructed with a single keyframe sequence with string offset roundtrips
-FAIL A KeyframeEffectReadOnly can be constructed with a one property keyframe sequence with some omitted offsets assert_equals: value for 'offset' on ComputedKeyframe #2 expected (object) null but got (number) NaN
-FAIL A KeyframeEffectReadOnly constructed with a one property keyframe sequence with some omitted offsets roundtrips Failed to construct 'KeyframeEffectReadOnly': Non numeric offset provided
-FAIL A KeyframeEffectReadOnly can be constructed with a one property keyframe sequence with some null offsets assert_equals: value for 'offset' on ComputedKeyframe #2 expected (object) null but got (number) NaN
-FAIL A KeyframeEffectReadOnly constructed with a one property keyframe sequence with some null offsets roundtrips Failed to construct 'KeyframeEffectReadOnly': Non numeric offset provided
-FAIL A KeyframeEffectReadOnly can be constructed with a two property keyframe sequence with some omitted offsets assert_equals: value for 'offset' on ComputedKeyframe #2 expected (object) null but got (number) NaN
-FAIL A KeyframeEffectReadOnly constructed with a two property keyframe sequence with some omitted offsets roundtrips Failed to construct 'KeyframeEffectReadOnly': Non numeric offset provided
-FAIL A KeyframeEffectReadOnly can be constructed with a one property keyframe sequence with all omitted offsets assert_equals: value for 'offset' on ComputedKeyframe #0 expected (object) null but got (number) NaN
-FAIL A KeyframeEffectReadOnly constructed with a one property keyframe sequence with all omitted offsets roundtrips Failed to construct 'KeyframeEffectReadOnly': Non numeric offset provided
+PASS A KeyframeEffectReadOnly can be constructed with a one property keyframe sequence with some omitted offsets
+PASS A KeyframeEffectReadOnly constructed with a one property keyframe sequence with some omitted offsets roundtrips
+PASS A KeyframeEffectReadOnly can be constructed with a one property keyframe sequence with some null offsets
+PASS A KeyframeEffectReadOnly constructed with a one property keyframe sequence with some null offsets roundtrips
+PASS A KeyframeEffectReadOnly can be constructed with a two property keyframe sequence with some omitted offsets
+PASS A KeyframeEffectReadOnly constructed with a two property keyframe sequence with some omitted offsets roundtrips
+PASS A KeyframeEffectReadOnly can be constructed with a one property keyframe sequence with all omitted offsets
+PASS A KeyframeEffectReadOnly constructed with a one property keyframe sequence with all omitted offsets roundtrips
 PASS A KeyframeEffectReadOnly can be constructed with a keyframe sequence with different easing values, but the same easing value for a given offset
 PASS A KeyframeEffectReadOnly constructed with a keyframe sequence with different easing values, but the same easing value for a given offset roundtrips
 PASS A KeyframeEffectReadOnly can be constructed with a keyframe sequence with different composite values, but the same composite value for a given offset

third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp

@@ -167,7 +167,7 @@ class AnimationCompositorAnimationsTest : public RenderingTest {
 
   bool DuplicateSingleKeyframeAndTestIsCandidateOnResult(
       StringKeyframe* frame) {
-    EXPECT_EQ(frame->Offset(), 0);
+    EXPECT_EQ(frame->CheckedOffset(), 0);
     StringKeyframeVector frames;
     scoped_refptr<Keyframe> second = frame->CloneWithOffset(1);
 
@@ -260,7 +260,7 @@ class AnimationCompositorAnimationsTest : public RenderingTest {
       scoped_refptr<StringKeyframe> to,
       scoped_refptr<StringKeyframe> c = nullptr,
       scoped_refptr<StringKeyframe> d = nullptr) {
-    EXPECT_EQ(from->Offset(), 0);
+    EXPECT_EQ(from->CheckedOffset(), 0);
     StringKeyframeVector frames;
     frames.push_back(from);
     EXPECT_LE(from->Offset(), to->Offset());
@@ -272,9 +272,9 @@ class AnimationCompositorAnimationsTest : public RenderingTest {
     if (d) {
       frames.push_back(d);
       EXPECT_LE(c->Offset(), d->Offset());
-      EXPECT_EQ(d->Offset(), 1.0);
+      EXPECT_EQ(d->CheckedOffset(), 1.0);
     } else {
-      EXPECT_EQ(to->Offset(), 1.0);
+      EXPECT_EQ(to->CheckedOffset(), 1.0);
     }
     if (!HasFatalFailure()) {
       return StringKeyframeEffectModel::Create(frames);

third_party/WebKit/Source/core/animation/EffectInputTest.cpp

@@ -48,7 +48,7 @@ TEST(AnimationEffectInputTest, SortedOffsets) {
       DictionarySequenceOrDictionary::FromDictionarySequence(js_keyframes),
       EffectModel::kCompositeReplace, nullptr, scope.GetExceptionState());
   EXPECT_FALSE(scope.GetExceptionState().HadException());
-  EXPECT_EQ(1.0, effect->GetFrames()[1]->Offset());
+  EXPECT_EQ(1.0, effect->GetFrames()[1]->CheckedOffset());
 }
 
 TEST(AnimationEffectInputTest, UnsortedOffsets) {
@@ -102,7 +102,7 @@ TEST(AnimationEffectInputTest, LooslySorted) {
       DictionarySequenceOrDictionary::FromDictionarySequence(js_keyframes),
       EffectModel::kCompositeReplace, nullptr, scope.GetExceptionState());
   EXPECT_FALSE(scope.GetExceptionState().HadException());
-  EXPECT_EQ(1, effect->GetFrames()[2]->Offset());
+  EXPECT_EQ(1, effect->GetFrames()[2]->CheckedOffset());
 }
 
 TEST(AnimationEffectInputTest, OutOfOrderWithNullOffsets) {

third_party/WebKit/Source/core/animation/Keyframe.cpp

@@ -22,7 +22,11 @@ Keyframe::PropertySpecificKeyframe::CreateInterpolation(
 
 void Keyframe::AddKeyframePropertiesToV8Object(
     V8ObjectBuilder& object_builder) const {
-  object_builder.Add("offset", offset_);
+  if (offset_) {
+    object_builder.Add("offset", offset_.value());
+  } else {
+    object_builder.AddNull("offset");
+  }
   object_builder.Add("easing", easing_->ToString());
   if (composite_.has_value()) {
     object_builder.AddString(
@@ -33,7 +37,9 @@ void Keyframe::AddKeyframePropertiesToV8Object(
 
 bool Keyframe::CompareOffsets(const scoped_refptr<Keyframe>& a,
                               const scoped_refptr<Keyframe>& b) {
-  return a->Offset() < b->Offset();
+  if (!a->Offset() || !b->Offset())
+    return false;
+  return a->CheckedOffset() < b->CheckedOffset();
 }
 
 }  // namespace blink

third_party/WebKit/Source/core/animation/Keyframe.h

@@ -67,8 +67,9 @@ class CORE_EXPORT Keyframe : public RefCounted<Keyframe> {
   virtual ~Keyframe() {}
 
   // TODO(smcgruer): The keyframe offset should be immutable.
-  void SetOffset(double offset) { offset_ = offset; }
-  double Offset() const { return offset_; }
+  void SetOffset(WTF::Optional<double> offset) { offset_ = offset; }
+  WTF::Optional<double> Offset() const { return offset_; }
+  double CheckedOffset() const { return offset_.value(); }
 
   // TODO(smcgruer): The keyframe composite operation should be immutable.
   void SetComposite(EffectModel::CompositeOperation composite) {
@@ -191,10 +192,8 @@ class CORE_EXPORT Keyframe : public RefCounted<Keyframe> {
 
  protected:
   Keyframe()
-      : offset_(NullValue()),
-        composite_(),
-        easing_(LinearTimingFunction::Shared()) {}
-  Keyframe(double offset,
+      : offset_(), composite_(), easing_(LinearTimingFunction::Shared()) {}
+  Keyframe(WTF::Optional<double> offset,
            WTF::Optional<EffectModel::CompositeOperation> composite,
            scoped_refptr<TimingFunction> easing)
       : offset_(offset), composite_(composite), easing_(std::move(easing)) {
@@ -202,7 +201,7 @@ class CORE_EXPORT Keyframe : public RefCounted<Keyframe> {
       easing_ = LinearTimingFunction::Shared();
   }
 
-  double offset_;
+  WTF::Optional<double> offset_;
   WTF::Optional<EffectModel::CompositeOperation> composite_;
   scoped_refptr<TimingFunction> easing_;
   DISALLOW_COPY_AND_ASSIGN(Keyframe);

third_party/WebKit/Source/core/animation/KeyframeEffectModel.cpp

@@ -154,35 +154,39 @@ bool KeyframeEffectModelBase::SnapshotAllCompositorKeyframes(
 
 Vector<double> KeyframeEffectModelBase::GetComputedOffsets(
     const KeyframeVector& keyframes) {
+  // To avoid having to create two vectors when converting from the nullable
+  // offsets to the non-nullable computed offsets, we keep the convention in
+  // this function that std::numeric_limits::quiet_NaN() represents null.
   double last_offset = 0;
   Vector<double> result;
   result.ReserveCapacity(keyframes.size());
 
   for (const auto& keyframe : keyframes) {
-    double offset = keyframe->Offset();
-    if (!IsNull(offset)) {
-      DCHECK_GE(offset, 0);
-      DCHECK_LE(offset, 1);
-      DCHECK_GE(offset, last_offset);
-      last_offset = offset;
+    WTF::Optional<double> offset = keyframe->Offset();
+    if (offset) {
+      DCHECK_GE(offset.value(), 0);
+      DCHECK_LE(offset.value(), 1);
+      DCHECK_GE(offset.value(), last_offset);
+      last_offset = offset.value();
     }
-    result.push_back(offset);
+    result.push_back(offset.value_or(std::numeric_limits<double>::quiet_NaN()));
   }
 
   if (result.IsEmpty())
     return result;
 
-  if (IsNull(result.back()))
+  if (std::isnan(result.back()))
     result.back() = 1;
 
-  if (result.size() > 1 && IsNull(result[0]))
+  if (result.size() > 1 && std::isnan(result[0])) {
     result.front() = 0;
+  }
 
   size_t last_index = 0;
   last_offset = result.front();
   for (size_t i = 1; i < result.size(); ++i) {
     double offset = result[i];
-    if (!IsNull(offset)) {
+    if (!std::isnan(offset)) {
       for (size_t j = 1; j < i - last_index; ++j) {
         result[last_index + j] =
             last_offset + (offset - last_offset) * j / (i - last_index);

third_party/WebKit/Source/core/animation/KeyframeEffectTest.cpp

@@ -123,8 +123,8 @@ TEST_F(AnimationKeyframeEffectV8Test, CanCreateAnAnimation) {
 
   const KeyframeVector keyframes = animation->Model()->GetFrames();
 
-  EXPECT_EQ(0, keyframes[0]->Offset());
-  EXPECT_EQ(1, keyframes[1]->Offset());
+  EXPECT_EQ(0, keyframes[0]->CheckedOffset());
+  EXPECT_EQ(1, keyframes[1]->CheckedOffset());
 
   const CSSValue& keyframe1_width =
       ToStringKeyframe(keyframes[0].get())

third_party/WebKit/Source/core/animation/TransitionKeyframe.cpp

@@ -40,7 +40,7 @@ TransitionKeyframe::CreatePropertySpecificKeyframe(
   DCHECK(offset == offset_);
   EffectModel::CompositeOperation composite =
       composite_.value_or(effect_composite);
-  return PropertySpecificKeyframe::Create(Offset(), &Easing(), composite,
+  return PropertySpecificKeyframe::Create(CheckedOffset(), &Easing(), composite,
                                           value_->Clone(), compositor_value_);
 }
 

third_party/WebKit/Source/core/animation/css/CSSAnimations.cpp

@@ -150,7 +150,8 @@ StringKeyframeEffectModel* CreateKeyframeEffectModel(
                    Keyframe::CompareOffsets);
   size_t target_index = 0;
   for (size_t i = 1; i < keyframes.size(); i++) {
-    if (keyframes[i]->Offset() == keyframes[target_index]->Offset()) {
+    if (keyframes[i]->CheckedOffset() ==
+        keyframes[target_index]->CheckedOffset()) {
       for (const auto& property : keyframes[i]->Properties()) {
         keyframes[target_index]->SetCSSPropertyValue(
             property.GetCSSProperty().PropertyID(),
@@ -167,22 +168,22 @@ StringKeyframeEffectModel* CreateKeyframeEffectModel(
   // Add 0% and 100% keyframes if absent.
   scoped_refptr<StringKeyframe> start_keyframe =
       keyframes.IsEmpty() ? nullptr : keyframes[0];
-  if (!start_keyframe || keyframes[0]->Offset() != 0) {
+  if (!start_keyframe || keyframes[0]->CheckedOffset() != 0) {
     start_keyframe = StringKeyframe::Create();
     start_keyframe->SetOffset(0);
     start_keyframe->SetEasing(default_timing_function);
     keyframes.push_front(start_keyframe);
   }
   scoped_refptr<StringKeyframe> end_keyframe = keyframes[keyframes.size() - 1];
-  if (end_keyframe->Offset() != 1) {
+  if (end_keyframe->CheckedOffset() != 1) {
     end_keyframe = StringKeyframe::Create();
     end_keyframe->SetOffset(1);
     end_keyframe->SetEasing(default_timing_function);
     keyframes.push_back(end_keyframe);
   }
   DCHECK_GE(keyframes.size(), 2U);
-  DCHECK(!keyframes.front()->Offset());
-  DCHECK_EQ(keyframes.back()->Offset(), 1);
+  DCHECK_EQ(keyframes.front()->CheckedOffset(), 0);
+  DCHECK_EQ(keyframes.back()->CheckedOffset(), 1);
 
   StringKeyframeEffectModel* model = StringKeyframeEffectModel::Create(
       keyframes, EffectModel::kCompositeReplace, &keyframes[0]->Easing());

third_party/WebKit/Source/core/inspector/InspectorAnimationAgent.cpp

@@ -107,8 +107,7 @@ BuildObjectForAnimationEffect(KeyframeEffectReadOnly* effect,
     DCHECK(effect->Model()->IsKeyframeEffectModel());
     const KeyframeVector& keyframes = effect->Model()->GetFrames();
     if (keyframes.size() == 3) {
-      DCHECK(!IsNull(keyframes.at(1)->Offset()));
-      delay = keyframes.at(1)->Offset() * duration;
+      delay = keyframes.at(1)->CheckedOffset() * duration;
       duration -= delay;
       easing = keyframes.at(1)->Easing().ToString();
     } else {