SVG Web Animations: Add compositing pipeline for SVGInterpolationTypes

SVG Web Animations: Add compositing pipeline for SVGInterpolationTypes

This change defers application of SVG Web Animations until after we
have sampled all current Web Animations. This allows us to group
InvalidatableInterpolations into PropertyHandle specific animation
stacks and composite them appropriately.

There are no SVGInterpolationTypes implemented yet that support
non-replace composite modes to test the additive pipeline.

BUG=530436
Committed: https://crrev.com/7526ea570364bf239cdf4997702e5f6638433c90
Cr-Commit-Position: refs/heads/master@{#357030}

[alancutter (OOO until 2018), 2015-10-21 00:15:50]

alancutter@chromium.org changed reviewers:
+ dstockwell@chromium.org, pdr@chromium.org

[pdr., 2015-10-21 00:47:37]

Looks pretty good to me, just a few small things and requests for comments.

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp (right):

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp:111: for (auto&
svgElement : m_webAnimatedSVGElements) {
I'm a little worried about this for loop modifying m_webAnimatedSVGElements
during iteration. What do you think about copying m_webAnimatedSVGElements to a
local before iteration, clearing m_webAnimatedSVGElements, then asserting that
m_webAnimatedSVGElements is still empty after the loop? This will both guard
against re-entrant security bugs and will assert if it ever happens.

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp:112:
ActiveInterpolationsMap activeInterpolationsMap =
AnimationStack::activeInterpolations(&svgElement->elementAnimations()->animationStack(),
nullptr, nullptr, KeyframeEffect::DefaultPriority, 0);
We don't have a line limit but 200+ can be hard to read. Can you split this into
a few lines?

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h (right):

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:51: void
addPendingWebAnimatedSVGElement(SVGElement&);
Can you add a comment here?

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGElement.h:83: void
setWebAnimationsPending();
Can you comment this too?

[alancutter (OOO until 2018), 2015-10-21 04:20:42]

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp (right):

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp:111: for (auto&
svgElement : m_webAnimatedSVGElements) {
On 2015/10/21 at 00:47:37, pdr wrote:
> I'm a little worried about this for loop modifying m_webAnimatedSVGElements
during iteration. What do you think about copying m_webAnimatedSVGElements to a
local before iteration, clearing m_webAnimatedSVGElements, then asserting that
m_webAnimatedSVGElements is still empty after the loop? This will both guard
against re-entrant security bugs and will assert if it ever happens.

Good idea, done.

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp:112:
ActiveInterpolationsMap activeInterpolationsMap =
AnimationStack::activeInterpolations(&svgElement->elementAnimations()->animationStack(),
nullptr, nullptr, KeyframeEffect::DefaultPriority, 0);
On 2015/10/21 at 00:47:37, pdr wrote:
> We don't have a line limit but 200+ can be hard to read. Can you split this
into a few lines?

Done.

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h (right):

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:51: void
addPendingWebAnimatedSVGElement(SVGElement&);
On 2015/10/21 at 00:47:37, pdr wrote:
> Can you add a comment here?

Done.

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/svg/SVGElement.h (right):

https://codereview.chromium.org/1420553004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/svg/SVGElement.h:83: void
setWebAnimationsPending();
On 2015/10/21 at 00:47:37, pdr wrote:
> Can you comment this too?

Done.

[fs, 2015-10-21 11:24:29]

https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h (right):

https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:87: using
SVGElementSet = WillBeHeapHashSet<RawPtrWillBeMember<SVGElement>>;
In the time window between applyEffects() and serviceAnimations() - do we need
to worry about the lifetimes of the SVGElements in this set? (Once the RawPtr
part becomes irrelevant we don't - which is soon... hopefully)

[alancutter (OOO until 2018), 2015-10-26 04:13:45]

https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h (right):

https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:87: using
SVGElementSet = WillBeHeapHashSet<RawPtrWillBeMember<SVGElement>>;
On 2015/10/21 at 11:24:29, fs wrote:
> In the time window between applyEffects() and serviceAnimations() - do we need
to worry about the lifetimes of the SVGElements in this set? (Once the RawPtr
part becomes irrelevant we don't - which is soon... hopefully)

In theory SVGElements will be added and removed from m_webAnimatedSVGElements
during the call to PageAnimator::serviceScriptedAnimations() however I did not
intentionally make this a RawPtr. This is a copy+paste bug on my part from
copying m_timeContainers' type, thanks for catching it!

[pdr., 2015-10-26 04:35:36]

On 2015/10/26 at 04:13:45, alancutter wrote:
>
https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h (right):
> 
>
https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:87: using
SVGElementSet = WillBeHeapHashSet<RawPtrWillBeMember<SVGElement>>;
> On 2015/10/21 at 11:24:29, fs wrote:
> > In the time window between applyEffects() and serviceAnimations() - do we
need to worry about the lifetimes of the SVGElements in this set? (Once the
RawPtr part becomes irrelevant we don't - which is soon... hopefully)
> 
> In theory SVGElements will be added and removed from m_webAnimatedSVGElements
during the call to PageAnimator::serviceScriptedAnimations() however I did not
intentionally make this a RawPtr. This is a copy+paste bug on my part from
copying m_timeContainers' type, thanks for catching it!

LGTM

[fs, 2015-10-26 08:38:19]

On 2015/10/26 at 04:35:36, pdr wrote:
> On 2015/10/26 at 04:13:45, alancutter wrote:
> >
https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h (right):
> > 
> >
https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:87: using
SVGElementSet = WillBeHeapHashSet<RawPtrWillBeMember<SVGElement>>;
> > On 2015/10/21 at 11:24:29, fs wrote:
> > > In the time window between applyEffects() and serviceAnimations() - do we
need to worry about the lifetimes of the SVGElements in this set? (Once the
RawPtr part becomes irrelevant we don't - which is soon... hopefully)
> > 
> > In theory SVGElements will be added and removed from
m_webAnimatedSVGElements during the call to
PageAnimator::serviceScriptedAnimations() however I did not intentionally make
this a RawPtr. This is a copy+paste bug on my part from copying
m_timeContainers' type, thanks for catching it!
> 
> LGTM

LGTM2

[dstockwell, 2015-10-28 03:39:39]

lgtm

https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp (right):

https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp:64:
m_webAnimatedSVGElements.add(&element);
ASSERT runtime enabled feature?

https://codereview.chromium.org/1420553004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h (right):

https://codereview.chromium.org/1420553004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:52: void
addPendingWebAnimatedSVGElement(SVGElement&);
nit. pending what? I'd suggest something like: addAnimationPendingElement

[dstockwell, 2015-10-28 03:39:39]

lgtm

[alancutter (OOO until 2018), 2015-10-29 04:56:51]

https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp (right):

https://codereview.chromium.org/1420553004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.cpp:64:
m_webAnimatedSVGElements.add(&element);
On 2015/10/28 at 03:39:38, dstockwell wrote:
> ASSERT runtime enabled feature?

Done.

https://codereview.chromium.org/1420553004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h (right):

https://codereview.chromium.org/1420553004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:52: void
addPendingWebAnimatedSVGElement(SVGElement&);
On 2015/10/28 at 03:39:38, dstockwell wrote:
> nit. pending what? I'd suggest something like: addAnimationPendingElement

Done. Named as addWebAnimationsPendingSVGElement().

[dstockwell, 2015-10-29 05:03:28]

On 2015/10/29 at 04:56:51, alancutter wrote:
>
https://codereview.chromium.org/1420553004/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:52: void
addPendingWebAnimatedSVGElement(SVGElement&);
> On 2015/10/28 at 03:39:38, dstockwell wrote:
> > nit. pending what? I'd suggest something like: addAnimationPendingElement
> 
> Done. Named as addWebAnimationsPendingSVGElement().

We generally don't use "WebAnimations" in the code, are you trying to avoid
confusion with smil? I don't think there is any.

[alancutter (OOO until 2018), 2015-10-29 08:31:16]

On 2015/10/29 at 05:03:28, dstockwell wrote:
> On 2015/10/29 at 04:56:51, alancutter wrote:
> >
https://codereview.chromium.org/1420553004/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/svg/SVGDocumentExtensions.h:52: void
addPendingWebAnimatedSVGElement(SVGElement&);
> > On 2015/10/28 at 03:39:38, dstockwell wrote:
> > > nit. pending what? I'd suggest something like: addAnimationPendingElement
> > 
> > Done. Named as addWebAnimationsPendingSVGElement().
> 
> We generally don't use "WebAnimations" in the code, are you trying to avoid
confusion with smil? I don't think there is any.

I think it's a good idea to be explicit when SMIL and Web Animations code are
sitting next to each other in this class.

[alancutter (OOO until 2018), 2015-10-29 11:19:50]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 11:19:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420553004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420553004/120001

[commit-bot: I haz the power, 2015-10-29 12:06:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-29 12:06:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alancutter (OOO until 2018), 2015-10-29 23:26:32]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-29 23:27:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420553004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420553004/120001

[commit-bot: I haz the power, 2015-10-30 00:12:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 00:12:33]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[alancutter (OOO until 2018), 2015-10-30 00:42:37]

The CQ bit was checked by alancutter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-30 00:43:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420553004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420553004/140001

[commit-bot: I haz the power, 2015-10-30 02:00:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-30 02:00:16]

Dry run: This issue passed the CQ dry run.

[alancutter (OOO until 2018), 2015-10-30 02:29:48]

The CQ bit was checked by alancutter@chromium.org

[alancutter (OOO until 2018), 2015-10-30 02:29:49]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
dstockwell@chromium.org
Link to the patchset: https://codereview.chromium.org/1420553004/#ps140001
(title: "Fix oilpan and assert hit")

[commit-bot: I haz the power, 2015-10-30 02:30:14]

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

[commit-bot: I haz the power, 2015-10-30 02:36:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-30 02:37:35]

Patchset 8 (id:??) landed as
https://crrev.com/7526ea570364bf239cdf4997702e5f6638433c90
Cr-Commit-Position: refs/heads/master@{#357030}