layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-hex.svg fails on Stylo. This test code is as follows: var animAttrHash = { "attributeName" : "fill", "by" : "#AAF573" }; testAnimatedRectGrid("animate", [animAttrHash]); Adding some loggingto the code I see: Parsed fill property value '#AAF573' => rgb(170, 245, 115) Failed to get zero_value That is, the call to get_zero_value in Servo_AnimationValues_GetZeroValue is producing none. I guess we need to implement get_zero_value on IntermediateSVGPaint.

(In reply to Brian Birtles (:birtles) from comment #0) > layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-hex.svg fails on > Stylo. > > This test code is as follows: > > var animAttrHash = { "attributeName" : "fill", > "by" : "#AAF573" }; > testAnimatedRectGrid("animate", [animAttrHash]); > > Adding some loggingto the code I see: > > Parsed fill property value '#AAF573' => rgb(170, 245, 115) > Failed to get zero_value > > That is, the call to get_zero_value in Servo_AnimationValues_GetZeroValue is > producing none. > > I guess we need to implement get_zero_value on IntermediateSVGPaint. I tried to add the get_zero_value of IntermediateSVGPaint, however it didn't pass this test case. If we return zero value of IntermediateSVGPaint, stylo will call 'accumulate' with zero value and specified value with 'by'. Brian, I think that we will animation as accumulate from current color(i.e. in this test, Indigo) to added specified color(i.e. Indigo + #AAF573) in this case. Is my understand correct?

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1) > (In reply to Brian Birtles (:birtles) from comment #0) > > layout/reftests/svg/smil/style/anim-css-fill-1-by-ident-hex.svg fails on > > Stylo. > > > > This test code is as follows: > > > > var animAttrHash = { "attributeName" : "fill", > > "by" : "#AAF573" }; > > testAnimatedRectGrid("animate", [animAttrHash]); > > > > Adding some loggingto the code I see: > > > > Parsed fill property value '#AAF573' => rgb(170, 245, 115) > > Failed to get zero_value > > > > That is, the call to get_zero_value in Servo_AnimationValues_GetZeroValue is > > producing none. > > > > I guess we need to implement get_zero_value on IntermediateSVGPaint. > > I tried to add the get_zero_value of IntermediateSVGPaint, however it didn't > pass this test case. > If we return zero value of IntermediateSVGPaint, stylo will call > 'accumulate' with zero value and specified value with 'by'. > > Brian, > I think that we will animation as accumulate from current color(i.e. in this > test, Indigo) to added specified color(i.e. Indigo + #AAF573) in this case. > Is my understand correct? I discussed with Brian, I added the log into geckolib/glue.rs, maybe my modification is wrong. I added following get_zero_value function into IntermediateSVGPaint: ------------------------------------------------------------- fn get_zero_value(&self) -> Option<Self> { Some(IntermediateSVGPaint { kind: self.kind.get_zero_value().unwrap(), fallback: Some(IntermediateRGBA::transparent()), }) } ------------------------------------------------------------- However, specified value with 'by' don't contain the fallback(i.e. fallback is None), So we can't add() operation with zero value and specified value. We will need to following get_zero_value function: ------------------------------------------------------------- fn get_zero_value(&self) -> Option<Self> { Some(IntermediateSVGPaint { kind: self.kind.get_zero_value().unwrap(), fallback: None, }) } -------------------------------------------------------------

Better still, we should match |self|. If |self| contains a fallback, we should fill in fallback with a zero value, otherwise we make it None.

Thank you for your feedback. (In reply to Brian Birtles (:birtles) from comment #4) > Better still, we should match |self|. If |self| contains a fallback, we > should fill in fallback with a zero value, otherwise we make it None. OK. I didn't care the containing the fallback value. I've added these fallback matching: https://treeherder.mozilla.org/#/jobs?repo=try&revision=edf64f95ac6a727f717ec9099eddf073400e7eb0 (In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=e012a4f63f4cfcfb9bb3d594435a7ad5636f774c Two test failed. 1) layout/reftests/svg/smil/style/anim-css-fill-overflow-1-by.svg -> It will be test success, we can remove test fail annotation as well. 2) layout/reftests/svg/smil/anim-paintserver-1.svg -> It willbe test fail. If we run following test-case, gecko render lime rectange, however stylo didn't render anything. I added log into geckolib/glue.rs(Servo_AnimationValues_Interpolate/Servo_AnimationValues_Add/Servo_AnimationValues_Accumulate), but any functions didn't called. ---------------------------------------- <svg xmlns="http://www.w3.org/2000/svg"> <defs> <linearGradient id="lime"> <stop offset="0.0" stop-color="lime"/> </linearGradient> </defs> <rect x="0" y="0" width="100" height="100" fill="url(#lime)"> <animate attributeName="fill" to="pikapikaglittergold"/> </rect> </svg> ----------------------------------------

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review155384 Looks good but I'd like to check again with the get_zero_value for Option defined, assuming that works and you agree it makes sense. ::: commit-message-72346:1 (Diff revision 1) > +Bug 1371199 - Add get_zero_value of IntermediateSVGPaint. r?birtles Nit: s/of/for/ ::: servo/components/style/properties/helpers/animated_properties.mako.rs:2991 (Diff revision 1) > + fallback: match self.fallback { > + Some(_) => Some(IntermediateRGBA::transparent()), > + None => None, > + }, Could we just define get_zero_value for Option where we have: impl <T> Animatable for Option<T> where T: Animatable, and then use that here? ::: servo/components/style/properties/helpers/animated_properties.mako.rs:2997 (Diff revision 1) > + Some(_) => Some(IntermediateRGBA::transparent()), > + None => None, > + }, > + }) > + } > + Nit: no need for the whitespace here ::: servo/components/style/properties/helpers/animated_properties.mako.rs:3035 (Diff revision 1) > + &SVGPaintKind::None => { > + Some(SVGPaintKind::None) > + }, > + &SVGPaintKind::ContextFill => { > + Some(SVGPaintKind::ContextFill) > + }, > + &SVGPaintKind::ContextStroke => { > + Some(SVGPaintKind::ContextStroke) > + }, I don't know much about rust but it seems like there's probably a way to right this more simply. e.g. &SVGPaintKind::None | &SVGPaintKind::ContextFill | &SVGPaintKind::ContextStroke => Some(self.clone()), ? (There might be an even better way still)

Comment on attachment 8879358 [details] Bug 1371199 - Update Stylo test expectations for reftests that use by-animation of 'fill'. https://reviewboard.mozilla.org/r/150654/#review155396 ::: commit-message-b8ac7:1 (Diff revision 1) > +Bug 1371199 - Enable reftests of SVG related with 'by' on stylo. r?birtles Update Stylo test expectations for reftests that use by-animation of 'fill'

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review155952 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:2998 (Diff revision 2) > } > + > + #[inline] > + fn get_zero_value(&self) -> Option<Self> { > + Some(IntermediateSVGPaint { > + kind: self.kind.get_zero_value().unwrap(), This doesn't seem right. get_zero_value for IntermediateSVGPaintKind can return None in which case unwrap() will panic. I think you can use `kind: option_try!(self.kind.get_zero_value())` here. ::: servo/components/style/properties/helpers/animated_properties.mako.rs:2999 (Diff revision 2) > + // get_zero_value trait return Option<Self>, So if we call > + // Option's get_zero_value, return value will be > + // Option<Option<T>> since Self is Option<T>. > + fallback: self.fallback.get_zero_value().unwrap_or(None), Will rust actually complain if we change the type of get_zero_value() for Option to Self? (i.e. not Option<Self>)? If it does complain, perhaps we should just call get_zero_value on Option something else, like get_unwrapped_zero_value (get_inner_zero_value?), make it return Self, and call that here -- rather than doing the extra wrap/unwrap dance.

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review155970 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:891 (Diff revision 2) > + match self { > + &Some(ref value) => Some(value.get_zero_value()), > + &None => None, > + } > + } We can just use map().

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review155972 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:891 (Diff revision 2) > + match self { > + &Some(ref value) => Some(value.get_zero_value()), > + &None => None, > + } > + } Oh, nvm. this returns Option<Option<Self>>?

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review155974 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:891 (Diff revision 2) > + match self { > + &Some(ref value) => Some(value.get_zero_value()), > + &None => None, > + } > + } well, Option<Option<*inner*Self>>...

