Bug 1356941 - Use IntermediateRGBA to store overflowed RGBA components during interpolation. r?birtles draft
authorHiroyuki Ikezoe <hikezoe@mozilla.com>
Mon, 24 Apr 2017 15:03:42 +0900
changeset 566920 b8e21d6c40015f69e644573f9461916a3fb69744
parent 566919 8a181e3323c585c8d9084c2a0e7a89c9648c533b
child 566921 0f1e6c9ef32ea7bb043fb5fa5fe312145ba90457
push id55379
push userhikezoe@mozilla.com
push dateMon, 24 Apr 2017 06:04:15 +0000
reviewersbirtles
bugs1356941
milestone55.0a1
Bug 1356941 - Use IntermediateRGBA to store overflowed RGBA components during interpolation. r?birtles MozReview-Commit-ID: 6xGASRUPXH9
servo/components/style/properties/data.py
servo/components/style/properties/helpers/animated_properties.mako.rs
servo/components/style/properties/longhand/color.mako.rs
--- a/servo/components/style/properties/data.py
+++ b/servo/components/style/properties/data.py
@@ -173,23 +173,21 @@ class Longhand(object):
         # > but does accept the `animation-play-state` property and interprets it specially.
         self.allowed_in_keyframe_block = allowed_in_keyframe_block \
             and allowed_in_keyframe_block != "False"
 
         # This is done like this since just a plain bool argument seemed like
         # really random.
         if animation_value_type is None:
             raise TypeError("animation_value_type should be specified for (" + name + ")")
-        animation_value_types = ["none", "discrete", "ComputedValue"]
-        if animation_value_type not in animation_value_types:
-            raise TypeError("animation_value_type should be one of (" +
-                            str(animation_value_types) + ")")
         self.animation_value_type = animation_value_type
 
         self.animatable = animation_value_type != "none"
+        self.is_animatable_with_computed_value = animation_value_type == "ComputedValue" or \
+                                                 animation_value_type == "discrete"
         if self.logical:
             # Logical properties will be animatable (i.e. the animation type is
             # discrete). For now, it is still non-animatable.
             self.animatable = False
             self.animation_type = None
         # NB: Animatable implies clone because a property animation requires a
         # copy of the computed value.
         #
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -379,17 +379,21 @@ pub type AnimationValueMap = HashMap<Tra
 /// FIXME: We need to add a path for custom properties, but that's trivial after
 /// this (is a similar path to that of PropertyDeclaration).
 #[derive(Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub enum AnimationValue {
     % for prop in data.longhands:
         % if prop.animatable:
             /// ${prop.name}
-            ${prop.camel_case}(longhands::${prop.ident}::computed_value::T),
+            % if prop.is_animatable_with_computed_value:
+                ${prop.camel_case}(longhands::${prop.ident}::computed_value::T),
+            % else:
+                ${prop.camel_case}(${prop.animation_value_type}),
+            % endif
         % endif
     % endfor
 }
 
 impl AnimationValue {
     /// "Uncompute" this animation value in order to be used inside the CSS
     /// cascade.
     pub fn uncompute(&self) -> PropertyDeclaration {
@@ -397,17 +401,23 @@ impl AnimationValue {
         match *self {
             % for prop in data.longhands:
                 % if prop.animatable:
                     AnimationValue::${prop.camel_case}(ref from) => {
                         PropertyDeclaration::${prop.camel_case}(
                             % if prop.boxed:
                                 Box::new(longhands::${prop.ident}::SpecifiedValue::from_computed_value(from)))
                             % else:
-                                longhands::${prop.ident}::SpecifiedValue::from_computed_value(from))
+                                longhands::${prop.ident}::SpecifiedValue::from_computed_value(
+                                % if prop.is_animatable_with_computed_value:
+                                    from
+                                % else:
+                                    &from.into()
+                                % endif
+                                ))
                             % endif
                     }
                 % endif
             % endfor
         }
     }
 
     /// Construct an AnimationValue from a property declaration
