663288 - SVG SMIL: Instance times should not be dependent on themselves

Status: VERIFIED FIXED

Description

[Brian Birtles (:birtles)]

Created attachment 538402 [details]
Proposed patch v1a

In some arrangements of syncbase timing we can arrive at a situation where an instance time is directly dependent on itself. This shouldn't be.

Whilst self-referential timing such as:

  <animate id="a" begin="a.begin+2s" .../>

is perfectly reasonable (and required by the spec) setting an interval endpoint to an instance time that is already dependent on that same interval endpoint doesn't make sense.

This is best illustrated in the test cases in the attached patch.

Just how we should behave isn't specified by SVG/SMIL since they don't really deal with dynamic document changes. However, I think the test cases in the attached patch demonstrate why this new behaviour is correct.

Note that the patch applies on top of the patch for bug 650732 and may not apply entirely cleanly otherwise.

Comment 1

[Brian Birtles (:birtles)]

Created attachment 538404 [details] [diff] [review]
Proposed patch v1b

Comment 2

[Brian Birtles (:birtles)]

Created attachment 538405 [details]
Test case 1

Test case 1 -- identical to cycle-self-ref-4.svg from the proposed patch but attached separately here for easier testing.

Comment 3

[Brian Birtles (:birtles)]

Created attachment 538406 [details]
Test case 2

Test case 2 -- identical to cycle-self-ref-5.svg from the proposed patch but attached separately here for easier testing.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 538404 [details] [diff] [review]
Proposed patch v1b

A few notes on "Testcase 2" a.k.a cycle-self-ref-5.svg:

>+       If b's interval disappears the end time "a.end+2s" (calculated as
>+       t=(b.end=3s)+2s=5s, but then updated to become 7s) should not be used as
>+       the end time since b never really ended.

I don't understand the "but then updated to become 7s" part there -- that sounds like something is changing in the document to replace the 5s ending with a 7s ending, but I don't see why that'd happen.

Perhaps this is just talking about the third "end" event that we queue up for element 'a', at (3s + 2s) + 2s = 7s?  If so, it might be good to (a) use a different word from "updated" & (b) explain why we're specifically watching out for the 7s end event, instead of the 5s one.

Also, could you add a comma after "disappears", & after "used as the end time".

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 538404 [details] [diff] [review]
Proposed patch v1b

>    * @param aPrevInterval   The previous interval used. If supplied, the first
>    *                        interval that begins after aPrevInterval will be
>    *                        returned. May be nsnull.
>+   * @param aCurrentInterval The current interval that is being updated. This is
>+   *                        used to ensure we don't return interval endpoints
>+   *                        that are dependent on themselves. May be nsnull.
[...]
>    * @return  PR_TRUE if a suitable interval was found, PR_FALSE otherwise.
>    */
>   PRBool            GetNextInterval(const nsSMILInterval* aPrevInterval,
>+                                    const nsSMILInterval* aCurrentInterval,
>                                     const nsSMILInstanceTime* aFixedBeginTime,
>                                     nsSMILInterval& aResult) const;

I don't love the name "aCurrentInterval" here -- it's a bit confusing. My thought process when initially re-paging in this code: "So, given a previous interval, this method looks up the next interval.  And now we're also considering a 'current' interval. So 'current' sounds like it's in between 'previous' and 'next'... but I thought 'next' was the interval immediately after 'previous'!"  (I've sorted this out now, but it's confusing on an initial reading. :))
(side-note: the above reminds me of http://www.youtube.com/watch?v=VeZ9HhHU86o&t=54s )

Perhaps we could name this argument aReplacedInterval, or something like that?  I'd prefer that we remove the word "current" from the method header-comment, too ("The current interval that is being updated"), to completely avoid potentially-confusing temporal implications. :)

r=dholbert with that

Comment 6

[Brian Birtles (:birtles)]

Created attachment 539959 [details] [diff] [review]
Proposed patch v1c, r=dholbert

Thanks Daniel! Those are good comments. I've addressed the review feedback in this patch and updated the comments for the test case too.

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 539959 [details] [diff] [review]
Proposed patch v1c, r=dholbert

Landed on m-i:
http://hg.mozilla.org/integration/mozilla-inbound/rev/76781a68c1ba

Comment 8

[Matt Brubeck (:mbrubeck)]

http://hg.mozilla.org/mozilla-central/rev/76781a68c1ba

Comment 9

[Vlad [QA]]

Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 1

Hi. I wanted to verify this bug by running the two testcases. In both of the tabs, there is a green square.

I have tried the same thing in Google Chrome, but in the second test case, a red square appears that turns green.

Wich is the normal behavior?

Comment 10

[Brian Birtles (:birtles)]

(In reply to Vlad from comment #9)
> Hi. I wanted to verify this bug by running the two testcases. In both of the
> tabs, there is a green square.

This is the correct behaviour, both should be green.
 
> I have tried the same thing in Google Chrome, but in the second test case, a
> red square appears that turns green.

Chrome is wrong. Unfortunately WebKit's support of SMIL is poor so many of our test cases fail in Chrome.

Comment 11

[Vlad [QA]]

Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2