Web Animations API: Load resources referenced in element.animate()

Web Animations API: Load resources referenced in element.animate()

This patch fixes a bug where resources in element.animate() calls were
not being fetched. This resulted in StylePendingImages leaking out of
the style resolving stage and into rendering which causes a crash.

This patch fixes the bug by ensuring StylePendingImages introduced in
applyAnimatedProperties() will be resolved as non-pending StyleImages
through an additional call to loadPendingResources().
This patch also modifies AnimatableImage to be a wrapper of CSSValues
instead of StyleImages in order to follow the same resource caching
logic as typical CSS image values.
This patch also updates EffectInput::convert() to pass the document's
StyleSheetContents to MutableStylePropertySet::setProperty() so that
relative URLs are correctly resolved to absolute URLs.

BUG=353385
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169424

[alancutter (OOO until 2018), 2014-03-18 00:52:34]

This patch basically rewrites AnimatableImage to wrap CSSValues in order to
integrate properly with the image caching logic.

+dstockwell for review.

[dstockwell, 2014-03-18 01:02:34]

This is a big patch. Can you update the description to explain what the bug was
and how this patch fixes it.

Is this related to how EffectInput::convert resolves styles? Why do CSS Keyframe
animations not have this problem?

https://codereview.chromium.org/196573030/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimatableImage.cpp (left):

https://codereview.chromium.org/196573030/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimatableImage.cpp:73: // FIXME: Once cross-fade works on
generated image types, remove this check.
Does this work now?

[alancutter (OOO until 2018), 2014-03-18 02:43:52]

On 2014/03/18 01:02:34, dstockwell wrote:
> This is a big patch. Can you update the description to explain what the bug
was
> and how this patch fixes it.

Done.

On 2014/03/18 01:02:34, dstockwell wrote:
> Is this related to how EffectInput::convert resolves styles?

Correct, EffectInput::convert() was not resolving StylePendingImages before
building AnimatableImages and animated values were being applied after
loadPendingResources() got called, thus the StylePendingImages were never
resolved before being passed to the rendering stage.
I had attempted moving the loadPendingResources() call to after
applyAnimatedProperties() and simply catch the StylePendingImages that got
applied however this interferes with CSS transition start detection as
loadPendingResources() actually modifies the CSSImageValues such that they apply
a cached StyleImage the next time they are applied which is not equal to the
StylePendingImage that was applied previously. The transition logic asserts that
changes like that do not occur during a transition animation so by resolving the
pending resources before the transition logic sees the render style (as we do
now) this caching behaviour is not visible and as far as it knows the assertion
remains true. This is similar reasoning to performing RenderStyle adjustment
prior to applying animated properties.

On 2014/03/18 01:02:34, dstockwell wrote:
> Why do CSS Keyframe animations not have this problem?

CSS Keyframes don't have this problem because they go through styleForKeyframe()
which resolves StylePendingImages properly.
I could have gone the route of updating createKeyframeEffectModel() to do the
same however using StyleBuilder::applyProperty() is not the way we want to
resolve KeyframeEffects in the long run. This change enables us to move forwards
towards animating CSS values instead of RenderStyle values.

https://codereview.chromium.org/196573030/diff/1/Source/core/animation/Animat...
File Source/core/animation/AnimatableImage.cpp (left):

https://codereview.chromium.org/196573030/diff/1/Source/core/animation/Animat...
Source/core/animation/AnimatableImage.cpp:73: // FIXME: Once cross-fade works on
generated image types, remove this check.
On 2014/03/18 01:02:35, dstockwell wrote:
> Does this work now?

This FIXME is still on the usesDefaultInterpolationWith() definiton.
interpolateTo() shouldn't need updating if it changes.
Removed the isImageValue() asserts.

[alancutter (OOO until 2018), 2014-03-18 02:46:54]

+esprehn for StyleResolver changes.

[esprehn, 2014-03-18 02:58:55]

lgtm, one comment.

https://codereview.chromium.org/196573030/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/196573030/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:1094: loadPendingResources(state);
The callers of this should do this? It seems like the last step of any
styleFor*() method should be "load pending resources".

[alancutter (OOO until 2018), 2014-03-18 03:54:30]

https://codereview.chromium.org/196573030/diff/20001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/196573030/diff/20001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:1094: loadPendingResources(state);
On 2014/03/18 02:58:56, esprehn wrote:
> The callers of this should do this? It seems like the last step of any
> styleFor*() method should be "load pending resources".

I put it inside applyAnimatedProperties() to match the structure of
applyMatchedProperties(StyleResolverState&, const MatchResult&) which also does
it at the end.
Perhaps the calls to loadPendingResources() should be pulled out of
applyMatchingProperties() and applyAnimatedProperties() and explicitly called in
the styleFor*() methods?

[esprehn, 2014-03-18 06:26:09]

On 2014/03/18 03:54:30, alancutter wrote:
>
https://codereview.chromium.org/196573030/diff/20001/Source/core/css/resolver...
> File Source/core/css/resolver/StyleResolver.cpp (right):
> 
>
https://codereview.chromium.org/196573030/diff/20001/Source/core/css/resolver...
> Source/core/css/resolver/StyleResolver.cpp:1094: loadPendingResources(state);
> On 2014/03/18 02:58:56, esprehn wrote:
> > The callers of this should do this? It seems like the last step of any
> > styleFor*() method should be "load pending resources".
> 
> I put it inside applyAnimatedProperties() to match the structure of
> applyMatchedProperties(StyleResolverState&, const MatchResult&) which also
does
> it at the end.
> Perhaps the calls to loadPendingResources() should be pulled out of
> applyMatchingProperties() and applyAnimatedProperties() and explicitly called
in
> the styleFor*() methods?

If I remember right that had something to do with the matched properties cache.
Perhaps we should land this and then sort that out later.

[esprehn, 2014-03-18 06:27:05]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2014-03-18 06:27:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alancutter@chromium.org/196573030/20001

[commit-bot: I haz the power, 2014-03-18 07:30:20]

Change committed as 169424

[alancutter (OOO until 2018), 2014-03-19 10:59:19]

A revert of this CL has been created in
https://codereview.chromium.org/203413008/ by alancutter@chromium.org.

The reason for reverting is: Causing assertion failure in ChromeOS
browser_tests.
https://code.google.com/p/chromium/issues/detail?id=353962.