Bug 1540581 - P7. Use Variant instead of Union/Enum. r=kats
authorJean-Yves Avenard <jyavenard@mozilla.com>
Thu, 11 Apr 2019 12:37:06 +0000
changeset 469135 cc37bdabd566f3b0e29be2c4bc00eacd585aed0d
parent 469134 884f5dc55873c9345ee945cd6d5427e8ab398071
child 469136 d2b6cc14a04347d2b69015c0dc08bb7718863ba3
push id35856
push usercsabou@mozilla.com
push dateFri, 12 Apr 2019 03:19:48 +0000
treeherdermozilla-central@940684cd1065 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats
bugs1540581
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
gfx/layers/AnimationHelper.cpp
gfx/layers/AnimationHelper.h
gfx/layers/composite/AsyncCompositionManager.cpp
gfx/layers/wr/WebRenderBridgeParent.cpp
--- a/gfx/layers/AnimationHelper.cpp
+++ b/gfx/layers/AnimationHelper.cpp
@@ -45,62 +45,52 @@ AnimatedValue* CompositorAnimationStorag
 
 OMTAValue CompositorAnimationStorage::GetOMTAValue(const uint64_t& aId) const {
   OMTAValue omtaValue = mozilla::null_t();
   auto animatedValue = GetAnimatedValue(aId);
   if (!animatedValue) {
     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);
 
-      // 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;
-  }
-
+        // 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;
 }
 
 void CompositorAnimationStorage::SetAnimatedValue(
     uint64_t aId, gfx::Matrix4x4&& aTransformInDevSpace,
     gfx::Matrix4x4&& aFrameTransform, const TransformData& aData) {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
   auto count = mAnimatedValues.Count();
   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);
   }
 }
 
 void CompositorAnimationStorage::SetAnimatedValue(
     uint64_t aId, gfx::Matrix4x4&& aTransformInDevSpace) {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
   const TransformData dontCare = {};
   SetAnimatedValue(aId, std::move(aTransformInDevSpace), gfx::Matrix4x4(),
@@ -108,29 +98,29 @@ void CompositorAnimationStorage::SetAnim
 }
 
 void CompositorAnimationStorage::SetAnimatedValue(uint64_t aId,
                                                   nscolor aColor) {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
   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);
   }
 }
 
 void CompositorAnimationStorage::SetAnimatedValue(uint64_t aId,
                                                   const float& aOpacity) {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
   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);
   }
 }
 
 nsTArray<PropertyAnimationGroup>* CompositorAnimationStorage::GetAnimations(
     const uint64_t& aId) const {
   MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread());
   return mAnimations.Get(aId);
 }
--- a/gfx/layers/AnimationHelper.h
+++ b/gfx/layers/AnimationHelper.h
@@ -8,16 +8,17 @@
 #define mozilla_layers_AnimationHelper_h
 
 #include "mozilla/dom/Nullable.h"
 #include "mozilla/ComputedTimingFunction.h"    // for ComputedTimingFunction
 #include "mozilla/layers/LayersMessages.h"     // for TransformData, etc
 #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 {
 struct AnimationValue;
 
 namespace dom {
 enum class CompositeOperation : uint8_t;
 enum class IterationCompositeOperation : uint8_t;
@@ -83,45 +84,41 @@ struct AnimationTransform {
    * This transform is calculated from frame and used by getOMTAStyle()
    * for OMTA testing.
    */
   gfx::Matrix4x4 mFrameTransform;
   TransformData mData;
 };
 
 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
 // keyed by a CompositorAnimationsId. The "animations" are a representation of
 // an entire animation over time, while the "animated values" are values sampled
 // from the animations at a particular point in time.
 //
 // There is one CompositorAnimationStorage per CompositorBridgeParent (i.e.
--- a/gfx/layers/composite/AsyncCompositionManager.cpp
+++ b/gfx/layers/composite/AsyncCompositionManager.cpp
@@ -720,31 +720,32 @@ static bool SampleAnimations(Layer* aLay
 #ifdef DEBUG
             const TransformData& transformData =
                 lastPropertyAnimationGroup.mAnimationData.ref();
             Matrix4x4 frameTransform =
                 AnimationHelper::ServoAnimationValueToMatrix4x4(animationValues,
                                                                 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
             // previous shadow base transform value.
             HostLayer* layerCompositor = layer->AsHostLayer();
             layerCompositor->SetShadowBaseTransform(
                 // FIXME: Bug 1459775: It seems possible that we somehow try
                 // to sample animations and skip it even if the previous value
                 // has been discarded from the animation storage when we enable
                 // 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;
           }
           default:
             MOZ_ASSERT_UNREACHABLE("Unsupported properties");
             break;
         }
         break;
--- a/gfx/layers/wr/WebRenderBridgeParent.cpp
+++ b/gfx/layers/wr/WebRenderBridgeParent.cpp
@@ -1844,22 +1844,22 @@ bool WebRenderBridgeParent::SampleAnimat
   // return the animated data if has
   if (mAnimStorage->AnimatedValueCount()) {
     for (auto iter = mAnimStorage->ConstAnimatedValueTableIter(); !iter.Done();
          iter.Next()) {
       AnimatedValue* value = iter.UserData();
       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()));
       }
     }
   }
 
   return isAnimating;
 }
 
 void WebRenderBridgeParent::CompositeIfNeeded() {