1404803 - When using discrete animation with an "empty" nsSMILCSSValueType value the zero-value fixup never happens

Status: RESOLVED FIXED

(Core :: SVG, defect, P3)

Description

[Brian Birtles (:birtles)]

See bug 1402547 comment 10

Comment 1

[Brian Birtles (:birtles)]

Attachment: Mochitest

Comment 2

[Brian Birtles (:birtles)]

Attachment: Mochitest

(Accidentally left the begin="indefinite" in from another test)

Comment 3

[Brian Birtles (:birtles)]

Attachment: Patch

This patch fixes the bug but it's awefully complicated for something that, as far as I can tell, only ever happens with a discrete non-additive SMIL by-animation. That also makes it really hard to test the code here so I'm a bit uncomfortable about landing something as complicated as this given how unlikely it is to be exercised.

Perhaps I can find other ways to test/simplify it.

Comment 4

[Brian Birtles (:birtles)]

Attachment: Bug 1404803 - Convert empty values to suitable zero values even when using discrete interpolation;

Normally when we interpolate values for CSS properties we convert empty values
(such as the empty "from" value created for a by-animation) to a suitable zero
value. However, when we use discrete interpolation that step never occurs and in
some rare cases that means we end up setting the empty value on the target
attribute directly which will have an incorrect result (e.g. setting "" for the
height property will have no effect, whereas setting "0px" will).

This patch makes us perform the empty value to zero value conversion when
performing discrete animation.

Unfortunately it is not possible to test this behavior with shorthands since
there are currently no shorthand properties that are additive.

Review commit: https://reviewboard.mozilla.org/r/186292/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/186292/

Comment 5

[Brian Birtles (:birtles)]

I'm not sure if this patch is worth it honestly. It fixes the behavior but it introduces a fair bit of code for a very rare case.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8915023 [details]
Bug 1404803 - Convert empty values to suitable zero values even when using discrete interpolation;

https://reviewboard.mozilla.org/r/186292/#review191616

::: dom/smil/nsSMILAnimationFunction.cpp:491
(Diff revision 1)
> +        // We have currently only ever encountered this case for the first
> +        // value of a by-animation (which has two values) and since we have no
> +        // way of testing other cases we just skip them (but assert if we
> +        // ever do encounter them so that we can add code to handle them).
> +        if (index + 1 >= aValues.Length()) {
> +          MOZ_ASSERT(static_cast<void*>(aResult.mU.mPtr),

Is this static_cast necessary?

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8915023 [details]
Bug 1404803 - Convert empty values to suitable zero values even when using discrete interpolation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/186292/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8974272e3e3
Convert empty values to suitable zero values even when using discrete interpolation; r=hiro

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d8974272e3e3