Simplify and test SVGInterpolation effect application

Simplify and test SVGInterpolation effect application

This patch is a simple clean up of SVGInterpolation
effect application with an added test to ensure it
continues to work for use elements.
There is no change in behaviour in this patch.

Committed: https://crrev.com/f09e500334a9c5867b7f93ecf984c2448296f37f
Cr-Commit-Position: refs/heads/master@{#364237}

[alancutter (OOO until 2018), 2015-12-09 06:27:21]

Description was changed from

==========
+dontApplyToInstances

BUG=
==========

to

==========
Simplify SVG Web Animation effect application

This patch changes SVG Web Animations to not apply animation effects
to
==========

[alancutter (OOO until 2018), 2015-12-09 06:27:21]

alancutter@chromium.org changed reviewers:
+ ericwilligers@chromium.org, fs@opera.com

[alancutter (OOO until 2018), 2015-12-09 06:28:08]

Description was changed from

==========
Simplify SVG Web Animation effect application

This patch changes SVG Web Animations to not apply animation effects
to
==========

to

==========
Simplify SVG Web Animation effect application

This patch changes SVG Web Animations to not apply animation
effects to all element instances as it is unnecessary.
There is no change in behaviour in this patch.
==========

[Eric Willigers, 2015-12-09 06:39:14]

lgtm

[fs, 2015-12-09 12:19:08]

Explanation for why I think this code may be good to keep follow. Sorry for
chasing you up the wrong tree with my somewhat unclear comment in the other
review =/. If you end up keeping code, maybe land the test still?

https://codereview.chromium.org/1512483004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (left):

https://codereview.chromium.org/1512483004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGElement.cpp:249:
SVGElement::InstanceUpdateBlocker blocker(element);
So, the "benefit" of the scheme you're removing can be explained with this
single line (I hope...) Here we say "prevent the regular instance invalidation
logic from doing anything - we will make sure the instances are up-to-date
'manually'". So the "pro" is that we don't need to tear down and recreate the
instances for each mutation (or, slightly more correct, "frame"). The "con" is
of course complexity...
We use this pattern in the SMIL code (SVGAnimateElement,
SVGAnimatedTypeAnimator) for the above "pro" (AFAIK at least). It's open-coded
in all the places though, so if we could have it once (in SVGElement, preferably
folding SVGAnimateElement::findElementInstances into it) that would be an
improvement too I think. Doesn't necessarily have to imply you doing any of the
legwork either =)

[alancutter (OOO until 2018), 2015-12-09 22:13:51]

Description was changed from

==========
Simplify SVG Web Animation effect application

This patch changes SVG Web Animations to not apply animation
effects to all element instances as it is unnecessary.
There is no change in behaviour in this patch.
==========

to

==========
Simplify and test SVGInterpolation effect application

This patch is a simple clean up of SVGInterpolation
effect application with an added test to ensure it
continues to work for use elements.
There is no change in behaviour in this patch.
==========

[alancutter (OOO until 2018), 2015-12-09 22:14:12]

https://codereview.chromium.org/1512483004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGElement.cpp (left):

https://codereview.chromium.org/1512483004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGElement.cpp:249:
SVGElement::InstanceUpdateBlocker blocker(element);
On 2015/12/09 at 12:19:08, fs wrote:
> So, the "benefit" of the scheme you're removing can be explained with this
single line (I hope...) Here we say "prevent the regular instance invalidation
logic from doing anything - we will make sure the instances are up-to-date
'manually'". So the "pro" is that we don't need to tear down and recreate the
instances for each mutation (or, slightly more correct, "frame"). The "con" is
of course complexity...
> We use this pattern in the SMIL code (SVGAnimateElement,
SVGAnimatedTypeAnimator) for the above "pro" (AFAIK at least). It's open-coded
in all the places though, so if we could have it once (in SVGElement, preferably
folding SVGAnimateElement::findElementInstances into it) that would be an
improvement too I think. Doesn't necessarily have to imply you doing any of the
legwork either =)

Ah so it's an optimisation, that makes sense why it behaved correctly. Thanks
for the explanation, I'll make this patch just the test and the duplicate code
removal.

[fs, 2015-12-09 22:18:37]

On 2015/12/09 at 22:14:12, alancutter wrote:
>
https://codereview.chromium.org/1512483004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/svg/SVGElement.cpp (left):
> 
>
https://codereview.chromium.org/1512483004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/svg/SVGElement.cpp:249:
SVGElement::InstanceUpdateBlocker blocker(element);
> On 2015/12/09 at 12:19:08, fs wrote:
> > So, the "benefit" of the scheme you're removing can be explained with this
single line (I hope...) Here we say "prevent the regular instance invalidation
logic from doing anything - we will make sure the instances are up-to-date
'manually'". So the "pro" is that we don't need to tear down and recreate the
instances for each mutation (or, slightly more correct, "frame"). The "con" is
of course complexity...
> > We use this pattern in the SMIL code (SVGAnimateElement,
SVGAnimatedTypeAnimator) for the above "pro" (AFAIK at least). It's open-coded
in all the places though, so if we could have it once (in SVGElement, preferably
folding SVGAnimateElement::findElementInstances into it) that would be an
improvement too I think. Doesn't necessarily have to imply you doing any of the
legwork either =)
> 
> Ah so it's an optimisation, that makes sense why it behaved correctly. Thanks
for the explanation, I'll make this patch just the test and the duplicate code
removal.

lgtm

[alancutter (OOO until 2018), 2015-12-09 22:24:47]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2015-12-09 22:24:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
ericwilligers@chromium.org
Link to the patchset: https://codereview.chromium.org/1512483004/#ps20001
(title: "Remove removal")

[commit-bot: I haz the power, 2015-12-09 22:27:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1512483004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1512483004/20001

[commit-bot: I haz the power, 2015-12-10 01:10:15]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-10 01:11:08]

Description was changed from

==========
Simplify and test SVGInterpolation effect application

This patch is a simple clean up of SVGInterpolation
effect application with an added test to ensure it
continues to work for use elements.
There is no change in behaviour in this patch.
==========

to

==========
Simplify and test SVGInterpolation effect application

This patch is a simple clean up of SVGInterpolation
effect application with an added test to ensure it
continues to work for use elements.
There is no change in behaviour in this patch.

Committed: https://crrev.com/f09e500334a9c5867b7f93ecf984c2448296f37f
Cr-Commit-Position: refs/heads/master@{#364237}
==========

[commit-bot: I haz the power, 2015-12-10 01:11:09]

Patchset 2 (id:??) landed as
https://crrev.com/f09e500334a9c5867b7f93ecf984c2448296f37f
Cr-Commit-Position: refs/heads/master@{#364237}