[SVG] Apply animations in sorted order based on begin time and document order

[SVG] Apply animations in sorted order based on begin time and document order

Sorting the animations before applying based on begin time and document order as per the spec.
Otherwise it may lead to confusing behavior.

This change is made to fix flakiness for svg/animations/discard-check-removal-order.html

R=pdr@chromium.org, schenney@chromium.org, dschulze@chromium.org, fmalita@chromium.org, rob.buis@samsung.com
BUG=333772
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165881

[pavane, 2014-01-15 16:14:33]

Please have a look at this patch.

Ideally, i would have liked if calculation of animated values is also done in a
sorted manner based on begin time and document order. But somehow current
animation system is made to tightly couple with {target element, target
attribute} pair. If possible i want to remove that dependency in my future
patches as it is creating problems in several other cases. Need to discuss about
this.

[pdr., 2014-01-15 22:58:33]

I don't think templates are the right approach here because they add significant
mental complexity to an already complex class. I would prefer an adapter class
to handle this, where you create PriorityCompareRefPtr that ends up calling
PriorityCompare. You can probably remove sortByPriority and move the
m_documentOrderIndexesDirty check right above the first sort call. I don't think
m_documentOrderIndexesDirty can be invalidated between the first and second call
to sort, but I'd assert that to prove it.

I don't quite follow why the second sort is required though. Can you explain it?
Does the for loop on line 284 change the elapsed order? Could you just sort at
the end, or are both sorts required?

[pavane, 2014-01-25 12:08:30]

Sorry for the delay. I was caught up with some other work.

Please review this patch.

@pdr, yes it is necessary for the animations to be sorted before calculating the
animated values. The reason behind this is in case of multiple animations, each
animation will build upon the value of previous animation. So, order is very
important. Please refer to the below example for clarity.

LayoutTests\svg\animations\resources\multiple-animations-ending.svg

Ideally we require only first sort which is applied over all animations. But
currently our animations are grouped based on target {element, attribute} pair.
So, sort is applied internal to each group. This means that along with begin
time and document order, even target is affecting the order which is a wrong
behavior.

My current patch is to make sure that it doesn't effect discard elements. So, i
am re-sorting all the ready-to-apply animations.

There are other problems with grouping animations based on target. What if the
target is not valid? It should still apply the animation according to spec.

[pavane, 2014-01-25 12:23:46]

@pdr,

Behavior was correct before the below changeset.
http://trac.webkit.org/changeset/129670

It is also affecting some functionality of discard element.

For example, <discard xlink:href="foo" begin="2s" />

Above discard has invalid target. So it should delete itself at 2s. For it to be
applied at 2s, it has to be added into scheduled animations which is not
possible currently because of invalid or already deleted target.

If you agree on this, then i can work on removing the target dependency. Need
your opinion on my inputs.

[pdr., 2014-01-27 18:33:10]

Much cleaner, thanks for fixing this up.

LGTM

[commit-bot: I haz the power, 2014-01-27 18:39:53]

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

[commit-bot: I haz the power, 2014-01-27 20:03:16]

Failed to apply the patch.
Sending        LayoutTests/TestExpectations
Sending        Source/core/svg/animation/SMILTimeContainer.cpp
Sending        Source/core/svg/animation/SMILTimeContainer.h
Transmitting file data ...svn: Commit failed (details follow):
svn: File '/trunk/LayoutTests/TestExpectations' is out of date

[commit-bot: I haz the power, 2014-01-27 20:47:53]

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

[commit-bot: I haz the power, 2014-01-27 20:48:16]

Change committed as 165881