The original purpose of nsStyleAnimation::StoreComputedValue was essentially to have a faster path so we wouldn't need to go through what I originally described as the slow path for UncomputeValue in bug 504652. However, we're not doing the slow path... although we may eventually need to for some values. However, those values are unlikely to be the ones that people animate often. Furthermore, StoreComputedValue is an obstacle to implementing a decent number of properties that have complicated computation code in nsRuleNode. So I think we should: * eliminate StoreComputedValue (there are no plans to use it for SMIL, right?) * make transitions use UncomputeValue and normal rule mapping instead of StoreComputedValue and post-resolve callbacks (I was originally thinking we could do this for only some properties and leave StoreComputedValue as a fast path for others, but given that we have a fast path (currently the only path, but maybe not that way forever) in UncomputeValue, I don't think we even need to do that.)

This works, and fixes a bunch of transitions bugs, especially with font-size. (It also removes some incorrect code in StoreComputedValue dealing with font-size, except also incorrectly testing for font rather than font-size.) Daniel... are there tests I should run to check that SMIL still works with this? Also, once this lands, I should back out: http://hg.mozilla.org/mozilla-central/rev/c87e6a6a41bb http://hg.mozilla.org/mozilla-central/rev/9ef12a27ab14

Oh... and diff did a particularly poor job with nsStyleAnimation.cpp; it made the wrong choice for which UncomputeValue method to treat as new vs. modified.

(In reply to comment #1) > Daniel... are there tests I should run to check that SMIL still works with > this? Yeah -- to unit-test the CSS-related SMIL code, run... - the reftests in layout/reftests/svg/smil/style/ - the mochitests in content/smil/test/

(In reply to comment #4) > Yeah -- to unit-test the CSS-related SMIL code, run... > - the reftests in layout/reftests/svg/smil/style/ > - the mochitests in content/smil/test/ Those all run as expected (though I had to flip the smil pref for the reftests).

Comment on attachment 404455 [details] [diff] [review] patch >+/* static */ PRBool >+nsCSSDeclaration::AppendStorageToString(nsCSSProperty aProperty, >+ const void* aStorage, >+ nsAString& aResult) >+{ >+ if (aStorage) { > switch (nsCSSProps::kTypeTable[aProperty]) { [SNIP] > case eCSSType_ValueList: { > const nsCSSValueList* val = >- *static_cast<nsCSSValueList*const*>(storage); >+ *static_cast<nsCSSValueList*const*>(aStorage); [SNIP] >- return storage != nsnull; >+ return aStorage != nsnull; > } So, AppendStorageToString's documentation implies that aStorage can be null, and the function will just return false. But I think that a null aStorage value would actually make us crash in the ValueList and ValuePairList cases, right? In that case, we'd treat aStorage as a double-pointer and immediately dereference it to get a single-pointer. (see code above) And the null-deref will make us crash. I think the ValueList / ValuePairList cases just need an "if (aStorage) {" statement inside each of them. Looking at your patch-queue version ("no-store-computed-value"), which has bug 520325's changes merged in: > switch (aComputedValue.GetUnit()) { > case eStyleUnit_None: >- value.SetNoneValue(); >+ NS_ABORT_IF_FALSE(nsCSSProps::kTypeTable[aProperty] == >+ eCSSType_ValuePair, "type mismatch"); >+ static_cast<nsCSSValuePair*>(aSpecifiedValue)-> >+ SetBothValuesTo(nsCSSValue(eCSSUnit_None)); break; So, your patch introduces the assumption that *only* pair-valued properties will use "eStyleUnit_None". I don't think that's a good assumption to introduce. e.g. I've got a patch here... http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/f2d177aea650/smil_css_styleanimation_enum8 ...to support enumerated-value properties (bug 520486), and a number of them (filter, pointer-events) use the value "none" (and I use eStyleUnit_None to represent it and to get back final string-value of "none"). I also use "eStyleUnit_None" for the float-valued property "font-size-adjust, which takes both floating point values and the "none" value: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/file/f2d177aea650/smil_css_styleanimation_font_size_adjust I'd be happier if we made eStyleUnit_None look more like the eStyleUnit_Color case -- with one clause for paint-server-AnimType (pair-valued) properties, and one clause for single-valued properties. If you prefer, we can hold off on that change, and bundle it with the "support enum-valued types" bug (e.g. bug 520486), though. r=dholbert with the first change and a decision regarding on the second change.

(In reply to comment #2) > Oh... and diff did a particularly poor job with nsStyleAnimation.cpp; it made > the wrong choice for which UncomputeValue method to treat as new vs. modified. FWIW, it makes the right choice if you pass "diff" the "-d" flag. (For that to work in "hg diff", I think you have to use an alias in your .hgrc file.)

(In reply to comment #6) > So, AppendStorageToString's documentation implies that aStorage can be null, > and the function will just return false. But I think that a null aStorage > value would actually make us crash in the ValueList and ValuePairList cases, > right? In that case, we'd treat aStorage as a double-pointer and immediately > dereference it to get a single-pointer. (see code above) And the null-deref > will make us crash. > > I think the ValueList / ValuePairList cases just need an "if (aStorage) {" > statement inside each of them. Except the entire function (including those cases) except for the last line is inside an if (aStorage) {} already. > So, your patch introduces the assumption that *only* pair-valued properties > will use "eStyleUnit_None". I don't think that's a good assumption to > introduce. The idea is that I'll add things as they're needed rather than writing code that can't be tested. So when we add a value-valued property that takes 'none' values we should add the appropriate code. So is it ok if I make neither change?

Comment on attachment 404455 [details] [diff] [review] patch r=bzbarsky

(In reply to comment #8) > Except the entire function (including those cases) except for the last line is > inside an if (aStorage) {} already. Ah, of course! Looks good then. > So when we add a value-valued property that takes 'none' > values we should add the appropriate code. > > So is it ok if I make neither change? Sure. r=dholbert.

Comment on attachment 404455 [details] [diff] [review] patch Actually, one nit that I just noticed: From nsStyleAnimation::UncomputeValue, under "case eStyleUnit_Color:": >+ nsCSSValue val; >+ val.SetColorValue(aComputedValue.GetColorValue()); >+ static_cast<nsCSSValuePair*>(aSpecifiedValue)-> >+ SetBothValuesTo(val); Those four lines can be shortened to: static_cast<nsCSSValuePair*>(aSpecifiedValue)-> SetBothValuesTo(nsCSSValue(aComputedValue.GetColorValue())); eliminating the need for the "val" local variable. (This is similar to what you do in the "eStyleUnit_None" case, up a little higher.)

Hmmm. I guess nsCSSValue does have a constructor taking nscolor, but I think that constructor was a bad idea, since nscolor is indistinguishable from PRInt32 or nscoord. I should probably remove it.

Good point. Anyway, if we end up if we end up still having *some* sort of nsCSSValue() constructor for nscolor values, I'd prefer that we use it here, as in comment 11. Not a huge deal, though, and r=dholbert either way.

Landed: http://hg.mozilla.org/mozilla-central/rev/d3ad054f80e2 Still need to file a followup on removing that constructor.

(In reply to comment #1) > Also, once this lands, I should back out: > http://hg.mozilla.org/mozilla-central/rev/c87e6a6a41bb > http://hg.mozilla.org/mozilla-central/rev/9ef12a27ab14 Done: http://hg.mozilla.org/mozilla-central/rev/d8c914dbd3f8

dunno if it's related but after this change looks like there is a new random failure in /tests/layout/style/test/test_transitions.html, for example: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1254996299.1254998758.2575.gz i don't recall this random failure before, but i could be wrong.

test_transitions was a new test landed at the same time this landed, but in another patch. See the last few comments on bug 435441.

(In reply to comment #12) > Hmmm. I guess nsCSSValue does have a constructor taking nscolor, but I think > that constructor was a bad idea, since nscolor is indistinguishable from > PRInt32 or nscoord. I should probably remove it. I filed this as bug 521350 (with patch), and also filed bug 521352 (with patch) on an even worse situation in nsStyleCoord.