Web Animations API: Avoid style resolution when calling element.animate()

This also removes the check for an active document, because we don't need it any more.

[rjwright, 2014-05-23 06:04:10]

Ptal

[alancutter (OOO until 2018), 2014-05-23 07:01:36]

https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
File Source/core/animation/EffectInput.cpp (left):

https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:51: return element->renderer();
I think we still need to check for renderer() in case animate() is called on a
display: none element.
We should write a test for this case in another patch.

[dstockwell, 2014-05-23 07:40:58]

https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
File Source/core/animation/EffectInput.cpp (left):

https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:51: return element->renderer();
On 2014/05/23 07:01:36, alancutter wrote:
> I think we still need to check for renderer() in case animate() is called on a
> display: none element.
> We should write a test for this case in another patch.

But, we won't know whether or not there should be a renderer unless we call
updateRenderTreeIfNeeded...

[dstockwell, 2014-05-23 07:42:25]

https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
File Source/core/animation/EffectInput.cpp (left):

https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:51: return element->renderer();
On 2014/05/23 07:40:58, dstockwell wrote:
> On 2014/05/23 07:01:36, alancutter wrote:
> > I think we still need to check for renderer() in case animate() is called on
a
> > display: none element.
> > We should write a test for this case in another patch.
> 
> But, we won't know whether or not there should be a renderer unless we call
> updateRenderTreeIfNeeded...

Wait, why do we care if it's display: none?

[dstockwell, 2014-05-23 07:44:29]

https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
File Source/core/animation/EffectInput.cpp (left):

https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:51: return element->renderer();
On 2014/05/23 07:42:25, dstockwell wrote:
> On 2014/05/23 07:40:58, dstockwell wrote:
> > On 2014/05/23 07:01:36, alancutter wrote:
> > > I think we still need to check for renderer() in case animate() is called
on
> a
> > > display: none element.
> > > We should write a test for this case in another patch.
> > 
> > But, we won't know whether or not there should be a renderer unless we call
> > updateRenderTreeIfNeeded...
> 
> Wait, why do we care if it's display: none?

Ahh, because of forceConversions... Hmm, I'm not sure this patch can work.

[alancutter (OOO until 2018), 2014-05-23 08:55:24]

On 2014/05/23 07:44:29, dstockwell wrote:
>
https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
> File Source/core/animation/EffectInput.cpp (left):
> 
>
https://codereview.chromium.org/299073003/diff/20001/Source/core/animation/Ef...
> Source/core/animation/EffectInput.cpp:51: return element->renderer();
> On 2014/05/23 07:42:25, dstockwell wrote:
> > On 2014/05/23 07:40:58, dstockwell wrote:
> > > On 2014/05/23 07:01:36, alancutter wrote:
> > > > I think we still need to check for renderer() in case animate() is
called
> on
> > a
> > > > display: none element.
> > > > We should write a test for this case in another patch.
> > > 
> > > But, we won't know whether or not there should be a renderer unless we
call
> > > updateRenderTreeIfNeeded...
> > 
> > Wait, why do we care if it's display: none?
> 
> Ahh, because of forceConversions... Hmm, I'm not sure this patch can work.

We should just push checkDocumentAndRenderer() down into
forceConversionsToAnimatableValues() in its entirety and if it fails then use
the default RenderStyle instead.
See this change to StyleResolver::createAnimatableValueSnapshot():
https://codereview.chromium.org/273683005/diff/60001/Source/core/css/resolver...

I think this would more or less match our intended behaviour in these
circumstances when we remove the AnimatableValue dependency completely.

[alancutter (OOO until 2018), 2014-05-26 00:25:00]

On 2014/05/23 08:55:24, alancutter wrote:
> We should just push checkDocumentAndRenderer() down into
> forceConversionsToAnimatableValues() in its entirety and if it fails then use
> the default RenderStyle instead.

On second thought we're already broken for display: none elements so we can punt
this particular issue to a separate patch since it's unrelated to the
performance change you're doing here.

[rjwright, 2014-05-26 05:02:05]

On 2014/05/26 00:25:00, alancutter wrote:
> On 2014/05/23 08:55:24, alancutter wrote:
> > We should just push checkDocumentAndRenderer() down into
> > forceConversionsToAnimatableValues() in its entirety and if it fails then
use
> > the default RenderStyle instead.
> 
> On second thought we're already broken for display: none elements so we can
punt
> this particular issue to a separate patch since it's unrelated to the
> performance change you're doing here.

