Bug 1540581 - P7. Use Variant instead of Union/Enum. r=kats

It allows for use of default constructor/destructor and leaves no room to incorrectly modify the union members with a wrong type.

Differential Revision: https://phabricator.services.mozilla.com/D26061

--HG--
extra : moz-landing-system : lando

gfx/layers/AnimationHelper.cpp

@@ -50,37 +50,28 @@ OMTAValue CompositorAnimationStorage::GetOMTAValue(const uint64_t& aId) const {
     return omtaValue;
   }
 
-  switch (animatedValue->mType) {
-    case AnimatedValue::COLOR:
-      omtaValue = animatedValue->mColor;
-      break;
-    case AnimatedValue::OPACITY:
-      omtaValue = animatedValue->mOpacity;
-      break;
-    case AnimatedValue::TRANSFORM: {
-      gfx::Matrix4x4 transform = animatedValue->mTransform.mFrameTransform;
-      const TransformData& data = animatedValue->mTransform.mData;
-      float scale = data.appUnitsPerDevPixel();
-      gfx::Point3D transformOrigin = data.transformOrigin();
+  animatedValue->Value().match(
+      [&](const AnimationTransform& aTransform) {
+        gfx::Matrix4x4 transform = aTransform.mFrameTransform;
+        const TransformData& data = aTransform.mData;
+        float scale = data.appUnitsPerDevPixel();
+        gfx::Point3D transformOrigin = data.transformOrigin();
 
-      // Undo the rebasing applied by
-      // nsDisplayTransform::GetResultingTransformMatrixInternal
-      transform.ChangeBasis(-transformOrigin);
-
-      // Convert to CSS pixels (this undoes the operations performed by
-      // nsStyleTransformMatrix::ProcessTranslatePart which is called from
-      // nsDisplayTransform::GetResultingTransformMatrix)
-      double devPerCss = double(scale) / double(AppUnitsPerCSSPixel());
-      transform._41 *= devPerCss;
-      transform._42 *= devPerCss;
-      transform._43 *= devPerCss;
-      omtaValue = transform;
-      break;
-    }
-    case AnimatedValue::NONE:
-      break;
-  }
+        // Undo the rebasing applied by
+        // nsDisplayTransform::GetResultingTransformMatrixInternal
+        transform.ChangeBasis(-transformOrigin);
 
+        // Convert to CSS pixels (this undoes the operations performed by
+        // nsStyleTransformMatrix::ProcessTranslatePart which is called from
+        // nsDisplayTransform::GetResultingTransformMatrix)
+        double devPerCss = double(scale) / double(AppUnitsPerCSSPixel());
+        transform._41 *= devPerCss;
+        transform._42 *= devPerCss;
+        transform._43 *= devPerCss;
+        omtaValue = transform;
+      },
+      [&](const float& aOpacity) { omtaValue = aOpacity; },
+      [&](const nscolor& aColor) { omtaValue = aColor; });
   return omtaValue;
 }
 
@@ -92,10 +83,9 @@ void CompositorAnimationStorage::SetAnimatedValue(
   AnimatedValue* value = mAnimatedValues.LookupOrAdd(
       aId, std::move(aTransformInDevSpace), std::move(aFrameTransform), aData);
   if (count == mAnimatedValues.Count()) {
-    MOZ_ASSERT(value->mType == AnimatedValue::TRANSFORM);
-    value->mTransform.mTransformInDevSpace = std::move(aTransformInDevSpace);
-    value->mTransform.mFrameTransform = std::move(aFrameTransform);
-    value->mTransform.mData = aData;
+    MOZ_ASSERT(value->Is<AnimationTransform>());
+    *value = AnimatedValue(std::move(aTransformInDevSpace),
+                           std::move(aFrameTransform), aData);
   }
 }
 
@@ -113,8 +103,8 @@ void CompositorAnimationStorage::SetAnimatedValue(uint64_t aId,
   auto count = mAnimatedValues.Count();
   AnimatedValue* value = mAnimatedValues.LookupOrAdd(aId, aColor);
   if (count == mAnimatedValues.Count()) {
-    MOZ_ASSERT(value->mType == AnimatedValue::COLOR);
-    value->mColor = aColor;
+    MOZ_ASSERT(value->Is<nscolor>());
+    *value = AnimatedValue(aColor);
   }
 }
 
@@ -124,8 +114,8 @@ void CompositorAnimationStorage::SetAnimatedValue(uint64_t aId,
   auto count = mAnimatedValues.Count();
   AnimatedValue* value = mAnimatedValues.LookupOrAdd(aId, aOpacity);
   if (count == mAnimatedValues.Count()) {
-    MOZ_ASSERT(value->mType == AnimatedValue::OPACITY);
-    value->mOpacity = aOpacity;
+    MOZ_ASSERT(value->Is<float>());
+    *value = AnimatedValue(aOpacity);
   }
 }
 

