I'm currently trying to work out what to do with shorthand values. SMIL currently just stores them in a StyleAnimationValue as a string and then fails to interpolate them. For the following test case we fall back to discrete animation (as required by SMIL when interpolation fails): <svg> <text dy=1em id=text>Abc</text> <animate href="#text" dur=2s attributeName=font to="40px serif" fill="freeze"/> </svg> Neither Chrome nor Safari animate this. However, we have a couple of tests that rely on animating shorthands (using discrete interpolation). I think we could do one of the following: a) Use a PropertyDeclarationBlock for shorthands and do discrete interpolation for them b) Expand shorthands and store an array of AnimationValue objects and interpolate each pair c) Deprecate animating shorthands in SMIL (b) Is probably my ideal option. It would likely involve building up a Servo PropertyDeclarationBlock in nsSMILCSSValueType::ValueToString using the interpolated longhand components and then calling Servo_DeclarationBlock_SerializeOneValue much like we do in KeyframeEffectReadOnly::GetKeyframes. It will take a bit of work, however, to make everything in nsSMILCSSValueType and nsSMILCSSProperty able to deal with an array of values. (c) is also an attractive option since it would actually improve interop. For now, I'll leave dealing with this to a separate bug where we either deprecate SMIL animation of shorthand properties in the Gecko code path, or do (b) from above.

Comment on attachment 8861751 [details] Bug 1355348 - Make Servo_NoteExplicitHints allow multiple animation restyle hints, including SMIL; https://reviewboard.mozilla.org/r/133744/#review136620 ::: servo/ports/geckolib/glue.rs:1799 (Diff revision 1) > + debug_assert!(!restyle_hint.intersects(RestyleHint::for_animations()) || > + !restyle_hint.intersects(!RestyleHint::for_animations()), I think RestyleHint::for_animations().contains(restyle_hint) should work.

Comment on attachment 8861755 [details] Bug 1355348 - Move ServoComputedValuesWithParent to separate file; https://reviewboard.mozilla.org/r/133752/#review136622

Comment on attachment 8861756 [details] Bug 1355348 - Add constructor to AnimationValue; https://reviewboard.mozilla.org/r/133754/#review136626

Comment on attachment 8861748 [details] Bug 1355348 - Add SMIL override cascade level; https://reviewboard.mozilla.org/r/133738/#review136618 ::: servo/components/style/rule_tree/mod.rs:268 (Diff revision 1) > for node in iter { > - if node.cascade_level() != CascadeLevel::Animations && > + if node.cascade_level() != CascadeLevel::SMILOverride && > + node.cascade_level() != CascadeLevel::Animations && > node.cascade_level() != CascadeLevel::Transitions { > children.push((node.get().source.clone().unwrap(), node.cascade_level())); > } > last = node; > } Can we add a CascadeLevel::is_animation function for checking these three values? ::: servo/components/style/rule_tree/mod.rs:789 (Diff revision 1) > - .any(|node| matches!(node.cascade_level(), CascadeLevel::Animations | CascadeLevel::Transitions)) > + .any(|node| matches!(node.cascade_level(), > + CascadeLevel::SMILOverride | > + CascadeLevel::Animations | > + CascadeLevel::Transitions)) And we could use that function here too, in place of matches!.

Comment on attachment 8861749 [details] Bug 1355348 - Rename remove_animation_and_transition_rules to just remove_animation_rules; https://reviewboard.mozilla.org/r/133740/#review136628 I almost suggested this myself, on the previous patch, but convinced myself that in "animations_or_transitions" the "animations" could cover both CSS Animations and SMIL Animations. :-)

