Bug 85372 - Fix multiple begin values support - especially with seeking through setCurrentTime
Summary: Fix multiple begin values support - especially with seeking through setCurren...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2012-05-02 06:44 PDT by Nikolas Zimmermann
Modified: 2012-05-03 01:44 PDT (History)
2 users (show)

See Also:


Attachments
Patch (27.37 KB, patch)
2012-05-02 06:49 PDT, Nikolas Zimmermann
zherczeg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2012-05-02 06:44:16 PDT
Multiple begin values aka. begin="0s; 2s" aren't correctly handled - resulting in broken & unexpected behavior.
Supporting seeking properly on documents containing such animations is very important, otherwise we can't reliable test animations using either reftests or the SVG JS animation test framework.

Testcase:
<rect height="100" fill="green">
    <animate attributeName="width" begin="0s; 2s" dur="8s" from="0" to="100" fill="freeze"/>
</rect>

What's expected?
Two times should be contained in the 'begin' times list in SVGSMILElement: m_beginTimes = { 0s, 2s }.
The initial first resolved interval is: m_intervalBegin=0.0s, m_intervalEnd=8.0s.

During t=0s..1.9999s the m_intervalBegin/m_intervalEnd are correct.
At t=2s, a new interval can be started. m_intervalEnd should be set to nextBeginTime, where nextBeginTime=2s.
The current interval should get cropped to: m_intervalBegin=0s, m_intervalEnd=2s. The following call to resolveNextInterval() sees that elapsed >= m_intervalEnd, and thus moves on to the next interval.
m_intervalBegin should be 2s and m_intervalEnd=10s after that.

In trunk this behavior is only partly implemented and broken. Especially broken together with seeking via SVGSVGElement.setCurrentTime.
That's because we don't correctly seek to the right interval in case of multiple begin values, eg. if we sample an animation with begin="0s; 3s" dur="6s" we always remain in the first interval and don't move on.
Comment 1 Nikolas Zimmermann 2012-05-02 06:49:04 PDT
Created attachment 139804 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-02 06:52:28 PDT
Attachment 139804 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1
Source/WebCore/svg/animation/SVGSMILElement.cpp:786:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Nikolas Zimmermann 2012-05-02 08:32:42 PDT
(In reply to comment #2)
> Attachment 139804 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/svg/..." exit_code: 1
> Source/WebCore/svg/animation/SVGSMILElement.cpp:786:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
This error is in purpose. I can't use !smilTime, I have to use smilTime == 0
Comment 4 Nikolas Zimmermann 2012-05-03 00:33:46 PDT
(In reply to comment #3)
> > Source/WebCore/svg/animation/SVGSMILElement.cpp:786:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
> This error is in purpose. I can't use !smilTime, I have to use smilTime == 0

Another way to get rid of the warning is to use !smilTime.value(), instead of smilTime == 0.
I could include this upon landing, if desired.
Comment 5 Zoltan Herczeg 2012-05-03 01:24:46 PDT
Comment on attachment 139804 [details]
Patch

r=me. Fix the complain of the style bot.
Comment 6 Nikolas Zimmermann 2012-05-03 01:44:49 PDT
Committed r115947: <http://trac.webkit.org/changeset/115947>