After bug 896050, the drop-shadow animation is still missing and needs to be implemented.

Comment on attachment 789582 [details] [diff] [review] drop-shadow-animation-moz.patch >+static bool >+AppendCSSShadowValue(const nsCSSShadowArray* aShadowArray, >+ nsCSSValueList **&aResultTail) >+{ [...] >+ nsCSSValueList *resultItem = new nsCSSValueList; >+ if (!resultItem) { >+ return false; >+ } No need to null-check resultItem there -- we have infallible "new", so you can assume it succeeds (or else it would have aborted). This is an issue already present in the code that you're moving (since it predated the switch to infallible-new), so it's not your fault, of course. :) But it's worth cleaning it up as you move the code, because it lets the function that you're refactoring this into be guaranteed-to-succeed as well. (That null-check is the only failure-case, so after you remove it, you won't need the bool return value anymore.) Also, stepping back slightly: I'm not sure the pattern you're following for this function (Take "aResultTail" as an outparam, which we append to and update to point to the new tail) is really the best match for how this function is actually used. In particular -- neither of this function's two callers actually have a pre-existing "resultTail" that you're appending to (they both declare a fresh list-pointer, and use that to get a trivial "tail" of an empty list to pass in), and neither of the callers actually care about the fact that we update the tail before we return -- they don't use the value at all after we return. So -- I'm not sure what other pattern would make the most sense, but I'm imagining something closer to the "StyleCoordToCSSValue" pattern -- maybe called "ShadowValueArrayToCSSValueList()". I imagine it'd either take a nsCSSValueList* pointing to an already-created nsCSSValueList object, or it could return a nsCSSValueList* directly (with a "return result.forget()" at the end). Either way, we'd be able to simplify the two callers a bit by switching away from needing to set up & pass in a resultTail.

Comment on attachment 789582 [details] [diff] [review] drop-shadow-animation-moz.patch >+ case eCSSKeyword_drop_shadow: { >+ nsCSSValueList* shadow = result->Item(1).SetListValue(); >+ nsAutoPtr<nsCSSValueList> shadowValue; >+ nsCSSValueList **shadowTail = getter_Transfers(shadowValue); >+ if (!AddShadowItems(aCoeff1, a1->Item(1).GetListValue()->mValue, >+ aCoeff2, a2->Item(1).GetListValue()->mValue, >+ shadowTail)) { >+ return false; >+ } >+ *shadow = *shadowValue; >+ break; So, the already-existing caller of AddShadowItems() (for the "box-shadow" property) calls it in a loop, advancing the inputs to their "->mNext" values until there are no more next-values. (and has some other logic as well): http://mxr.mozilla.org/mozilla-central/source/layout/style/nsStyleAnimation.cpp?rev=552bca1bc885#2018 I think that's to handle comma-separated values, which appear to be valid inside of a "drop-shadow()" filter-function as well. At least, we have one comma-separated drop-shadow() filter-value in property_database.js: > 4583 "drop-shadow(green 2px 2px, blue 1px 3px 4px)", http://mxr.mozilla.org/mozilla-central/source/layout/style/test/property_database.js?rev=5fdf867f67d6#4583 Do we need the same thing here, in case we have a comma-separated list inside of our drop-shadow() function? Or is that invalid for drop-shadow()? (And if it's invalid, then maybe we should really be storing our shadow value as a nsCSSValue instead of a nsCSSValueList, since we know there will only be one? Seems like that might make things simpler.)

so -- proceeding under the assumption that the drop-shadow() filter-function only takes a single shadow value -- we'd of course still use a nsCSSValueList for the "box-shadow" property itself, but for the drop-shadow() filter function, you'd have your e.g. "filterArray->Item(1)" being an array-typed nsCSSValue, rather than list-typed. And you'd probably want to factor out AppendCSSShadowValue such that your helper-function only populates a single nsCSSValue (since that's all you should need for drop-shadow), rather than constructing a whole list.)

(In reply to Daniel Holbert [:dholbert] from comment #4) > so -- proceeding under the assumption that the drop-shadow() filter-function > only takes a single shadow value -- we'd of course still use a > nsCSSValueList for the "box-shadow" property itself, but for the > drop-shadow() filter function, you'd have your e.g. "filterArray->Item(1)" > being an array-typed nsCSSValue, rather than list-typed. And you'd probably > want to factor out AppendCSSShadowValue such that your helper-function only > populates a single nsCSSValue (since that's all you should need for > drop-shadow), rather than constructing a whole list.) I would rather not treat drop-shadow differently. This requires refactoring of code in nsCSSParser and nsRuleNode. I already looked into that when I implemented the parsing for drop-shadow. The code looked more complicated and given that I am still pushing to support multiple drop-shadows for the next level of the specification, it might make things more complicated in the long run and not worth the work.

OK, that makes sense; disregard comment 4, then. Could you add an assertion to the code that I quoted in comment 3 (where we call AddShadowItems) to enforce that the lists for a1 and a2 don't have anything past that first value that you're adding? (both for documentation and for sanity-check purposes). If & when a future version of the spec extends drop-shadow to take a list of shadow-values, then that assertion can go away, but until then, it'll keep us honest. Also: now that I know that drop-shadow() takes just a single value (whereas "box-shadow" takes a list), I think I have a slightly different take on how "AppendCSSShadowValue" should work -- different from what I said in comment 2. I think we should pull the "for" loop *out of that function*, so that the function only appends a *single* Shadow value. Then, we can invoke that function *in a for loop* when we're handling "box-shadow" (across an array of values), whereas we can invoke it exactly once for "drop-shadow" (on the only element in shadow-array of length 1 -- and we should assert that the shadow-array has length 1, too.) Then, the "aResultTail" paradigm will actually make sense, because each invocation *will* be appending to a continuously-advanced tail (for box-shadow, at least). And we'll more closely match the way we do addition of shadow-values, too. (piecewise, rather than all at once). Does that make sense?

I'm duping this bug to bug 896050, since you've merged this bug's code into the patch over there.