Code cleanup: Replace Element with Document in element.animate()

Code cleanup: Replace Element with Document in element.animate()

Since compositor keyframes are no longer snapshot during element.animate()
we don't have a dependency on the target Element any more and can instead
use the Document.

This will make it easier to create SharedKeyframes in the future that are
decoupled from specific elements.

[alancutter (OOO until 2018), 2016-06-08 11:12:01]

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

[commit-bot: I haz the power, 2016-06-08 11:12:17]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2047293002/1

[commit-bot: I haz the power, 2016-06-08 11:56:05]

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

[commit-bot: I haz the power, 2016-06-08 11:56:06]

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), 2016-06-09 00:33:39]

alancutter@chromium.org changed reviewers:
+ ericwilligers@chromium.org

[Eric Willigers, 2016-06-09 14:48:35]

lgtm

[alancutter (OOO until 2018), 2016-07-06 07:32:42]

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

[commit-bot: I haz the power, 2016-07-06 07:32:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alancutter (OOO until 2018), 2016-07-06 08:04:37]

alancutter@chromium.org changed reviewers:
+ suzyh@chromium.org

[alancutter (OOO until 2018), 2016-07-06 08:05:42]

Description was changed from

==========
Code cleanup: Replace Element with Document in element.animate()

Since compositor keyframes are no longer snapshot during element.animate()
we don't have a dependency on the target Element any more and can instead
use the Document.
==========

to

==========
Code cleanup: Replace Element with Document in element.animate()

Since compositor keyframes are no longer snapshot during element.animate()
we don't have a dependency on the target Element any more and can instead
use the Document.

This will make it easier to create SharedKeyframes in the future that are
decoupled from specific elements.
==========

[alancutter (OOO until 2018), 2016-07-06 08:05:42]

alancutter@chromium.org changed reviewers:
- suzyh@chromium.org

[alancutter (OOO until 2018), 2016-07-06 08:05:59]

alancutter@chromium.org changed reviewers:
+ suzyh@chromium.org

[commit-bot: I haz the power, 2016-07-06 08:51:27]

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

[commit-bot: I haz the power, 2016-07-06 08:51:28]

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

[alancutter (OOO until 2018), 2016-07-07 03:34:41]

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

[commit-bot: I haz the power, 2016-07-07 03:34:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alancutter (OOO until 2018), 2016-07-07 03:34:58]

Will wait for suzyh's review before committing.

[commit-bot: I haz the power, 2016-07-07 05:47:06]

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

[commit-bot: I haz the power, 2016-07-07 05:47:08]

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

[suzyh_UTC10 (ex-contributor), 2016-07-07 05:59:44]

I'm more or less happy with this, but just have a concern about the
ExecutionContext to Document conversion.

In the CL description, please link to the "Defer compositor keyframe snapshots"
patch. That snapshot deferral patch doesn't seem to be responsible for all of
the dead Element code, so I suggest also mentioning that some Element variables
have been removed because they are no longer used/needed for other reasons.

For the win_chromium_rel_ng trybot failure, it says "TEST RESULTS WERE INVALID".
In case it helps, I saw an unhelpful error message like this once before, and
crbug.com/600612 was filed as a result.

https://codereview.chromium.org/2047293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationEffectTiming.cpp (right):

https://codereview.chromium.org/2047293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationEffectTiming.cpp:133: if
(TimingInput::setTimingFunction(timing, easing, toDocument(*executionContext),
exceptionState))
Could the toDocument conversion here ever fail, or not make sense for some other
reason? Also, what if the ExecutionContext is null?

My best guess is that this function is currently only called by the
AnimationEffectTiming generated code, and as a result will only be called with a
document as the ExecutionContext. However it's not clear to me that it is
impossible for other code to be written that passes in a different kind of
ExecutionContext.


... And I think your third patchset illustrates my concern. There needs to be
some kind of error checking here and in KeyframeEffect.cpp to prevent crashes
(or give informative/targeted error messages on crashes) in that event.

https://codereview.chromium.org/2047293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/AnimationInputHelpers.cpp (left):

https://codereview.chromium.org/2047293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/AnimationInputHelpers.cpp:189: if
(!RuntimeEnabledFeatures::webAnimationsSVGEnabled() || !element.isSVGElement()
|| !isSVGPrefixed(property))
Why do we no longer need the SVG element checks here and in the function above?

https://codereview.chromium.org/2047293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/KeyframeEffect.cpp (right):

