[SVG] Regression: SVG syncbase animation no longer supports cyclic references

[SVG] Regression: SVG syncbase animation no longer supports cyclic references

Removed the logic of reconnecting the conditions which was added before to support dynamic interactions of eventbases using js.
It was breaking the syncbase logic for cyclic references as there is common code used for both eventbases and syncbases.
Hence removing the logic and as a result of this, it will break one testcase related to dynamic eventbase interactions.
Modifying the test expectations for this file and planning to add back the support once we implement eventbase logic with SVGDocumentExtensions.

Also fixed the seekToTime bug related to cyclic references.

R=pdr@chromium.org, schenney@chromium.org, dschulze@chromium.org, fmalita@chromium.org
BUG=308394
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162961

[pavane, 2013-11-26 14:22:14]

Please have a look at this patch

[pdr., 2013-11-27 06:57:01]

https://codereview.chromium.org/80263007/diff/40001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/80263007/diff/40001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:611: crbug.com/308394
svg/animations/repeatn-remove-add-animation.html [ Failure ]
Please create a new bug for this and list the id here.

https://codereview.chromium.org/80263007/diff/40001/Source/core/svg/animation...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/80263007/diff/40001/Source/core/svg/animation...
Source/core/svg/animation/SVGSMILElement.cpp:1027: if
(!resolveNextInterval(true))
If we do this, no callers exist with resolveNextInterval(false) and we should
refactor the code to remove that.

But, I don't think these resolveNextInterval(true) calls are right, or I need
more convincing. How did any dependent animation work without passing true here?
For example, how did this work:
<animate id="anim1" begin="0" dur="1s"/>
<animate id="anim2" begin="anim1.end" dur="1s"/>

I worry that this will unnecessarily notify dependents.

[pavane, 2013-11-27 07:41:29]

https://codereview.chromium.org/80263007/diff/40001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/80263007/diff/40001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:611: crbug.com/308394
svg/animations/repeatn-remove-add-animation.html [ Failure ]
On 2013/11/27 06:57:01, pdr wrote:
> Please create a new bug for this and list the id here.

Ok, i will create a bug for it

https://codereview.chromium.org/80263007/diff/40001/Source/core/svg/animation...
File Source/core/svg/animation/SVGSMILElement.cpp (right):

https://codereview.chromium.org/80263007/diff/40001/Source/core/svg/animation...
Source/core/svg/animation/SVGSMILElement.cpp:1027: if
(!resolveNextInterval(true))
On 2013/11/27 06:57:01, pdr wrote:
> If we do this, no callers exist with resolveNextInterval(false) and we should
> refactor the code to remove that.
> 
> But, I don't think these resolveNextInterval(true) calls are right, or I need
> more convincing. How did any dependent animation work without passing true
here?
> For example, how did this work:
> <animate id="anim1" begin="0" dur="1s"/>
> <animate id="anim2" begin="anim1.end" dur="1s"/>
> 
> I worry that this will unnecessarily notify dependents.

This code is part of seekToIntervalCorrespondingToTime() which gets called only
if we use setCurrentTime(). I wrote this code to pass the layout tests. If you
observe, there are no layout tests which are using dependent animations till
now. This is first such test case.

The normal path for dependent animations is through checkRestart()which works
pretty well and you will see that it is notifying the dependents that your
interval is changed where as for layout tests, dependent animations doesn't work
because we are not notifying the dependents.

[pavane, 2013-11-28 06:36:33]

pdr, i refactored the resolveNextInterval() to remove the arguments.

For your reference, please have a look at
http://trac.webkit.org/changeset/115947

It seems to me like Nikolas added support for seeking Intervals for simpler
cases. He didn't consider dependent animations. To my understanding, this patch
seems to be reasonable. Please give your opinion on this.

[pdr., 2013-12-02 03:15:37]

On 2013/11/28 06:36:33, pavane wrote:
> pdr, i refactored the resolveNextInterval() to remove the arguments.
> 
> For your reference, please have a look at
> http://trac.webkit.org/changeset/115947
> 
> It seems to me like Nikolas added support for seeking Intervals for simpler
> cases. He didn't consider dependent animations. To my understanding, this
patch
> seems to be reasonable. Please give your opinion on this.

(Apologies for the slow response... I was out of town for the Thanksgiving
holiday in the US.)

I agree with your conclusions on Niko's original intent and your refactoring in
the latest patch. LGTM!

[commit-bot: I haz the power, 2013-12-02 05:53:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pavan.e@samsung.com/80263007/120001

[commit-bot: I haz the power, 2013-12-02 06:51:05]

Change committed as 162961