Closed
Bug 663288
Opened 14 years ago
Closed 14 years ago
SVG SMIL: Instance times should not be dependent on themselves
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
VERIFIED
FIXED
mozilla7
People
(Reporter: birtles, Assigned: birtles)
Details
Attachments
(3 files, 2 obsolete files)
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.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #538402 -
Attachment is obsolete: true
Attachment #538404 -
Flags: review?(dholbert)
Assignee | ||
Comment 2•14 years ago
|
||
Test case 1 -- identical to cycle-self-ref-4.svg from the proposed patch but attached separately here for easier testing.
Assignee | ||
Comment 3•14 years ago
|
||
Test case 2 -- identical to cycle-self-ref-5.svg from the proposed patch but attached separately here for easier testing.
Comment 4•14 years ago
|
||
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•14 years ago
|
||
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
Attachment #538404 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 6•14 years ago
|
||
Thanks Daniel! Those are good comments. I've addressed the review feedback in this patch and updated the comments for the test case too.
Attachment #538404 -
Attachment is obsolete: true
Assignee | ||
Comment 7•14 years ago
|
||
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
Attachment #539959 -
Flags: checkin+
Assignee | ||
Updated•14 years ago
|
Whiteboard: [inbound]
Comment 8•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
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?
Assignee | ||
Comment 10•14 years ago
|
||
(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•14 years ago
|
||
Setting resolution to Verified Fixed on Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0 beta 2
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•