Comment on attachment 8861750 [details] Bug 1355348 - Add restyle hint for SMIL animations; https://reviewboard.mozilla.org/r/133742/#review136630 ::: servo/components/style/matching.rs:1082 (Diff revision 1) > + if hint.contains(RESTYLE_SMIL) { > + replace_rule_node(CascadeLevel::SMILOverride, > + self.get_smil_override(), > + primary_rules); > + } > + Nit: I would move this down below the replace_rule_node_for_animation declaration, since that function feels more like a prelude for the real work of the function, which begins with replacing the SMIL rule node. ::: servo/components/style/restyle_hints.rs:110 (Diff revision 1) > pub fn for_self() -> Self { > - RESTYLE_SELF | RESTYLE_STYLE_ATTRIBUTE | RESTYLE_CSS_ANIMATIONS | RESTYLE_CSS_TRANSITIONS > + RESTYLE_SELF | RESTYLE_STYLE_ATTRIBUTE | RESTYLE_SMIL | RESTYLE_CSS_ANIMATIONS | > + RESTYLE_CSS_TRANSITIONS > } > > /// The subset hints that are used for animation restyle. > pub fn for_animations() -> Self { > - RESTYLE_CSS_ANIMATIONS | RESTYLE_CSS_TRANSITIONS > + RESTYLE_SMIL | RESTYLE_CSS_ANIMATIONS | RESTYLE_CSS_TRANSITIONS > } While you're here, consider adding #[inline] to these, and making for_self() be: RESTYLE_SELF | RESTYLE_STYLE_ATTRIBUTE | Self::for_animations()

Comment on attachment 8861757 [details] Bug 1355348 - Factor out method to remove minus sign from property values; https://reviewboard.mozilla.org/r/133756/#review136632 ::: dom/smil/nsSMILCSSValueType.cpp:339 (Diff revision 1) > } > nsIPresShell* shell = doc->GetShell(); > return shell ? shell->GetPresContext() : nullptr; > } > > -// Helper function to parse a string into a StyleAnimationValue > +static nsDependentSubstring const nsDependentSubstring? ::: dom/smil/nsSMILCSSValueType.cpp:381 (Diff revision 1) > if (!styleContext) { > return false; > } > - nsDependentSubstring subString(aString, subStringBegin); > + > + bool isNegative = false; > + nsDependentSubstring subString = Likewise.

Comment on attachment 8861752 [details] Bug 1355348 - Add FFI for getting SMIL override styles; https://reviewboard.mozilla.org/r/133746/#review136634

Comment on attachment 8861753 [details] Bug 1355348 - Move IsFlattenedTreeDescendantOf helper to nsContentUtils; https://reviewboard.mozilla.org/r/133748/#review136636

Comment on attachment 8861759 [details] Bug 1355348 - Convert stylo assert in nsComputedDOMStyle::DoGetStyleContextNoFlush to a warning; https://reviewboard.mozilla.org/r/133760/#review136638 ::: commit-message-3370a:3 (Diff revision 1) > +This allows us to run SMIL tests that include a combination of additive and > +non-additive animation. Yay!

Comment on attachment 8861754 [details] Bug 1355348 - Add SMIL restyles in the stylo pretraverse; https://reviewboard.mozilla.org/r/133750/#review136642 ::: dom/smil/nsSMILAnimationController.cpp:736 (Diff revision 1) > + return PreTraverseInSubtree(nullptr); > +} > + > +bool > +nsSMILAnimationController::PreTraverseInSubtree(Element* aRoot) > +{ Let's assert we're on the main thread, like EffectCompositor::PreTraverseInSubtree does.