@@ -421,17 +431,23 @@ impl AnimationValue {
             % for prop in data.longhands:
             % if prop.animatable:
             PropertyDeclaration::${prop.camel_case}(ref val) => {
             % if prop.ident in SYSTEM_FONT_LONGHANDS and product == "gecko":
                 if let Some(sf) = val.get_system() {
                     longhands::system_font::resolve_system_font(sf, context);
                 }
             % endif
-                Some(AnimationValue::${prop.camel_case}(val.to_computed_value(context)))
+                Some(AnimationValue::${prop.camel_case}(
+                % if prop.is_animatable_with_computed_value:
+                    val.to_computed_value(context)
+                % else:
+                    From::from(&val.to_computed_value(context))
+                % endif
+                ))
             },
             % endif
             % endfor
             PropertyDeclaration::CSSWideKeyword(id, keyword) => {
                 match id {
                     // We put all the animatable properties first in the hopes
                     // that it might increase match locality.
                     % for prop in data.longhands:
@@ -449,16 +465,19 @@ impl AnimationValue {
                                 CSSWideKeyword::Unset |
                             % endif
                             CSSWideKeyword::Inherit => {
                                 let inherit_struct = context.inherited_style
                                                             .get_${prop.style_struct.name_lower}();
                                 inherit_struct.clone_${prop.ident}()
                             },
                         };
+                        % if not prop.is_animatable_with_computed_value:
+                            let computed = From::from(&computed);
+                        % endif
                         Some(AnimationValue::${prop.camel_case}(computed))
                     },
                     % endif
                     % endfor
                     % for prop in data.longhands:
                     % if not prop.animatable:
                     LonghandId::${prop.camel_case} => None,
                     % endif
@@ -509,17 +528,22 @@ impl AnimationValue {
                                 computed_values: &ComputedValues)
                                 -> Self {
         match *transition_property {
             TransitionProperty::All => panic!("Can't use TransitionProperty::All here."),
             % for prop in data.longhands:
                 % if prop.animatable:
                     TransitionProperty::${prop.camel_case} => {
                         AnimationValue::${prop.camel_case}(
+                        % if prop.is_animatable_with_computed_value:
                             computed_values.get_${prop.style_struct.ident.strip("_")}().clone_${prop.ident}())
+                        % else:
+                            From::from(&computed_values.get_${prop.style_struct.ident.strip("_")}()
+                                                                  .clone_${prop.ident}()))
+                        % endif
                     }
                 % endif
             % endfor
             other => panic!("Can't use TransitionProperty::{:?} here.", other),
         }
     }
 }
 
@@ -2036,16 +2060,35 @@ impl<T, U> Interpolate for Either<T, U>
             _ => {
                 let interpolated = if progress < 0.5 { *self } else { *other };
                 Ok(interpolated)
             }
         }
     }
 }
 
+impl <'a> From<<&'a IntermediateRGBA> for RGBA {
+    fn from(extended_rgba: &IntermediateRGBA) -> RGBA {
+        // RGBA::from_floats clamps each component values.
+        RGBA::from_floats(extended_rgba.red,
+                          extended_rgba.green,
+                          extended_rgba.blue,
+                          extended_rgba.alpha)
+    }
+}
+
+impl <'a> From<<&'a RGBA> for IntermediateRGBA {
+    fn from(rgba: &RGBA) -> IntermediateRGBA {
+        IntermediateRGBA::new(rgba.red_f32(),
+                              rgba.green_f32(),
+                              rgba.blue_f32(),
+                              rgba.alpha_f32())
+    }
+}
+
 #[derive(Copy, Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 /// Unlike RGBA, each component value may exceed the range [0.0, 1.0].
 pub struct IntermediateRGBA {
     /// The red component.
     pub red: f32,
     /// The green component.
     pub green: f32,
--- a/servo/components/style/properties/longhand/color.mako.rs
+++ b/servo/components/style/properties/longhand/color.mako.rs
@@ -3,17 +3,18 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 <%namespace name="helpers" file="/helpers.mako.rs" />
 
 <% data.new_style_struct("Color", inherited=True) %>
 
 <% from data import to_rust_ident %>
 
-<%helpers:longhand name="color" need_clone="True" animation_value_type="ComputedValue"
+<%helpers:longhand name="color" need_clone="True"
+                   animation_value_type="IntermediateRGBA"
                    spec="https://drafts.csswg.org/css-color/#color">
     use cssparser::RGBA;
     use std::fmt;
     use style_traits::ToCss;
     use values::HasViewportPercentage;
     use values::specified::{Color, CSSColor, CSSRGBA};
 
     impl ToComputedValue for SpecifiedValue {