Yeah, this is all a bit odd because get_zero_value should probably return Result instead of Option, but I'm suggesting for Option we create a separate method that just returns Option<*inner*Self> and call that.

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review155952 Thanks a lot. I updated the patch, Could you please review it again?

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review155970 > We can just use map(). I created the get_inner_zero_value instead of it.

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #21) > Comment on attachment 8879357 [details] > Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/150652/diff/3-4/ I discussed with Brian, it is better to unwrap Option<Option<Self>> at get_zero_value of IntermediateSVGPaint than creating a new trait. So I updated the patch.

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #24) > Comment on attachment 8879357 [details] > Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/150652/diff/4-5/ Sorry, I addressed wrong matching.

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review156568 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:2991 (Diff revision 5) > + > + #[inline] > + fn get_zero_value(&self) -> Option<Self> { > + Some(IntermediateSVGPaint { > + kind: option_try!(self.kind.get_zero_value()), > + fallback: option_try!(self.fallback.map(|v| v.get_zero_value())), This isn't right. If fallback is None, this will make the whole function return None which is not what we want. I suspect you want to just do: fallback: self.fallback.map(|v| v.get_zero_value().unwrap())

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review156568 > This isn't right. If fallback is None, this will make the whole function return None which is not what we want. > > I suspect you want to just do: > > fallback: self.fallback.map(|v| v.get_zero_value().unwrap()) I'm sorry to bother you.. I updated the patch. Thanks.

Comment on attachment 8879357 [details] Bug 1371199 - Add get_zero_value for IntermediateSVGPaint. https://reviewboard.mozilla.org/r/150652/#review156576

Try: Stylo : https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a2b6303da7f4ab1d38fbb8e9bb45f11a927ca2d Gecko(Test only) : https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2e94dff4371fad98226810c547ef89483e7d6a1

Pushed by mantaroh@gmail.com: https://hg.mozilla.org/integration/autoland/rev/61a32c9dfaf1 Update Stylo test expectations for reftests that use by-animation of 'fill'. r=birtles