Comment on attachment 8861758 [details] Bug 1355348 - Make nsSMILCSSValueType store an AnimationValue instead of a StyleAnimationValue; https://reviewboard.mozilla.org/r/133758/#review136646 ::: dom/smil/nsSMILCSSProperty.cpp:70 (Diff revision 1) > + MOZ_ASSERT(mBaseStyleContext->StyleSource().IsServoComputedValues(), > + "Base style context should have servo style source"); > + const ServoComputedValues* currentStyle = > + mBaseStyleContext->StyleSource().AsServoComputedValues(); The AsServoComputedValues() call will assert, so I think we can just skip the assertion here, unless you think it adds more context for the failure. ::: dom/smil/nsSMILCSSProperty.cpp:74 (Diff revision 1) > + computedValue.mServo = > + Servo_ComputedValues_ExtractAnimationValue(currentStyle, mPropID) > + .Consume(); Do we need to check whether this return null, and return baseValue if so? ::: dom/smil/nsSMILCSSValueType.cpp:51 (Diff revision 1) > { > - static const StyleAnimationValue > + static const AnimationValue sZeroCoord( Maybe add a comment in here saying that we just initialize these with StyleAnimationValues, since we don't support additive animation in stylo yet? ::: dom/smil/nsSMILCSSValueType.cpp:469 (Diff revision 1) > + // Get a suitable style context for Servo > + const ServoComputedValues* currentStyle = > + aStyleContext->StyleSource().AsServoComputedValues(); > + const ServoComputedValues* parentStyle = > + aStyleContext->GetParentAllowServo() > + ? aStyleContext->GetParentAllowServo()->StyleSource() > + .AsServoComputedValues() > + : nullptr; > + const ServoComputedValuesWithParent servoStyles = > + { currentStyle, parentStyle }; GetParentAllowServo() will go away at some point. If there is a bug on file for it, please mention it here. ::: dom/smil/nsSMILCSSValueType.cpp:512 (Diff revision 1) > + Keyframe keyframe; > + keyframe.mPropertyValues.AppendElement(Move(propValuePair)); > + AutoTArray<Keyframe, 1> keyframes; > + keyframes.AppendElement(Move(keyframe)); What about: AutoTArray<Keyframe, 1> keyframes; keyframes.AppendElement()->mPropertyValues.AppendElement(...); to avoid copying the Keyframe. ::: dom/smil/nsSMILCSSValueType.cpp:631 (Diff revision 1) > bool > nsSMILCSSValueType::ValueToString(const nsSMILValue& aValue, > nsAString& aString) > { > MOZ_ASSERT(aValue.mType == &nsSMILCSSValueType::sSingleton, > "Unexpected SMIL value type"); > const ValueWrapper* wrapper = ExtractValueWrapper(aValue); > - return !wrapper || > - StyleAnimationValue::UncomputeValue(wrapper->mPropID, > - wrapper->mCSSValue, aString); > + if (!wrapper) { > + return true; > + } > + > + wrapper->mCSSValue.SerializeSpecifiedValue(wrapper->mPropID, aString); > + return true; > } Make this return void since it always returns true now?

Comment on attachment 8861760 [details] Bug 1355348 - Don't return early when sampling SMIL animations with Servo backend; https://reviewboard.mozilla.org/r/133762/#review136652

Comment on attachment 8861747 [details] Bug 1355348 - Drop extra eRestyle_StyleAttribute hint from Element::SetSMILOverrideStyleDeclaration; https://reviewboard.mozilla.org/r/133736/#review136660

Comment on attachment 8861751 [details] Bug 1355348 - Make Servo_NoteExplicitHints allow multiple animation restyle hints, including SMIL; https://reviewboard.mozilla.org/r/133744/#review136620 > I think RestyleHint::for_animations().contains(restyle_hint) should work. Yeah. We still want to test the negative version, though, i.e. RestyleHint::for_animations().contains(restyle_hint) || !RestyleHint::for_animations().intersects(restyle_hint)

Comment on attachment 8861750 [details] Bug 1355348 - Add restyle hint for SMIL animations; https://reviewboard.mozilla.org/r/133742/#review137070 ::: servo/components/style/matching.rs:1082 (Diff revision 1) > + if hint.contains(RESTYLE_SMIL) { > + replace_rule_node(CascadeLevel::SMILOverride, > + self.get_smil_override(), > + primary_rules); > + } > + Oh really, I thought the opposite :) My reading of this function is: 1. Do SMIL replacements 2. Define helper function for dealing with CSS Transitions and CSS Animations 3. Do CSS Transition replacements 4. Do CSS Animation replacements Since (2) is tightly coupled with (3) and (4) they should stick together. Sticking SMIL inbetween seems odd to me?

