Closed
Bug 941315
Opened 11 years ago
Closed 11 years ago
nsSMILTimedElement calls UpdateCurrentInterval() inconsistently between error cases vs. success cases
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: dholbert, Assigned: birtles)
References
Details
Attachments
(1 file, 2 obsolete files)
11.97 KB,
patch
|
longsonr
:
review+
|
Details | Diff | Splinter Review |
While reviewing bug 937614, I noticed some error-handling cases where nsSMILTimedElement might be missing a call to UpdateCurrentInterval(). It looks like we try to call UpdateCurrentInterval every time we modify mSimpleDur, mMax, or mMin, but there are a few early-returns where we modify those (setting them to Indefinite) but we *don't* update the current interval. e.g. > 906 nsSMILTimedElement::SetSimpleDuration(const nsAString& aDurSpec) > 907 { > [...] > 915 if (NS_FAILED(rv)) { > 916 mSimpleDur.SetIndefinite(); > 917 return NS_ERROR_FAILURE; > 918 } [...] > 929 if (isMedia) > 930 duration.SetIndefinite(); [...] > 937 mSimpleDur = duration; > 938 UpdateCurrentInterval(); http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILTimedElement.cpp?rev=5f1f90c96c91#916 Here, the error-handling case and the "isMedia" case both end up setting mSimpleDur to indefinite, but we only call UpdateCurrentInterval() in one of those code-paths and not the other. And there are similar inconsistencies in the SetMin() and SetMax() methods there. Brian, do you know if these differences are intentional & OK, or should we really be calling UpdateCurrentInterval() in those cases as well? (I don't remember this code well enough to have a good feel for when it's required & when it's not.)
Reporter | ||
Updated•11 years ago
|
Flags: needinfo?(birtles)
Reporter | ||
Comment 1•11 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #0) > It looks like we try to call UpdateCurrentInterval every time we modify > mSimpleDur, mMax, or mMin, but there are a few early-returns where we modify > those (setting them to Indefinite (sorry -- "setting them to Indefinite" is only accurate for mSimpleDur & mMax. In contrast, the error-handling code for mMin calls SetMillis(0L), since that's the default value for the 'min' attribute).
Reporter | ||
Comment 2•11 years ago
|
||
SetRepeatDur() is another function that might need this: http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILTimedElement.cpp?rev=5f1f90c96c91#1077
Assignee | ||
Comment 3•11 years ago
|
||
Sorry, I won't have a chance to look into this properly until next week. I don't remember doing this on purpose so it could well be a bug.
Flags: needinfo?(birtles)
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(birtles)
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #0) > It looks like we try to call UpdateCurrentInterval every time we modify > mSimpleDur, mMax, or mMin, but there are a few early-returns where we modify > those (setting them to Indefinite) but we *don't* update the current > interval. Sorry for the delay here. Yes, I agree we should update the current interval in these four places (SetSimpleDuration, SetMin, SetMax, SetRepeatDur). It's been a long time since I worked on this code so I'm not 100% sure it's necessary but if (a) current tests pass with this change added, and (b) we can generate a test that fails without this added then I think we should be make this change. I think we will probably rework large parts of this SVG/SMIL animation code on top of the Web Animations code when we do that, so it would be good to have regression tests for this. I can do this next week if you don't have time.
Flags: needinfo?(birtles)
Comment 5•11 years ago
|
||
Please wait for bug 937614 to land Brian.
Reporter | ||
Comment 6•11 years ago
|
||
I don't have cycles do do this at the moment, so I'll defer to you (I think you're also probably more likely to be able to achieve (b) in comment 4 than I am. :)).
Depends on: 937614
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•11 years ago
|
||
This patch depends on the patches for bug 948245
Comment 8•11 years ago
|
||
I wonder if a stack class that caLls UpdateCurrentInterval in its destructor would be more robust? Just instantiate it and exit the method however you like.
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Robert Longson from comment #8) > I wonder if a stack class that caLls UpdateCurrentInterval in its destructor > would be more robust? Just instantiate it and exit the method however you > like. Yes, it would be more robust. I am anticipating we rewrite all this code in the next 12 months when rebase on top of Web Animations components but perhaps its still worth doing anyway.
Comment 10•11 years ago
|
||
I think it is worth doing please if that's ok Brian :)
Assignee | ||
Comment 11•11 years ago
|
||
Rewritten using a stack-based helper class.
Attachment #8345172 -
Attachment is obsolete: true
Attachment #8346285 -
Flags: review?(longsonr)
Assignee | ||
Comment 12•11 years ago
|
||
Try run is here: https://tbpl.mozilla.org/?tree=Try&rev=4164f6a6f786
Assignee | ||
Comment 13•11 years ago
|
||
These patches, by the way, depend on those from bug 948245 since otherwise the test case for 'min' will fail.
Assignee | ||
Comment 14•11 years ago
|
||
Just a quick update to the comments to remove reference to RAII since that may be confusing.
Attachment #8346285 -
Attachment is obsolete: true
Attachment #8346285 -
Flags: review?(longsonr)
Attachment #8346288 -
Flags: review?(longsonr)
Assignee | ||
Updated•11 years ago
|
Attachment #8346288 -
Attachment description: makeInvalidValuesUpdateTheModel → Patch v1c
Comment 15•11 years ago
|
||
Comment on attachment 8346288 [details] [diff] [review] Patch v1c Lovely. Thanks Brian.
Attachment #8346288 -
Flags: review?(longsonr) → review+
Assignee | ||
Comment 16•11 years ago
|
||
(In reply to Robert Longson from comment #15) > Lovely. Thanks Brian. Thanks Robert! I'll push this together with bug 948245 once its reviewed rather than prepare a separate version of this patch.
Assignee | ||
Comment 17•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/00218997103b
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/00218997103b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•