Comment on attachment 8877526 [details] Bug 1369625 - Make stop-color, flood-color, lighting-color animatable. https://reviewboard.mozilla.org/r/148980/#review153430 Using IntermediateRGB color for these properties means that additive SMIL animation for them will differ from the ones on Gecko. Is there no test cases that unexpectedly passed? If there is no test cases, it would be nice to have such test cases (fails on Gecko though).

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3) > Comment on attachment 8877526 [details] > Bug 1369625 - Make stop-color, flood-color, lighting-color animatable. > > https://reviewboard.mozilla.org/r/148980/#review153430 > > Using IntermediateRGB color for these properties means that additive SMIL > animation for them will differ from the ones on Gecko. > Is there no test cases that unexpectedly passed? If there is no test cases, > it would be nice to have such test cases (fails on Gecko though). Let's wait for try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e24cda00568e

(In reply to Boris Chiou [:boris] from comment #4) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #3) > > Comment on attachment 8877526 [details] > > Bug 1369625 - Make stop-color, flood-color, lighting-color animatable. > > > > https://reviewboard.mozilla.org/r/148980/#review153430 > > > > Using IntermediateRGB color for these properties means that additive SMIL > > animation for them will differ from the ones on Gecko. > > Is there no test cases that unexpectedly passed? If there is no test cases, > > it would be nice to have such test cases (fails on Gecko though). > > Let's wait for try result: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e24cda00568e I mean unexpectedly passed tests that requires overflowed RGBA values during additive operations. If there are such cases but didn't show up on the try, it means that the patch is something wrong. I found one, but it's for fill property: layout/reftests/svg/smil/style/anim-css-fill-overflow-1-from-by.svg

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > I mean unexpectedly passed tests that requires overflowed RGBA values during > additive operations. > If there are such cases but didn't show up on the try, it means that the > patch is something wrong. > > I found one, but it's for fill property: > layout/reftests/svg/smil/style/anim-css-fill-overflow-1-from-by.svg Thanks, hiro. Maybe I have to write one for {stop,flood,lighting}-color to make sure this.

(In reply to Boris Chiou [:boris] from comment #6) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > > I mean unexpectedly passed tests that requires overflowed RGBA values during > > additive operations. > > If there are such cases but didn't show up on the try, it means that the > > patch is something wrong. > > > > I found one, but it's for fill property: > > layout/reftests/svg/smil/style/anim-css-fill-overflow-1-from-by.svg > > Thanks, hiro. Maybe I have to write one for {stop,flood,lighting}-color to > make sure this. Got an unexpected pass, but it seems to me that it's not related to overflowed RGBA values during additive operations [1] you mentioned. [1] http://searchfox.org/mozilla-central/source/layout/reftests/svg/smil/anim-defs-gradient-property.svg

Right, that one is normal animation.

Comment on attachment 8878377 [details] Bug 1369625 - Update reftest list for SMIL animations. https://reviewboard.mozilla.org/r/149718/#review154380

Comment on attachment 8878377 [details] Bug 1369625 - Update reftest list for SMIL animations. https://reviewboard.mozilla.org/r/149718/#review154380

Comment on attachment 8878378 [details] Bug 1369625 - Add reftest for stop-color and flood-color. https://reviewboard.mozilla.org/r/149720/#review154382 I'd like to defer this to Daniel since he is the original author of anim-css-fill-overflow-1-from-by.svg. Daniel, could you please take time to review this patch. This patch includes two reftests which are ideally equivarent to anim-css-fill-overflow-1-from-by.svg but for stop-color and other colors. Thank you!

try looks good, but got a new hazard write on Gecko_GetNextStyleChild....why!?!?!?!?!?!?!?!!? I will upload an extra patch to add it into whitelist....

My fault, I landed the hazard just now. Should be fixed with this: https://hg.mozilla.org/integration/autoland/rev/8d9bab00cc59

(In reply to Cameron McCormack (:heycam) from comment #19) > My fault, I landed the hazard just now. Should be fixed with this: > https://hg.mozilla.org/integration/autoland/rev/8d9bab00cc59 Ha, Good news. Thanks.

Comment on attachment 8878378 [details] Bug 1369625 - Add reftest for stop-color and flood-color. https://reviewboard.mozilla.org/r/149720/#review154576 Nice usage of smil-grid.js! I like this generally, but there are a number of things that could stand to be changed a bit to be clearer. See below for specific things that were confusing for me when I read through this, & which I'd like to see clarified. ::: layout/reftests/svg/smil/smil-grid.js:98 (Diff revision 3) > +// those specific elements. |defElementList| is a tag list we will create, and > +// the first element is the outer element in |defs|, the last element is the > +// inner element which includes the animation element. |styleAttributeName| is > +// the attribute which uses the url to the reference (i.e. outer element in > +// |defElementList|). The wording in this chunk of documentation is a bit fuzzy, which makes it hard to follow. In particular: - "defElementList is a tag list we will create": taken literally, this statement is clearly wrong. Really, defElementList is an array, and the *caller* creates it and passes to us. (Though it is a compact representation of something that we will create -- that's sort of what you're meaning to say, I think) - "the first element is the outer element" -- I think by "first element" you mean "first tag/entry in defElementList", right? The differing meanings of "element" are hard to follow here. - "|styleAttributeName| is the attribute which uses the url to the reference (i.e. outer element in |defElementList|)" -- just reading the comment, I don't understand at all what this means. This variable is kinda misnamed, too -- see my suggested renamings for this function's parameters below. Please rephrase this documentation to make it clearer (though you probably want to look into the renamings first). ::: layout/reftests/svg/smil/smil-grid.js:104 (Diff revision 3) > +// e.g. > +// defElementList = [ "linearGradient", "stop" ]; will look like: > +// > +// <defs> Please make this example clearer by rephrasing like so: === e.g. if a caller passes a defElementList of [ "linearGradient", "stop" ], this function will generate the following subtree: ::: layout/reftests/svg/smil/smil-grid.js:114 (Diff revision 3) > +function testAnimatedGridWithDefs(targetTagName, targetAttrHash, > + defElementList, styleAttributeName, > + animationTagName, animationAttrHashList) { "targetTagName" and "targetAttrHash" are unfortunately misleading names for parameters in this version of the function, because here, this "target" element is *not* actually targeted by the animation. (Instead, the animation target is the innermost thing in your tag list.) Also, the naming is a bit inconsistent here ("tagName" vs. "Element" both used for tag names; "Attr" vs "Attribute"; and "styleAttributeName" which is really a CSS property name.) I think the following names would be clearer, more consistent, & more self-documenting: graphicElemTagName graphicElemAttrHash graphicElemIdValuedProperty defTagNameList animationTagName animationAttrHashList (And per my ordering here, I think it's better to group the ID-valued property earlier than you've got it (before defTagNameList), so that all of the bits of the <rect> [or whatever] are grouped together.) What do you think? ::: layout/reftests/svg/smil/smil-grid.js:140 (Diff revision 3) > + var outerElement = buildElement(defElementList[0], { "id": "elem" + i }); > + var innerElement = outerElement; > + var tempParent = outerElement; > + for (var defIdx = 1; defIdx < defElementList.length; ++defIdx) { > + innerElement = buildElement(defElementList[defIdx]); > + tempParent.appendChild(innerElement); > + tempParent = innerElement; > + } This is a bit hard to follow, with all the various variables (defs, outerElement, innerElement, tempParent), some of which are sometimes equal and sometimes not. It'd be great to eliminate the redundancy here and make these easier to follow & reason about. Also, the 1-based indexing is a bit unfortunate. It'd be great to eliminate that, too, and just fold the first buildElement call into the loop. I think you can simplify this to the following: // This will track the innermost element in our subtree: var innerElement = defs; for (var defIdx = 0; defIdx < defElementList.length; ++defIdx) { // Set an ID on the first level of nesting (on child of defs): var attrs = defIdx == 0 ? { "id": "elem" + i } : {}; var newElem = buildElement(defElementList[defIdx], attrs); innerElement.appendChild(newElem); innerElement = newElem; } I think that (or something like it) should work and should be a bit easier to follow... ::: layout/reftests/svg/smil/style/reftest.list:56 (Diff revision 3) > +fails-if(!stylo) == anim-css-stopcolor-overflow-1-from-by.svg anim-css-stopcolor-overflow-1-ref.svg > +fails-if(!stylo) == anim-css-floodcolor-overflow-1-from-by.svg anim-css-floodcolor-overflow-1-ref.svg Is it a bug that these fail in non-stylo? Could you include a bug reference? (Or maybe it's just a version of the comment above about "the second test fails..."? If that's the case, please update that comment so it more clearly is covering these tests as well.)

Comment on attachment 8878378 [details] Bug 1369625 - Add reftest for stop-color and flood-color. https://reviewboard.mozilla.org/r/149720/#review154606 ::: layout/reftests/svg/smil/smil-grid.js:104 (Diff revision 3) > +// e.g. > +// defElementList = [ "linearGradient", "stop" ]; will look like: > +// > +// <defs> > +// <linearGradient id="elem0"> > +// <stop> > +// <animate ..../> > +// </stop> > +// </linearGradient> > +// </defs> One more note on this example -- right now, it makes it look like we generate exactly one linearGradient inside of <defs>. But really, we generate a bunch. Probably best to insert something like this before the </defs>, to avoid giving that mistaken impression: // <!--- more similar linearGradients here, up to START_TIMES.length -->

Comment on attachment 8878378 [details] Bug 1369625 - Add reftest for stop-color and flood-color. https://reviewboard.mozilla.org/r/149720/#review154576 > "targetTagName" and "targetAttrHash" are unfortunately misleading names for parameters in this version of the function, because here, this "target" element is *not* actually targeted by the animation. (Instead, the animation target is the innermost thing in your tag list.) > > Also, the naming is a bit inconsistent here ("tagName" vs. "Element" both used for tag names; "Attr" vs "Attribute"; and "styleAttributeName" which is really a CSS property name.) > > I think the following names would be clearer, more consistent, & more self-documenting: > graphicElemTagName > graphicElemAttrHash > graphicElemIdValuedProperty > defTagNameList > animationTagName > animationAttrHashList > > (And per my ordering here, I think it's better to group the ID-valued property earlier than you've got it (before defTagNameList), so that all of the bits of the <rect> [or whatever] are grouped together.) > > What do you think? I see. I will adopt these name and make sure the consistency on them. > This is a bit hard to follow, with all the various variables (defs, outerElement, innerElement, tempParent), some of which are sometimes equal and sometimes not. It'd be great to eliminate the redundancy here and make these easier to follow & reason about. > > Also, the 1-based indexing is a bit unfortunate. It'd be great to eliminate that, too, and just fold the first buildElement call into the loop. > > I think you can simplify this to the following: > > // This will track the innermost element in our subtree: > var innerElement = defs; > > for (var defIdx = 0; defIdx < defElementList.length; ++defIdx) { > // Set an ID on the first level of nesting (on child of defs): > var attrs = defIdx == 0 ? { "id": "elem" + i } : {}; > > var newElem = buildElement(defElementList[defIdx], attrs); > innerElement.appendChild(newElem); > innerElement = newElem; > } > > I think that (or something like it) should work and should be a bit easier to follow... Thanks, this looks much simpler. > Is it a bug that these fail in non-stylo? Could you include a bug reference? > > (Or maybe it's just a version of the comment above about "the second test fails..."? If that's the case, please update that comment so it more clearly is covering these tests as well.) Yes. These tests (including anim-css-fill-overflow-1-from-by.svg) are always failed in Gecko now. The bug is Bug 515919. And this is the case of the above comment. I will update the comment. Thanks.

Comment on attachment 8879011 [details] Bug 1369625 - Update WPT expectations. https://reviewboard.mozilla.org/r/150290/#review154936

Comment on attachment 8878378 [details] Bug 1369625 - Add reftest for stop-color and flood-color. https://reviewboard.mozilla.org/r/149720/#review155196 Thanks! This is much clearer. r=me with nits: ::: layout/reftests/svg/smil/smil-grid.js:94 (Diff revision 4) > setTimeAndSnapshot(SNAPSHOT_TIME, true); > } > > +// Generates a visual grid of elements of type |graphicElemTagName|, with the > +// attribute values given in |graphicElemAttrHash|. This is a variation of the > +// above function. We use |defs| tag to include the reference elements because s/|defs|/<defs>/ (angle brackets instead of vertical bars) (Your other vertical-bar usages here are all to quote variable names. And defs is not a variable name.) There are three instances of |defs| in this comment -- please fix all three. ::: layout/reftests/svg/smil/smil-grid.js:95 (Diff revision 4) > } > > +// Generates a visual grid of elements of type |graphicElemTagName|, with the > +// attribute values given in |graphicElemAttrHash|. This is a variation of the > +// above function. We use |defs| tag to include the reference elements because > +// some animatable properties are only applied to some specific elements s/applied/applicable/ ::: layout/reftests/svg/smil/smil-grid.js:102 (Diff revision 4) > +// |animationTagName|, with the attribute values given in |animationAttrHash|, > +// to those specific elements. |defTagNameList| is an array of tag names. > +// We will create elements hierarchically according to this array. The first tag > +// in |defTagNameList| is the outer-most one in |defs|, and the last tag is the > +// inner-most one and it is the target to which the animation element will be > +// applild. |graphicElemIdValueProperty| is the property of the graphic element typo: s/applild/applied/ ::: layout/reftests/svg/smil/smil-grid.js:102 (Diff revision 4) > +// applild. |graphicElemIdValueProperty| is the property of the graphic element > +// to which we apply by a url reference which is defined in one of the > +// outer-most element in |defs|. This documentation as a whole is much clearer -- thanks! -- but this last sentence is still a bit hard to read, for me at least. (I stumbled several over the nested clauses when trying to read it, particularly over "...to which we apply by a url reference which ...") Maybe replace it with the following two sentences, which I find easier to grok: "We visualize the effect of our animation by referencing each animated subtree from some graphical element that we generate. The |graphicElemIdValueProperty| parameter provides the name of the CSS property that we should use to hook up this reference." ::: layout/reftests/svg/smil/style/reftest.list:49 (Diff revision 4) > fuzzy-if(skiaContent,1,580) fails-if(styloVsGecko||stylo) == anim-css-fill-3-by-ident-ident.svg anim-css-fill-3-ref.svg > fuzzy-if(skiaContent,1,580) == anim-css-fill-3-from-by-ident-ident.svg anim-css-fill-3-ref.svg > fuzzy-if(skiaContent,1,580) == anim-css-fill-3-from-by-rgb-ident.svg anim-css-fill-3-ref.svg > > # check handling of overflowing color values > -# NOTE: The second test fails because we compute "from + by" as the animation > +# NOTE: The second & third & forth tests fail in Gecko because we compute Typo: s/forth/fourth/ ("forth" is a different word which means "forward") Also, if you like (optional): I think it might be a bit more future-proof and readable to just do away with the numeric indices here (second, third, fourth) and instead just start this comment out with "NOTE: Some of the tests below...". And then flag the specific tests you're talking about by adding "# bug 515919" to the end of their line, as is customary for known reftest failures (and that'd tie them back to the longer comment because it's the same bug number mentioned there). This is OK as-is too, though, modulo the "forth" typo-fix.

Comment on attachment 8878378 [details] Bug 1369625 - Add reftest for stop-color and flood-color. https://reviewboard.mozilla.org/r/149720/#review155196 > s/|defs|/<defs>/ (angle brackets instead of vertical bars) > > (Your other vertical-bar usages here are all to quote variable names. And defs is not a variable name.) > > There are three instances of |defs| in this comment -- please fix all three. (Er, I misspoke slightly -- I suppose "defs" *is* a variable-name. What I meant was, it's not a *parameter* name. And in particular, the bit that confused me is the patch's current text here: "We use |defs| tag" That sounds a bit like it's saying |defs| is some variable that contains a tag name (like some of our other params here are). But it's not -- it's a reference to an actual <defs> element. So since you don't need to bother documenting the variable anyway, it seems clearer to just say "<defs>")

Comment on attachment 8878378 [details] Bug 1369625 - Add reftest for stop-color and flood-color. https://reviewboard.mozilla.org/r/149720/#review155196 > (Er, I misspoke slightly -- I suppose "defs" *is* a variable-name. What I meant was, it's not a *parameter* name. And in particular, the bit that confused me is the patch's current text here: > "We use |defs| tag" > That sounds a bit like it's saying |defs| is some variable that contains a tag name (like some of our other params here are). But it's not -- it's a reference to an actual <defs> element. So since you don't need to bother documenting the variable anyway, it seems clearer to just say "<defs>") Understood. I will replace "|defs| tag" with "<defs>". > This documentation as a whole is much clearer -- thanks! -- but this last sentence is still a bit hard to read, for me at least. (I stumbled several over the nested clauses when trying to read it, particularly over "...to which we apply by a url reference which ...") > > Maybe replace it with the following two sentences, which I find easier to grok: > > "We visualize the effect of our animation by referencing each animated subtree from some graphical element that we generate. The |graphicElemIdValueProperty| parameter provides the name of the CSS property that we should use to hook up this reference." Thanks for making this sentence much clearer. :) > Typo: s/forth/fourth/ > ("forth" is a different word which means "forward") > > Also, if you like (optional): I think it might be a bit more future-proof and readable to just do away with the numeric indices here (second, third, fourth) and instead just start this comment out with "NOTE: Some of the tests below...". And then flag the specific tests you're talking about by adding "# bug 515919" to the end of their line, as is customary for known reftest failures (and that'd tie them back to the longer comment because it's the same bug number mentioned there). This is OK as-is too, though, modulo the "forth" typo-fix. Thanks! This sounds better. I will add the bug number to the end of their lines.

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/71c9031c50d4 Update reftest list for SMIL animations. r=hiro https://hg.mozilla.org/integration/autoland/rev/992a6decd9df Add reftest for stop-color and flood-color. r=dholbert https://hg.mozilla.org/integration/autoland/rev/f8f055ef9dee Update WPT expectations. r=hiro

https://hg.mozilla.org/mozilla-central/rev/71c9031c50d4 https://hg.mozilla.org/mozilla-central/rev/992a6decd9df https://hg.mozilla.org/mozilla-central/rev/f8f055ef9dee