Comment on attachment 8861758 [details] Bug 1355348 - Make nsSMILCSSValueType store an AnimationValue instead of a StyleAnimationValue; https://reviewboard.mozilla.org/r/133758/#review137090 ::: dom/smil/nsSMILCSSValueType.cpp:51 (Diff revision 1) > { > - static const StyleAnimationValue > + static const AnimationValue sZeroCoord( This is a Gecko-specific method since it takes a StyleAnimationValue::Unit parameter. I went to add a comment but in light of that I think it makes it more confusing.

Comment on attachment 8861750 [details] Bug 1355348 - Add restyle hint for SMIL animations; https://reviewboard.mozilla.org/r/133742/#review137070 > Oh really, I thought the opposite :) > > My reading of this function is: > > 1. Do SMIL replacements > 2. Define helper function for dealing with CSS Transitions and CSS Animations > 3. Do CSS Transition replacements > 4. Do CSS Animation replacements > > Since (2) is tightly coupled with (3) and (4) they should stick together. Sticking SMIL inbetween seems odd to me? I can see it from both angles. :-) Anyway, fine to leave it as is, thanks.

Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dc594f991dc2 Add FFI for getting SMIL override styles; r=heycam https://hg.mozilla.org/integration/autoland/rev/5881280a872d Move IsFlattenedTreeDescendantOf helper to nsContentUtils; r=heycam https://hg.mozilla.org/integration/autoland/rev/f4de7213d853 Add SMIL restyles in the stylo pretraverse; r=heycam https://hg.mozilla.org/integration/autoland/rev/a1e5a034eefb Move ServoComputedValuesWithParent to separate file; r=hiro https://hg.mozilla.org/integration/autoland/rev/7dbc4fb20502 Add constructor to AnimationValue; r=hiro https://hg.mozilla.org/integration/autoland/rev/e5f624959d3d Factor out method to remove minus sign from property values; r=hiro https://hg.mozilla.org/integration/autoland/rev/d3553e38c07d Make nsSMILCSSValueType store an AnimationValue instead of a StyleAnimationValue; r=heycam https://hg.mozilla.org/integration/autoland/rev/f1bd37850558 Convert stylo assert in nsComputedDOMStyle::DoGetStyleContextNoFlush to a warning; r=hiro https://hg.mozilla.org/integration/autoland/rev/33f064e9137d Don't return early when sampling SMIL animations with Servo backend; r=heycam

Yay!

Oh, that hazard failure doesn't look good though: https://treeherder.mozilla.org/logviewer.html#?job_id=94751733&repo=autoland&lineNumber=23837

This is the least risky patch I can think of to fix the hazard bustage. I would push this to autoland but it is currently closed.

Pushed by kwierso@gmail.com: https://hg.mozilla.org/mozilla-central/rev/32598e58ad1f Fix hazard bustage by duplicating code in Gecko_GetStyleAttrDeclarationBlock and Gecko_GetSMILOverrideDeclarationBlock on a CLOSED TREE; r=me a=bustage-fix

https://hg.mozilla.org/mozilla-central/rev/dc594f991dc2 https://hg.mozilla.org/mozilla-central/rev/5881280a872d https://hg.mozilla.org/mozilla-central/rev/f4de7213d853 https://hg.mozilla.org/mozilla-central/rev/a1e5a034eefb https://hg.mozilla.org/mozilla-central/rev/7dbc4fb20502 https://hg.mozilla.org/mozilla-central/rev/e5f624959d3d https://hg.mozilla.org/mozilla-central/rev/d3553e38c07d https://hg.mozilla.org/mozilla-central/rev/f1bd37850558 https://hg.mozilla.org/mozilla-central/rev/33f064e9137d https://hg.mozilla.org/mozilla-central/rev/32598e58ad1f

Pushed by bbirtles@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8900bc3ec0 Drop extra eRestyle_StyleAttribute hint from Element::SetSMILOverrideStyleDeclaration; r=dbaron

I think when I split the patch series into Servo patches and Gecko patches this first patch got left out somehow.