gfx/layers/AnimationHelper.h

@@ -13,6 +13,7 @@
 #include "mozilla/webrender/WebRenderTypes.h"  // for RenderRoot
 #include "mozilla/TimeStamp.h"                 // for TimeStamp
 #include "mozilla/TimingParams.h"
+#include "mozilla/Variant.h"
 #include "X11UndefineNone.h"
 
 namespace mozilla {
@@ -88,35 +89,31 @@ struct AnimationTransform {
 };
 
 struct AnimatedValue final {
-  enum { TRANSFORM, OPACITY, COLOR, NONE } mType{NONE};
+  typedef Variant<AnimationTransform, float, nscolor> AnimatedValueType;
 
-  union {
-    AnimationTransform mTransform;
-    float mOpacity;
-    nscolor mColor;
-  };
+  const AnimatedValueType& Value() const { return mValue; }
+  const AnimationTransform& Transform() const {
+    return mValue.as<AnimationTransform>();
+  }
+  const float& Opacity() const { return mValue.as<float>(); }
+  const nscolor& Color() const { return mValue.as<nscolor>(); }
+  template <typename T>
+  bool Is() const {
+    return mValue.is<T>();
+  }
 
   AnimatedValue(gfx::Matrix4x4&& aTransformInDevSpace,
                 gfx::Matrix4x4&& aFrameTransform, const TransformData& aData)
-      : mType(AnimatedValue::TRANSFORM), mOpacity(0.0) {
-    mTransform.mTransformInDevSpace = std::move(aTransformInDevSpace);
-    mTransform.mFrameTransform = std::move(aFrameTransform);
-    mTransform.mData = aData;
-  }
+      : mValue(
+            AsVariant(AnimationTransform{std::move(aTransformInDevSpace),
+                                         std::move(aFrameTransform), aData})) {}
 
-  explicit AnimatedValue(const float& aValue)
-      : mType(AnimatedValue::OPACITY), mOpacity(aValue) {}
+  explicit AnimatedValue(const float& aValue) : mValue(AsVariant(aValue)) {}
 
-  explicit AnimatedValue(nscolor aValue)
-      : mType(AnimatedValue::COLOR), mColor(aValue) {}
-
-  // Can't use = default as AnimatedValue contains a union and has a variant
-  // member with non-trivial destructor.
-  // Otherwise a Deleted implicitly-declared destructor error will occur.
-  ~AnimatedValue() {}
+  explicit AnimatedValue(nscolor aValue) : mValue(AsVariant(aValue)) {}
 
  private:
-  AnimatedValue() = delete;
+  AnimatedValueType mValue;
 };
 
 // CompositorAnimationStorage stores the animations and animated values

gfx/layers/composite/AsyncCompositionManager.cpp

@@ -725,8 +725,9 @@ static bool SampleAnimations(Layer* aLayer,
                                                                 transformData);
             Matrix4x4 transformInDevice = FrameTransformToTransformInDevice(
                 frameTransform, layer, transformData);
-            MOZ_ASSERT(previousValue->mTransform.mTransformInDevSpace
-                           .FuzzyEqualsMultiplicative(transformInDevice));
+            MOZ_ASSERT(previousValue->Transform()
+                           .mTransformInDevSpace.FuzzyEqualsMultiplicative(
+                               transformInDevice));
 #endif
             // In the case of transform we have to set the unchanged
             // transform value again because APZC might have modified the
@@ -739,7 +740,7 @@ static bool SampleAnimations(Layer* aLayer,
                 // layer tree cache. So for the safety, in the case where we
                 // have no previous animation value, we set non-animating value
                 // instead.
-                previousValue ? previousValue->mTransform.mTransformInDevSpace
+                previousValue ? previousValue->Transform().mTransformInDevSpace
                               : layer->GetBaseTransform());
             break;
           }

gfx/layers/wr/WebRenderBridgeParent.cpp

@@ -1849,12 +1849,12 @@ bool WebRenderBridgeParent::SampleAnimations(
       wr::RenderRoot renderRoot = mAnimStorage->AnimationRenderRoot(iter.Key());
       auto& transformArray = aTransformArrays[renderRoot];
       auto& opacityArray = aOpacityArrays[renderRoot];
-      if (value->mType == AnimatedValue::TRANSFORM) {
+      if (value->Is<AnimationTransform>()) {
         transformArray.AppendElement(wr::ToWrTransformProperty(
-            iter.Key(), value->mTransform.mTransformInDevSpace));
-      } else if (value->mType == AnimatedValue::OPACITY) {
+            iter.Key(), value->Transform().mTransformInDevSpace));
+      } else if (value->Is<float>()) {
         opacityArray.AppendElement(
-            wr::ToWrOpacityProperty(iter.Key(), value->mOpacity));
+            wr::ToWrOpacityProperty(iter.Key(), value->Opacity()));
       }
     }
   }