Consensus reached offline:
Check for a renderer after the call to updateRenderTreeIfNeeded in
StringKeyframe::PropertySpecificKeyframe::createInterpolation(added in this CL)
and throw an exception if we don't find one. This code is temporary, and the
renderer will eventually be removed from the process all together.

[alancutter (OOO until 2018), 2014-05-27 23:37:57]

Eric's responsive interpolation patch has landed.
https://codereview.chromium.org/292173009/
We should be able to remove checkDocumentAndRenderer() completely now.

[rjwright, 2014-06-01 03:44:17]

On 2014/05/27 23:37:57, alancutter wrote:
> Eric's responsive interpolation patch has landed.
> https://codereview.chromium.org/292173009/
> We should be able to remove checkDocumentAndRenderer() completely now.

OK. So we don't need to check for a renderer anywhere?

[alancutter (OOO until 2018), 2014-06-01 04:01:34]

On 2014/06/01 03:44:17, rjwright wrote:
> On 2014/05/27 23:37:57, alancutter wrote:
> > Eric's responsive interpolation patch has landed.
> > https://codereview.chromium.org/292173009/
> > We should be able to remove checkDocumentAndRenderer() completely now.
> 
> OK. So we don't need to check for a renderer anywhere?

We still need a RenderStyle to call createAnimatableValueSnapshot() it just
doesn't need to be the Element's anymore since we only hit this path for values
that don't depend on other values in the RenderStyle.
You can probably just DEFINE_STATIC_REF() one for createInterpolation().

[rjwright, 2014-06-01 05:19:14]

PTAL

[alancutter (OOO until 2018), 2014-06-01 08:10:01]

lgtm

https://codereview.chromium.org/299073003/diff/40001/Source/core/animation/Ef...
File Source/core/animation/EffectInput.cpp (left):

https://codereview.chromium.org/299073003/diff/40001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:45: // FIXME: Remove this once we've
removed the dependency on Element.
We should keep a FIXME for the dependency on Element that we still have.

[dstockwell, 2014-06-01 23:59:31]

lgtm

Need to update the patch title and description too.

https://codereview.chromium.org/299073003/diff/40001/Source/core/animation/Ef...
File Source/core/animation/EffectInput.cpp (left):

https://codereview.chromium.org/299073003/diff/40001/Source/core/animation/Ef...
Source/core/animation/EffectInput.cpp:45: // FIXME: Remove this once we've
removed the dependency on Element.
On 2014/06/01 08:10:01, alancutter wrote:
> We should keep a FIXME for the dependency on Element that we still have.

What is the dependency on element that we still have? Can we just remove it now?

[alancutter (OOO until 2018), 2014-06-02 00:44:04]

On 2014/06/01 23:59:31, dstockwell wrote:
> lgtm
> 
> Need to update the patch title and description too.
> 
>
https://codereview.chromium.org/299073003/diff/40001/Source/core/animation/Ef...
> File Source/core/animation/EffectInput.cpp (left):
> 
>
https://codereview.chromium.org/299073003/diff/40001/Source/core/animation/Ef...
> Source/core/animation/EffectInput.cpp:45: // FIXME: Remove this once we've
> removed the dependency on Element.
> On 2014/06/01 08:10:01, alancutter wrote:
> > We should keep a FIXME for the dependency on Element that we still have.
> 
> What is the dependency on element that we still have? Can we just remove it
now?

Hmm, it's actually a dependency on Document.
The absolute path for relative URLs is determined during parse time, we'll need
to change this behaviour to be able to occur at style resolve time instead to
remove the dependency.

[rjwright, 2014-06-04 04:02:47]

The CQ bit was checked by rjwright@chromium.org

[commit-bot: I haz the power, 2014-06-04 04:03:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rjwright@chromium.org/299073003/40001

[commit-bot: I haz the power, 2014-06-04 04:58:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10197)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10522)

[alancutter (OOO until 2018), 2014-06-04 05:06:21]

The CQ bit was unchecked by alancutter@chromium.org

[alancutter (OOO until 2018), 2014-06-04 05:07:18]

On 2014/06/04 04:58:19, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> The failing builders are:
>   linux_blink_dbg on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
>   linux_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
>   mac_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/10197)
>   win_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/10522)

This is crashing on:
animations/interpolation/font-size-interpolation.html
animations/interpolation/outline-width-interpolation.html
web-animations-api/animation-constructor.html