Closed Bug 618205 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9

People

(Reporter: birtles, Assigned: birtles)

Details

(Whiteboard: [parity-Opera][parity-webkit])

Attachments

(2 files, 3 obsolete files)

Attached image 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.
Attached patch Possible patch (obsolete) — Splinter Review
Initial idea about how this should behave
Summary: SVG SMIL: Fallback to non-additive animation for 'additive' CSS animation → SVG SMIL: Correctly fallback to non-additive animation on display and shorthand properties
Whiteboard: [parity-Opera][parity-webkit]
Attached patch Patch v1a (obsolete) — Splinter Review
Proposed patch
Attachment #496714 - Attachment is obsolete: true
Attachment #497356 - Flags: review?(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?)
Attached patch Patch v1b (obsolete) — Splinter Review
(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.
Attachment #497356 - Attachment is obsolete: true
Attachment #497666 - Flags: review?(dholbert)
Attachment #497356 - Flags: review?(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)
Attachment #497666 - Flags: review?(dholbert) → review+
(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.)
Attachment #497666 - Attachment is obsolete: true
Attachment #497675 - Flags: review+
Attachment #497675 - Flags: approval2.0?
Requesting approval to land. Fixes broken behaviour by reverting to previous tested behaviour. Parity Opera and Webkit. Includes test.
Pushed: http://hg.mozilla.org/mozilla-central/rev/edc1ef56b1f9
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.