https://codereview.chromium.org/2047293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/KeyframeEffect.cpp:59: Document&
document = toDocument(*executionContext);
As above, what happens if the ExecutionContext is not a document or is null?

https://codereview.chromium.org/2047293002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/animation/TimingInputTest.cpp (right):

https://codereview.chromium.org/2047293002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/animation/TimingInputTest.cpp:17: Timing
applyTimingInputNumber(V8TestingScope& scope, String timingProperty, double
timingPropertyValue, bool& timingConversionSuccess)
Although the change in this file was required by other changes in this patch,
the reverse does not seem to be true: this file change does not seem to depend
on the other changes. In such a situation, I would recommend landing the change
to this file as a separate patch first, in order to make each CL as small and
focused as possible. (This also makes reviewing easier.)

Since this patch is essentially ready to go, I won't block my review on
requiring this, but it's something to keep in mind for the future.

[alancutter (OOO until 2018), 2016-07-07 06:33:40]

Thanks for the thorough review, I presume ExecutionContext is always going to be
a Document unless you're in a web worker (which these functions are not exposed
to), unfortunately I can't find any IDL documentation about ExecutionContext.

+haraken: Are my above statements correct regarding ExecutionContext being
Documents?



Also I shouldn't be removing those isSVGElement() checks as that's a change in
behaviour.
I will put committing this CL on hold until the change in behaviour is removed.

[haraken, 2016-07-07 09:00:03]

On 2016/07/07 06:33:40, alancutter wrote:
> Thanks for the thorough review, I presume ExecutionContext is always going to
be
> a Document unless you're in a web worker (which these functions are not
exposed
> to), unfortunately I can't find any IDL documentation about ExecutionContext.
> 
> +haraken: Are my above statements correct regarding ExecutionContext being
> Documents?

Basically yes. But in some unit tests that don't have a Document,
ExecutionContext can be a NullExecutionContext.

You're encouraged to use ExecutionContext unless your thing is really tied with
a concept of DOM document.

[suzyh_UTC10 (ex-contributor), 2016-07-08 00:48:55]

On 2016/07/07 at 09:00:03, haraken wrote:
> On 2016/07/07 06:33:40, alancutter wrote:
> > Thanks for the thorough review, I presume ExecutionContext is always going
to be
> > a Document unless you're in a web worker (which these functions are not
exposed
> > to), unfortunately I can't find any IDL documentation about
ExecutionContext.
> > 
> > +haraken: Are my above statements correct regarding ExecutionContext being
> > Documents?
> 
> Basically yes. But in some unit tests that don't have a Document,
ExecutionContext can be a NullExecutionContext.
> 
> You're encouraged to use ExecutionContext unless your thing is really tied
with a concept of DOM document.

Where is it controlled that these functions are not exposed for web workers? It
would be nice if we could put in a guard or test of some kind to make sure that
we can't expose them to web workers without also updating these functions to
handle them.

I think a preferable approach would be to define meaningful non-crash behaviour
for these functions in the cases where the given ExecutionContext is null or not
a Document. I expect setEasing could just return without doing anything or with
some kind of logging. Is there a meaningful alternative for
KeyframeEffect::create?

[alancutter (OOO until 2018), 2016-07-08 01:14:20]

On 2016/07/08 at 00:48:55, suzyh wrote:
> Where is it controlled that these functions are not exposed for web workers?
It would be nice if we could put in a guard or test of some kind to make sure
that we can't expose them to web workers without also updating these functions
to handle them.

You control where IDL interfaces are exposed via the Exposed extended attribute,
see Blob.idl as an example.

> I think a preferable approach would be to define meaningful non-crash
behaviour for these functions in the cases where the given ExecutionContext is
null or not a Document. I expect setEasing could just return without doing
anything or with some kind of logging. Is there a meaningful alternative for
KeyframeEffect::create?

The practise in Blink has been to crash on internal errors instead of silently
failing otherwise these errors would be very hard to detect and debug. I believe
the instability of this practise is mitigated by our frequent update cycle,
pre-release channels and ClusterFuzz.

[suzyh_UTC10 (ex-contributor), 2016-07-22 01:04:09]

Has there been any update on this, with respect to the isSVGElement checks?

Also, it's just occurred to me that several of the uses of Document here are
only for UseCounters, which can operate on ExecutionContext directly. We should
consider switching to ExecutionContext for this, taking haraken's advice.

[suzyh_UTC10 (ex-contributor), 2017-03-26 23:50:11]

suzyh@chromium.org changed reviewers:
- suzyh@chromium.org