Bug 49045

Created attachment 73024 [details] Patch

Comment on attachment 73024 [details] Patch i don't think we should be changing the test (it's ok for it to fail for now, i added it to test_expectations). instead, we should try to find somebody familiar with this code and make them take a look at this issue and fix it. i'll try to find a good owner for this bug tomorrow, unless you want to do it. can you please describe the problem we're observing, and ideally, how it can be reproduced?

From http://code.google.com/p/chromium/issues/detail?id=61978: "OK, looks like there's a bug somewhere in the animation test framework that makes it nasty to ask for the location of stuff at 0.01 seconds if executing in debug mode." Antti, Simon, is this something you're familiar with?

More details: to use the animation framework you make a call like: function executeTest() { const expectedValues = [ ["animation", 0, "rect", startSample], ["animation", 0.1, "rect", startAnimateSample], ["animation", 1.00, "rect", endSample] ]; runAnimationTest(expectedValues); } I believe this uses a set of extra hooks into the SMILAnimation code (WebCore::SVGDocumentExtensions::sampleAnimationAtTime?) to exactly control the animation state. if the time value (here 0, 0.1 and 1.00) is 0.01, then in debug mode there's an assert in SVGSMILElement::progress here: if (elapsed < m_intervalBegin) { ASSERT(m_activeState != Active); if (m_activeState == Frozen && resultElement) updateAnimation(m_lastPercent, m_lastRepeat, resultElement); m_nextProgressTime = m_intervalBegin; return; } I didn't get a chance to characterize further than this. Hope this helps.

*** Bug 49065 has been marked as a duplicate of this bug. ***

(In reply to comment #4) > More details: to use the animation framework you make a call like: > > function executeTest() { > const expectedValues = [ > ["animation", 0, "rect", startSample], > ["animation", 0.1, "rect", startAnimateSample], > ["animation", 1.00, "rect", endSample] > ]; > > runAnimationTest(expectedValues); > } > > I believe this uses a set of extra hooks into the SMILAnimation code (WebCore::SVGDocumentExtensions::sampleAnimationAtTime?) to exactly control the animation state. Correct. > > if the time value (here 0, 0.1 and 1.00) is 0.01, then in debug mode there's an assert in SVGSMILElement::progress here: > > if (elapsed < m_intervalBegin) { > ASSERT(m_activeState != Active); > if (m_activeState == Frozen && resultElement) > updateAnimation(m_lastPercent, m_lastRepeat, resultElement); > m_nextProgressTime = m_intervalBegin; > return; > } > > I didn't get a chance to characterize further than this. Hope this helps. Thanks, I'll have a look at this as soon as I'll find some time.

This also affects the new testcase provided in bug 48215.

I must have missed this bug. It should really get fixed :-P

*** Bug 48987 has been marked as a duplicate of this bug. ***

Thanks for looking into this. Please remember to unskip svg/animations/animate-path-nested-transforms.html on GTK+ when you fix the issue.

*** Bug 61798 has been marked as a duplicate of this bug. ***

I think I know what goes wrong. First, the id value in the expected values is wrong for this test. That causes a change in this code section: (SMILTimeContainer::updateAnimations): SVGElement* targetElement = animation->targetElement(); // FIXME: This should probably be using getIdAttribute instead of idForStyleResolution. if (!targetElement || !targetElement->hasID() || targetElement->idForStyleResolution() != m_nextSamplingTarget) continue; samplingDiff = animation->intervalBegin(); Because these tests are only started on click, and intervalBegin() in that case is small but not 0, it is important for the later code calling progress to take this into account. Again, this is caused by a wrong id value in the expected results, as m_nextSamplingTarget will mismatch. For now I propose to just fix the tests. The only alternative I can think of is to give a warning or even assert when the id is not found at all? Cheers, Rob.

Created attachment 95674 [details] Patch

Comment on attachment 95674 [details] Patch Good catch, r=me.

Committed r87907: <http://trac.webkit.org/changeset/87907>