618205 - SVG SMIL: Correctly fallback to non-additive animation on display and shorthand properties

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: Test case

Currently we are using a null base value for CSS shorthand and display properties. However, for animations on these properties that are (somewhat incorrectly) marked as additive (e.g. by-animation, or additive"sum") we should fall back to non-additive animation. Instead, however, this null value is interpreted as meaning that something went wrong in the animation sandwich below and we don't have a valid base value so we're bailing out altogether.

See the attached test case for a better explanation.

Comment 1

[Brian Birtles (:birtles)]

Attachment: Possible patch

Initial idea about how this should behave

Comment 2

[Brian Birtles (:birtles)]

Attachment: Patch v1a

Proposed patch

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 497356 [details] [diff] [review]
Patch v1a

># HG changeset patch
># Parent 0e04a8639b74b2489b40d4ba5be606a4b579fd3f
>Bug 618205 - SVG SMIL: Correctly fallback to non-additive animation on display and shorthand properties
>
>diff --git a/content/smil/nsSMILCSSProperty.cpp b/content/smil/nsSMILCSSProperty.cpp
>--- a/content/smil/nsSMILCSSProperty.cpp
>+++ b/content/smil/nsSMILCSSProperty.cpp
>-    return baseValue;
>+    // In either case, just return a dummy value (initialized with the right
>+    // type, so as not to indicate failure).
>+    return nsSMILValue(&nsSMILCSSValueType::sSingleton);

To let this function benefit from return-value-optimization, we need to make sure we return |baseValue| -- so you need to make |baseValue| into a just-initialized dummy-value, rather than returning one.

Basically it looks like you want to just revert this chunk:
http://hg.mozilla.org/mozilla-central/rev/edf3071a4fdb#l3.2

However, what are the implications of that?  (Presumably that chunk was useful in the changeset that tweaked it to be the way it is now?)

Comment 4

[Brian Birtles (:birtles)]

Attachment: Patch v1b

(In reply to comment #3)
> To let this function benefit from return-value-optimization, we need to make
> sure we return |baseValue| -- so you need to make |baseValue| into a
> just-initialized dummy-value, rather than returning one.
Ok.

(In reply to comment #3)
> Basically it looks like you want to just revert this chunk:
> http://hg.mozilla.org/mozilla-central/rev/edf3071a4fdb#l3.2
Yep, that's right.

> However, what are the implications of that?  (Presumably that chunk was useful
> in the changeset that tweaked it to be the way it is now?)

In that changeset I thought I could simplify things a bit however I later realised that was in error for the particular case this bug address. So this patch is really just reverting part of that changeset.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 497666 [details] [diff] [review]
Patch v1b

>+    // Return baseValue in the hope of getting some return-value-optimisation

There's already a comment about this at the beginning of this method, actually:
> 100   // To benefit from Return Value Optimization and avoid copy constructor calls
> 101   // due to our use of return-by-value, we must return the exact same object
> 102   // from ALL return points. This function must only return THIS variable:
> 103   nsSMILValue baseValue;

So you could probably just remove the quoted line above.  But if you want to keep it, s/optimisation/optimization/ for consistency with the earlier comment's spelling. :)  (only mentioning since these comments are so close together, & we shouldn't switch spelling within the same block of code)

Comment 6

[Brian Birtles (:birtles)]

Attachment: Patch v1c; r=dholbert

(In reply to comment #5)
> There's already a comment about this at the beginning of this method, actually:
Oh, so there is! Thanks!

> So you could probably just remove the quoted line above.  But if you want to
> keep it, s/optimisation/optimization/ for consistency with the earlier
> comment's spelling. :)  (only mentioning since these comments are so close
> together, & we shouldn't switch spelling within the same block of code)
(I'm fine with using US spelling for source code comments, it's just force of habit that I end up using UK spelling a lot of the time.)

Comment 7

[Brian Birtles (:birtles)]

Requesting approval to land. Fixes broken behaviour by reverting to previous tested behaviour. Parity Opera and Webkit. Includes test.

Comment 8

[Brian Birtles (:birtles)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/edc1ef56b1f9