Initial implementation of WorkletAnimation

Initial implementation of main thread WorkletAnimation IDL

This change adds a WorkletAnimation object accessible in the main js scope,
when the CompositorWorker (the old name for AnimationWorklet) Blink runtime 
feature is enabled. WorkletAnimation objects are hooked up to the document's 
animation lifecycle, owned by a WorkletAnimationController. The controller
will attempt to start them on the compositor, but this currently fails due
to lack of support in the animation logic.

BUG=720080
Review-Url: https://chromiumcodereview.appspot.com/2869183002
Cr-Commit-Position: refs/heads/master@{#501292}
Committed: https://chromium.googlesource.com/chromium/src/+/c7af2f07e7458da6bab556dd3a2f2dcac3f73276

[smcgruer, 2017-05-12 15:08:47]

smcgruer@chromium.org changed reviewers:
+ flackr@chromium.org, majidvp@chromium.org

[smcgruer, 2017-05-12 15:08:48]

Very WIP still, but just pinging you two as an FYI. Amazingly this code can
actually create a WorkletAnimation without the world melting down - not that it
actually does anything yet :).

http://output.jsbin.com/nazifomivo/quiet

[flackr, 2017-05-12 18:08:23]

Needs some tests and probably some TODO's about actually sending them over in
the commit but looks like a good start.

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:21:
*error_string = "ScrollTimeline timeRange must have non-auto value";
nit: Maybe "ScrollTimeline timeRange auto is not supported"? have non-auto value
read a bit confusing and this is technically a temporary error until we can
resolve the timeRange against the animation.

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:54:
Document& document = effects.at(0)->Target()->GetDocument();
effects.at(0) seems a bit arbitrary, perhaps we should verify all of the effects
have the same document?

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:75: //
Shut the compiler up.
Not very nice to the compiler, fortunately I hear it doesn't read comments it
just skips right over them.

[flackr, 2017-05-12 18:09:41]

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/generated.gni (right):

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/generated.gni:36:
"$bindings_modules_v8_output_dir/DocumentTimelineOrScrollTimeline.h",
Are these files missing from the patch?

[smcgruer, 2017-05-18 17:58:39]

Addressing some comments flackr made, plus a general code update. It's all still
very broken and very WIP, I don't suggest reviewing it :)

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/modules/v8/generated.gni (right):

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/generated.gni:36:
"$bindings_modules_v8_output_dir/DocumentTimelineOrScrollTimeline.h",
On 2017/05/12 18:09:41, flackr wrote:
> Are these files missing from the patch?

No; they are generated files.

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:21:
*error_string = "ScrollTimeline timeRange must have non-auto value";
On 2017/05/12 18:08:23, flackr wrote:
> nit: Maybe "ScrollTimeline timeRange auto is not supported"? have non-auto
value
> read a bit confusing and this is technically a temporary error until we can
> resolve the timeRange against the animation.

Done.

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:54:
Document& document = effects.at(0)->Target()->GetDocument();
On 2017/05/12 18:08:22, flackr wrote:
> effects.at(0) seems a bit arbitrary, perhaps we should verify all of the
effects
> have the same document?

Yes, we should. Done.

https://codereview.chromium.org/2869183002/diff/50001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:75: //
Shut the compiler up.
On 2017/05/12 18:08:22, flackr wrote:
> Not very nice to the compiler, fortunately I hear it doesn't read comments it
> just skips right over them.

Heh. Just temporary, of course.

[smcgruer, 2017-06-02 19:42:30]

Ok, I think this is about ready for your thoughts (not saying ready for
submit!). Input welcome :)

[flackr, 2017-06-05 19:08:39]

https://codereview.chromium.org/2869183002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationBase.h (right):

https://codereview.chromium.org/2869183002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationBase.h:18: //
TODO(smcgruer): Rename.
What does update mean? Run animations?

https://codereview.chromium.org/2869183002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:104: if
(Platform::Current()->IsThreadedAnimationEnabled() &&
Eventually we'd have to support running without threaded animation right? But
for now I think it would be simpler to assume threaded animation is enabled
(i.e. assert it and/or throw an error if we don't have it enabled), and then we
don't need to have all of these nullptr checks right?

[smcgruer, 2017-06-06 13:22:11]

https://codereview.chromium.org/2869183002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationBase.h (right):

https://codereview.chromium.org/2869183002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationBase.h:18: //
TODO(smcgruer): Rename.
On 2017/06/05 19:08:39, flackr wrote:
> What does update mean? Run animations?

No, it's analogous to CompositorPendingAnimations::Update. Essentially means
'start any pending animations on the compositor', though our logic is much
simpler.

https://codereview.chromium.org/2869183002/diff/350001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/350001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:104: if
(Platform::Current()->IsThreadedAnimationEnabled() &&
On 2017/06/05 19:08:39, flackr wrote:
> Eventually we'd have to support running without threaded animation right? But
> for now I think it would be simpler to assume threaded animation is enabled
> (i.e. assert it and/or throw an error if we don't have it enabled), and then
we
> don't need to have all of these nullptr checks right?

Sure, done. I vaguely wonder why we even have --disable-threaded-animation, but
*shrug*.

[majidvp, 2017-06-06 16:04:28]

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp:17:
animations_.insert(animation);
nit: may be a good idea to add a DCHECK for animation not already
existing in the list.
This will help avoid attaching animation multiple times by mistake.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationController.h
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.h:24: void
DeregisterAnimation(WorkletAnimationBase*);
nit: AnimationTimeline uses attach/detach terminology. Perhaps we should match
that.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:6229: }
Can we just expose worklet_animation_controller_ and just call these methods
directly? e.g., Document().WorkletAnimationController().RegisterAnimation(...)

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1142: void
RegisterWorkletAnimation(WorkletAnimationBase*);
nit: should take a const ref.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:79:
AnimationTimeline& main_document_timeline = document.Timeline();
nit: s/main_document_timeline/document_timeline/. I don't see why it needs to be
prefixed with "main".

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:111: if
(!on_compositor_) {
The current logic keeps trying to start the animation on compositor even if it
fails every time.
The main problem IMHO is that it fails silently to start animations without any
indication to
the user.


I suggest the following:
A) Rather than trying again, we should just fail and report an error
   with a TODO to support falling back to running on main thread.

B) Check if the pre-conditions are met for the animation to run on compositor:
  1) Check if effect targets are composited e.g.,
CompositorAnimations::CanStartAnimationOnCompositor
  2) Check if the effects are eligible e.g.,
CompositorAnimations::IsCandidateForAnimationOnCompositor

I dont think you need to have (B) for this patch at this point but having (A)
will be 
a nice addition.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:127: if
(compositor_player_) {
I agree with flackr@ that we should for now assume that we always
create the compositor_player_.
Even if you decide otherwise, I suggest turning these checks into
early exists e.g.,

if (!compositor_player_)
  return;

[majidvp, 2017-06-06 17:23:14]

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:79: class
CompositorAnimationPlayerHolder
Perhaps we should make this a public class in CompositorAnimationPlayer
e.g., CompositorAnimationPlayer::Holder.

This can work with either a WorkletAnimation or Animation.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:11:
SerializedScriptValue user_data),
nit: 'options' feels more appropriate to me than |user_data|

[flackr, 2017-06-07 17:21:33]

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:77: //
TODO(smcgruer): Document.
What do you mean by Document? This should maybe check that all of the effects
have the same document, right?

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:120:
on_compositor_ = effects_.at(0)->MaybeStartAnimationOnCompositor(
Why do we just start the first effect? Does that start the whole animation? If
so, please document.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:127: if
(compositor_player_) {
On 2017/06/06 16:04:28, majidvp wrote:
> I agree with flackr@ that we should for now assume that we always
> create the compositor_player_.
> Even if you decide otherwise, I suggest turning these checks into
> early exists e.g.,
> 
> if (!compositor_player_)
>   return;

I would suggest just removing them. When we add main thread support maybe we
won't call AttachCompositorTimeline if we're not composited?

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:131: if
(timeline)
It looks like this should always be set as long as threaded animations are
enabled.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:51:
LOG(INFO) << "WorkletAnimation::NotifyAnimationAborted";
This was probably testing code, but I don't think LOG should be in inline
functions since they could produce a non-trivial amount of code.

[smcgruer, 2017-06-07 18:23:26]

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp:17:
animations_.insert(animation);
On 2017/06/06 16:04:27, majidvp wrote:
> nit: may be a good idea to add a DCHECK for animation not already
> existing in the list.
> This will help avoid attaching animation multiple times by mistake.

Done.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationController.h
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.h:24: void
DeregisterAnimation(WorkletAnimationBase*);
On 2017/06/06 16:04:27, majidvp wrote:
> nit: AnimationTimeline uses attach/detach terminology. Perhaps we should match
> that.

Hrm, but CompositorPendingAnimations uses 'Add'. Such conflict! Changed to
attach, and dropped the detach method since it isn't called atm. We definitely
need to work out lifecycle for WorkletAnimation.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:6229: }
On 2017/06/06 16:04:27, majidvp wrote:
> Can we just expose worklet_animation_controller_ and just call these methods
> directly? e.g., Document().WorkletAnimationController().RegisterAnimation(...)

Done.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:1142: void
RegisterWorkletAnimation(WorkletAnimationBase*);
On 2017/06/06 16:04:27, majidvp wrote:
> nit: should take a const ref.
Made it take a ref (in WorkletAnimationController) so that it definitely exists,
but const ref can't be inserted into a HeapHashSet<Member<..>>?

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:77: //
TODO(smcgruer): Document.
On 2017/06/07 17:21:33, flackr wrote:
> What do you mean by Document? This should maybe check that all of the effects
> have the same document, right?

It was a reminder to myself to document this code, mostly leftover from when we
did more here. Irrelevant in new PS anyway.`

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:79:
AnimationTimeline& main_document_timeline = document.Timeline();
On 2017/06/06 16:04:28, majidvp wrote:
> nit: s/main_document_timeline/document_timeline/. I don't see why it needs to
be
> prefixed with "main".

Done.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:111: if
(!on_compositor_) {
On 2017/06/06 16:04:27, majidvp wrote:
> The current logic keeps trying to start the animation on compositor even if it
> fails every time.
> The main problem IMHO is that it fails silently to start animations without
any
> indication to
> the user.
> 
> 
> I suggest the following:
> A) Rather than trying again, we should just fail and report an error
>    with a TODO to support falling back to running on main thread.
> 
> B) Check if the pre-conditions are met for the animation to run on compositor:
>   1) Check if effect targets are composited e.g.,
> CompositorAnimations::CanStartAnimationOnCompositor
>   2) Check if the effects are eligible e.g.,
> CompositorAnimations::IsCandidateForAnimationOnCompositor
> 
> I dont think you need to have (B) for this patch at this point but having (A)
> will be 
> a nice addition.

Any thoughts on how to actually report an error from here?

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:120:
on_compositor_ = effects_.at(0)->MaybeStartAnimationOnCompositor(
On 2017/06/07 17:21:33, flackr wrote:
> Why do we just start the first effect? Does that start the whole animation? If
> so, please document.

Well the CL doesn't really *do* anything currently, so this is mostly testing
code. It does end up making it to CompositorAnimationPlayer::AddAnimation, but
not much beyond. As far as I understand, we're going to be introducing
WorkletAnimationPlayer or similar anyway which will replace that class, so it
didn't seem useful to get it moving further.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:127: if
(compositor_player_) {
On 2017/06/07 17:21:33, flackr wrote:
> On 2017/06/06 16:04:28, majidvp wrote:
> > I agree with flackr@ that we should for now assume that we always
> > create the compositor_player_.
> > Even if you decide otherwise, I suggest turning these checks into
> > early exists e.g.,
> > 
> > if (!compositor_player_)
> >   return;
> 
> I would suggest just removing them. When we add main thread support maybe we
> won't call AttachCompositorTimeline if we're not composited?

Done.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:131: if
(timeline)
On 2017/06/07 17:21:32, flackr wrote:
> It looks like this should always be set as long as threaded animations are
> enabled.

Done.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:51:
LOG(INFO) << "WorkletAnimation::NotifyAnimationAborted";
On 2017/06/07 17:21:33, flackr wrote:
> This was probably testing code, but I don't think LOG should be in inline
> functions since they could produce a non-trivial amount of code.

It was testing code, removed.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:79: class
CompositorAnimationPlayerHolder
On 2017/06/06 17:23:14, majidvp wrote:
> Perhaps we should make this a public class in CompositorAnimationPlayer
> e.g., CompositorAnimationPlayer::Holder.
> 
> This can work with either a WorkletAnimation or Animation.
> 
> 

Difficult; they take an Animation*/WorkletAnimation* in the Create() so until
those share a base class it'd be ugly.

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl
(right):

https://codereview.chromium.org/2869183002/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:11:
SerializedScriptValue user_data),
On 2017/06/06 17:23:14, majidvp wrote:
> nit: 'options' feels more appropriate to me than |user_data|

Done.

[flackr, 2017-06-19 20:35:33]

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:122: //
TODO(smcgruer): Should this set NeedsCompositingUpdate?
The WorkletAnimationController should I think.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:143:
compositor_player_->Player());
I'm a bit confused about what this is doing. We're just starting one of the
effects? Should we start all of them instead? Is plumbing needed on the cc side
to recognize that these are WorkletAnimation effects which shouldn't start
running immediately?

[alancutter (OOO until 2018), 2017-06-26 11:56:59]

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

[alancutter (OOO until 2018), 2017-06-26 11:57:02]

The DCHECK(GetAnimation()) in KeyframeEffectReadOnly is a hurdle in having these
AnimationWorklets actually apply effects on the main thread. After a bit of
digging I propose replacing the "Member<Animation> animation_;" on
AnimationEffectReadOnly with "Member<AnimationEffectOwnerDelegate>
effect_owner_;" and virtualise the parts that KeyframeEffectReadOnly requires.
This will be things like notifying of mutations to the effect and querying the
play state.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationBase.h (right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationBase.h:13: class
CORE_EXPORT WorkletAnimationBase
No need for this base class, let's not have it.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:23:
String* error_string) {
Blink style is to use references instead of pointers for parameters that are
never null.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:138:
static_cast<KeyframeEffectModelBase*>(effects_.at(0)->Model())
Use ToKeyframeEffectModelBase().

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:141:
bool success = effects_.at(0)->MaybeStartAnimationOnCompositor(
MaybeStartAnimationOnCompositor() is now
CanStartAnimationOnCompositor()/StartAnimationOnCompositor(). The former gives
an error message when an animation can't be started on the compositor.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:154: }
I'm not sure we need these two methods given they're one liners and only called
once.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:70: void
SetEffectInheritedTimeForTesting(Member<KeyframeEffectReadOnly>, double);
We should remove this if we're not using it.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:82: // do
this, or remove the layer of indirection.
We need to do some clean up in Dispose() that's not allowed during destruction
of a GC'd object.

https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:116:
Member<CompositorAnimationPlayerHolder> compositor_player_;
Shouldn't this be a vector of compositor_players to match the vector of effects?

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimationTest.cpp
(right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimationTest.cpp:19:
TEST(WorkletAnimationTest, CreateWorkletAnimation) {
Please add tests like this under LayoutTests/virtual/threaded. This unit test is
okay to keep.

You'll want to add a ref test to check pixel outputs to ensure compositor
animations are actually taking effect.
virtual/threaded/animations/composited-animation-style-update.html is an example
of this.

[smcgruer, 2017-06-26 21:14:08]

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationBase.h (right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationBase.h:13: class
CORE_EXPORT WorkletAnimationBase
On 2017/06/26 11:57:01, alancutter wrote:
> No need for this base class, let's not have it.

I believe it is required; core/ can't depend on modules/, can it?

We could of course move the code added from modules/compositorworker/ to
core/animation/. I wasn't clear on where new worklets are meant to live;
PaintWorklet was implemented in modules/

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:23:
String* error_string) {
On 2017/06/26 11:57:02, alancutter wrote:
> Blink style is to use references instead of pointers for parameters that are
> never null.

Done.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:138:
static_cast<KeyframeEffectModelBase*>(effects_.at(0)->Model())
On 2017/06/26 11:57:02, alancutter wrote:
> Use ToKeyframeEffectModelBase().

Done.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:141:
bool success = effects_.at(0)->MaybeStartAnimationOnCompositor(
On 2017/06/26 11:57:02, alancutter wrote:
> MaybeStartAnimationOnCompositor() is now
> CanStartAnimationOnCompositor()/StartAnimationOnCompositor(). The former gives
> an error message when an animation can't be started on the compositor.

Ack; I see that there is now a new cl to make them
Check{CanStartAnimationOnCompositor()/StartAnimationOnCompositor()} as well.
I'll rebase onto that one tomorrow and start using them properly. For now, just
fixed the code to compile with my current checkout.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:143:
compositor_player_->Player());
On 2017/06/19 20:35:33, flackr wrote:
> I'm a bit confused about what this is doing. We're just starting one of the
> effects? Should we start all of them instead? Is plumbing needed on the cc
side
> to recognize that these are WorkletAnimation effects which shouldn't start
> running immediately? 

As far as I understand it, a lot of plumbing is still needed on the cc side for
this CL to be worth anything. But I still don't understand either the proposed
or realized cc-side changes, so I think we need to sync offline about this.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:154: }
On 2017/06/26 11:57:02, alancutter wrote:
> I'm not sure we need these two methods given they're one liners and only
called
> once.

Done.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:70: void
SetEffectInheritedTimeForTesting(Member<KeyframeEffectReadOnly>, double);
On 2017/06/26 11:57:02, alancutter wrote:
> We should remove this if we're not using it.

Leftover, removed.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:116:
Member<CompositorAnimationPlayerHolder> compositor_player_;
On 2017/06/26 11:57:02, alancutter wrote:
> Shouldn't this be a vector of compositor_players to match the vector of
effects?

This goes back to flackr's comment in WorkletAnimation.cpp. We need to have a
discussion about how AnimationWorklet is going to work on the cc side before
this CL can be considered to do anything useful.

[alancutter (OOO until 2018), 2017-06-27 00:15:52]

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationBase.h (right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationBase.h:13: class
CORE_EXPORT WorkletAnimationBase
On 2017/06/26 at 21:14:08, smcgruer wrote:
> On 2017/06/26 11:57:01, alancutter wrote:
> > No need for this base class, let's not have it.
> 
> I believe it is required; core/ can't depend on modules/, can it?
> 
> We could of course move the code added from modules/compositorworker/ to
core/animation/. I wasn't clear on where new worklets are meant to live;
PaintWorklet was implemented in modules/

Oops, I missed the part where it lives in modules.

[smcgruer, 2017-06-27 18:02:43]

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:141:
bool success = effects_.at(0)->MaybeStartAnimationOnCompositor(
On 2017/06/26 21:14:08, smcgruer wrote:
> On 2017/06/26 11:57:02, alancutter wrote:
> > MaybeStartAnimationOnCompositor() is now
> > CanStartAnimationOnCompositor()/StartAnimationOnCompositor(). The former
gives
> > an error message when an animation can't be started on the compositor.
> 
> Ack; I see that there is now a new cl to make them
> Check{CanStartAnimationOnCompositor()/StartAnimationOnCompositor()} as well.
> I'll rebase onto that one tomorrow and start using them properly. For now,
just
> fixed the code to compile with my current checkout.

Done.

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:143:
compositor_player_->Player());
On 2017/06/26 21:14:08, smcgruer wrote:
> On 2017/06/19 20:35:33, flackr wrote:
> > I'm a bit confused about what this is doing. We're just starting one of the
> > effects? Should we start all of them instead? Is plumbing needed on the cc
> side
> > to recognize that these are WorkletAnimation effects which shouldn't start
> > running immediately? 
> 
> As far as I understand it, a lot of plumbing is still needed on the cc side
for
> this CL to be worth anything. But I still don't understand either the proposed
> or realized cc-side changes, so I think we need to sync offline about this.

A question was raised today as to why this doesn't currently do anything (even
just a single effect and normal CompositorPlayer) with the code so far. The
reason appears to be that for some reason element_animations_ is nullptr in
AnimationPlayer::AddAnimation.

[flackr, 2017-07-19 19:23:49]

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:143:
compositor_player_->Player());
On 2017/06/27 18:02:43, smcgruer wrote:
> On 2017/06/26 21:14:08, smcgruer wrote:
> > On 2017/06/19 20:35:33, flackr wrote:
> > > I'm a bit confused about what this is doing. We're just starting one of
the
> > > effects? Should we start all of them instead? Is plumbing needed on the cc
> > side
> > > to recognize that these are WorkletAnimation effects which shouldn't start
> > > running immediately? 
> > 
> > As far as I understand it, a lot of plumbing is still needed on the cc side
> for
> > this CL to be worth anything. But I still don't understand either the
proposed
> > or realized cc-side changes, so I think we need to sync offline about this.
> 
> A question was raised today as to why this doesn't currently do anything (even
> just a single effect and normal CompositorPlayer) with the code so far. The
> reason appears to be that for some reason element_animations_ is nullptr in
> AnimationPlayer::AddAnimation.

I think we should just assume success (or failure?) with a TODO to actually
create the element animations on the compositor, rather than do something that
doesn't seem to work.

https://codereview.chromium.org/2869183002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2869183002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:37: #include
"core/animation/WorkletAnimationController.h"
We talked about moving this to modules hashed off of the document, is this still
possible so we can keep this code isolated?

https://codereview.chromium.org/2869183002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:78: // do
this, or remove the layer of indirection.
I'm thinking we should not copy this pattern unless / until we find a reason to
need to do so.

[flackr, 2017-07-21 18:14:25]

https://codereview.chromium.org/2869183002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/2869183002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.h:37: #include
"core/animation/WorkletAnimationController.h"
On 2017/07/19 19:23:48, flackr wrote:
> We talked about moving this to modules hashed off of the document, is this
still
> possible so we can keep this code isolated?

Actually, on further thought, we will eventually want to do something with this
controller at commit time, so it would still need at least an interface to call.
Nevermind.

[smcgruer, 2017-07-21 19:45:19]

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

[commit-bot: I haz the power, 2017-07-21 19:45:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-21 21:04:45]

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

[commit-bot: I haz the power, 2017-07-21 21:04:46]

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

[smcgruer, 2017-07-24 14:30:06]

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

[commit-bot: I haz the power, 2017-07-24 14:30:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[smcgruer, 2017-07-24 14:48:58]

I still need to add some tests, but hopefully we're getting closer on this
patch?

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:122: //
TODO(smcgruer): Should this set NeedsCompositingUpdate?
On 2017/06/19 20:35:33, flackr wrote:
> The WorkletAnimationController should I think.

Ack added TODO on the WorkletAnimationController. I need to work out the best
way to get at the LocalFrameView (or ultimately PaintLayerCompositor).

https://codereview.chromium.org/2869183002/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:143:
compositor_player_->Player());
On 2017/07/19 19:23:48, flackr wrote:
> On 2017/06/27 18:02:43, smcgruer wrote:
> > On 2017/06/26 21:14:08, smcgruer wrote:
> > > On 2017/06/19 20:35:33, flackr wrote:
> > > > I'm a bit confused about what this is doing. We're just starting one of
> the
> > > > effects? Should we start all of them instead? Is plumbing needed on the
cc
> > > side
> > > > to recognize that these are WorkletAnimation effects which shouldn't
start
> > > > running immediately? 
> > > 
> > > As far as I understand it, a lot of plumbing is still needed on the cc
side
> > for
> > > this CL to be worth anything. But I still don't understand either the
> proposed
> > > or realized cc-side changes, so I think we need to sync offline about
this.
> > 
> > A question was raised today as to why this doesn't currently do anything
(even
> > just a single effect and normal CompositorPlayer) with the code so far. The
> > reason appears to be that for some reason element_animations_ is nullptr in
> > AnimationPlayer::AddAnimation.
> 
> I think we should just assume success (or failure?) with a TODO to actually
> create the element animations on the compositor, rather than do something that
> doesn't seem to work.

Done.

https://codereview.chromium.org/2869183002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:78: // do
this, or remove the layer of indirection.
On 2017/07/19 19:23:48, flackr wrote:
> I'm thinking we should not copy this pattern unless / until we find a reason
to
> need to do so.

Done.

[commit-bot: I haz the power, 2017-07-24 16:52:22]

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

[commit-bot: I haz the power, 2017-07-24 16:52:23]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[smcgruer, 2017-07-24 18:26:27]

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

[commit-bot: I haz the power, 2017-07-24 18:26:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-24 18:31:03]

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

[commit-bot: I haz the power, 2017-07-24 18:31:07]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[smcgruer, 2017-07-24 18:40:06]

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

[commit-bot: I haz the power, 2017-07-24 18:40:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-24 20:04:29]

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

[commit-bot: I haz the power, 2017-07-24 20:04:31]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[flackr, 2017-07-25 20:39:12]

Yes, I think this is getting close. We could probably use some WorkletAnimation
unit tests.

https://codereview.chromium.org/2869183002/diff/520018/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/520018/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:59: void
Dispose();
I think you need to declare USING_PRE_FINALIZER(Dispose) in order for Dispose to
get invoked.
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

[smcgruer, 2017-08-01 14:35:50]

What additional tests would you like to see? Creation is tested via the
LayoutTest, I could add some verification of e.g. being attached to the
CompositorTimeline but that is arguably too much of an internal detail. Actual
playback doesn't work yet so there isn't much to test there.

https://codereview.chromium.org/2869183002/diff/520018/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/520018/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:59: void
Dispose();
On 2017/07/25 20:39:12, flackr wrote:
> I think you need to declare USING_PRE_FINALIZER(Dispose) in order for Dispose
to
> get invoked.
>
https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So...

Done.

[majidvp, 2017-08-31 14:15:27]

lgtm


On 2017/08/01 14:35:50, smcgruer wrote:
> Actual playback doesn't work yet so there isn't much to test there.

FWIW, we have a WIP patch [1] that is introducing cc<->worklet playback for
these animations so this should not be far off.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/613860

https://codereview.chromium.org/2869183002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl
(right):

https://codereview.chromium.org/2869183002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:5: //
TODO(smcgruer): Spec link.
I think this is the right link: 
https://wicg.github.io/animation-worklet/#worklet-animation-desc

[majidvp, 2017-08-31 14:24:05]

nit: Please also update the description.

https://codereview.chromium.org/2869183002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl
(right):

https://codereview.chromium.org/2869183002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:12:
sequence<(DocumentTimeline or ScrollTimeline)> timelines,
FYI, the latest changes in spec has moved away from having
multiple timelines passed in constructor but given the flux
in this area and potential for change I am fine with not
changing this initial implementation yet and wait for
spec to land on final solution here.

[smcgruer, 2017-08-31 14:27:19]

smcgruer@chromium.org changed reviewers:
+ haraken@chromium.org

[smcgruer, 2017-08-31 14:27:21]

haraken@chromium.org: Please review changes in bindings/modules/v8/generated.gni
and modules/modules_idl_files.gni

[smcgruer, 2017-08-31 19:55:57]

Description was changed from

==========
Initial implementation of WorkletAnimation

WIP; change description before submit

BUG=720080
==========

to

==========
Initial implementation of main thread WorkletAnimation IDL

This change adds a WorkletAnimation object accessible in the main js scope
when the CompositorWorker (the old name for AnimationWorklet) blink runtime 
eature is enabled. WorkletAnimation objects are hooked up to the document's 
nimation lifecycle, owned by a WorkletAnimationController. The controller
ill attempt to start them on the compositor, but this currently fails due
to lack of support in the animation logic.

BUG=720080
==========

[smcgruer, 2017-08-31 19:59:56]

smcgruer@chromium.org changed reviewers:
+ suzyh@google.com

[smcgruer, 2017-08-31 19:59:58]

Description updated.

suzyh@ - please review changes to core/animation.

https://codereview.chromium.org/2869183002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl
(right):

https://codereview.chromium.org/2869183002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:5: //
TODO(smcgruer): Spec link.
On 2017/08/31 14:15:27, majidvp wrote:
> I think this is the right link: 
> https://wicg.github.io/animation-worklet/#worklet-animation-desc

Done.

https://codereview.chromium.org/2869183002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:12:
sequence<(DocumentTimeline or ScrollTimeline)> timelines,
On 2017/08/31 14:24:05, majidvp wrote:
> FYI, the latest changes in spec has moved away from having
> multiple timelines passed in constructor but given the flux
> in this area and potential for change I am fine with not
> changing this initial implementation yet and wait for
> spec to land on final solution here.
> 

Acknowledged.

[smcgruer, 2017-08-31 20:00:22]

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

[commit-bot: I haz the power, 2017-08-31 20:00:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-08-31 22:16:18]

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

[commit-bot: I haz the power, 2017-08-31 22:16:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Willigers, 2017-09-01 02:25:21]

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

[Eric Willigers, 2017-09-01 02:25:23]

https://codereview.chromium.org/2869183002/diff/560001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2869183002/diff/560001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt:7685:
interface Worklet
Do we understand why the virtual/stable interface listing changes on Mac?

The CompositorWorker flag is experimental.

https://codereview.chromium.org/2869183002/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl
(right):

https://codereview.chromium.org/2869183002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:7: enum
AnimationPlayState { "idle", "pending", "running" };
AnimationPlayState is already defined
in core/animation/Animation.idl.

I don't understand why any enums need to be defined twice.

The only cases where we currently define an enum twice are
EffectiveConnectionType and RequestCredentials.
In each of those cases (unlike AnimationPlayState),
the two definitions are identical.

[smcgruer, 2017-09-01 17:20:29]

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

[commit-bot: I haz the power, 2017-09-01 17:20:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-01 18:24:29]

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

[commit-bot: I haz the power, 2017-09-01 18:24:30]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[smcgruer, 2017-09-01 18:31:03]

https://codereview.chromium.org/2869183002/diff/560001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt
(right):

https://codereview.chromium.org/2869183002/diff/560001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt:7685:
interface Worklet
On 2017/09/01 02:25:23, Eric Willigers wrote:
> Do we understand why the virtual/stable interface listing changes on Mac?
> 
> The CompositorWorker flag is experimental.

Oversight; removed.

[smcgruer, 2017-09-01 18:37:18]

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

[commit-bot: I haz the power, 2017-09-01 18:37:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[smcgruer, 2017-09-01 19:03:01]

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

[commit-bot: I haz the power, 2017-09-01 19:03:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-01 19:43:20]

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

[commit-bot: I haz the power, 2017-09-01 19:43:21]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[smcgruer, 2017-09-06 14:57:27]

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

[commit-bot: I haz the power, 2017-09-06 14:57:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-06 15:00:12]

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

[commit-bot: I haz the power, 2017-09-06 15:00:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[smcgruer, 2017-09-06 15:32:11]

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

[commit-bot: I haz the power, 2017-09-06 15:32:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-06 15:45:12]

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

[commit-bot: I haz the power, 2017-09-06 15:45:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[smcgruer, 2017-09-06 17:09:02]

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

[commit-bot: I haz the power, 2017-09-06 17:09:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-06 18:59:50]

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

[commit-bot: I haz the power, 2017-09-06 18:59:51]

Dry run: This issue passed the CQ dry run.

[smcgruer, 2017-09-06 19:16:53]

smcgruer@chromium.org changed reviewers:
- alancutter@chromium.org, suzyh@google.com

[smcgruer, 2017-09-06 19:16:55]

https://codereview.chromium.org/2869183002/diff/560001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl
(right):

https://codereview.chromium.org/2869183002/diff/560001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:7: enum
AnimationPlayState { "idle", "pending", "running" };
On 2017/09/01 02:25:23, Eric Willigers wrote:
> AnimationPlayState is already defined
> in core/animation/Animation.idl.
> 
> I don't understand why any enums need to be defined twice.
> 
> The only cases where we currently define an enum twice are
> EffectiveConnectionType and RequestCredentials.
> In each of those cases (unlike AnimationPlayState),
> the two definitions are identical.

Good point. I was trying to create a restricted enum that only had the types
that I currently support in WorkletAnimation, but there is little point. Removed
this and used Animation::WorkletAnimation in the cpp.

[Eric Willigers, 2017-09-06 23:35:35]

lgtm

core/animation lgtm

[smcgruer, 2017-09-07 14:41:08]

Thanks Eric. Summary of remaining requests for review:

flackr: Please review modules/compositorworker changes

haraken: Please review bindings/modules/v8/generated.gni,
modules/modules_idl_files.gni, and dom/ changes

[flackr, 2017-09-07 15:59:13]

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationBase.h (right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationBase.h:19: // returned
true, it should not be called again.
It's a bit of a strange interface, it might be worth explaining why it would
return false and that you should keep calling it (every main frame?) until it
returns true?

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:10:
sequence<(DocumentTimeline or ScrollTimeline)> timelines,
These construction parameters have become obsolete - we can take a single effect
or sequence of effects and only a single timeline for now. We should at least
have a TODO but if possible it'd be nice to match the spec parameters.

[haraken, 2017-09-08 01:11:21]

haraken@chromium.org changed reviewers:
+ nhiroki@chromium.org

[haraken, 2017-09-08 01:11:24]

+nhiroki

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp:40: //
TODO(smcgruer): On failure, warn user. Perhaps fire cancel event?

Yeah, it doesn't look nice to silently ignore the failure. Maybe you could
address it in a follow-up.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:644:
worklet_animation_controller_(new WorkletAnimationController),

Don't you need to take some action when Document is ShutDown()?

For example, cancel all pending / running animations on the WorkletAnimation.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:107:
play_state_ = Animation::kPending;

Is there any restriction about the state transition of play_state_? For example,
is it valid to call play() twice in a row?

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:26:
USING_PRE_FINALIZER(WorkletAnimation, Dispose);

Would you help me understand why you need to call Dispose in a pre-finalizer?

In general it's not encouraged to do complex operations in a pre-finalizer. Is
there any other explicit point where you can call Dispose()?

[nhiroki, 2017-09-08 03:58:23]

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp:13:
WorkletAnimationController::~WorkletAnimationController() {}
= default; ?

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:84:
String animator_name,
const String& ?

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:21: class
MODULES_EXPORT WorkletAnimation : public WorkletAnimationBase,
Can you add class-level comments? Which thread does this lives on?

Worker/Worklet components are heavily threaded, so comments about thread model
and checks like DCHECK(IsMainThread()) at the beginning of each public function
would be really helpful for code readers.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:74: String
animator_name_;
const?

[nhiroki, 2017-09-08 04:00:17]

It looks like the CL description was chomped:

eature -> feature
nimation -> animation
ill -> will

[smcgruer, 2017-09-11 18:19:00]

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationBase.h (right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationBase.h:19: // returned
true, it should not be called again.
On 2017/09/07 15:59:13, flackr wrote:
> It's a bit of a strange interface, it might be worth explaining why it would
> return false and that you should keep calling it (every main frame?) until it
> returns true?

Changed the comment to better reflect reality.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp:13:
WorkletAnimationController::~WorkletAnimationController() {}
On 2017/09/08 03:58:22, nhiroki wrote:
> = default; ?

Done. Not familiar with our style here, but also marked the constructor since it
is similarly (afaik) default.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp:40: //
TODO(smcgruer): On failure, warn user. Perhaps fire cancel event?
On 2017/09/08 01:11:23, haraken wrote:
> 
> Yeah, it doesn't look nice to silently ignore the failure. Maybe you could
> address it in a follow-up.
> 

Ack, plan to do so.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:644:
worklet_animation_controller_(new WorkletAnimationController),
On 2017/09/08 01:11:23, haraken wrote:
> 
> Don't you need to take some action when Document is ShutDown()?
> 
> For example, cancel all pending / running animations on the WorkletAnimation.

At the moment, I'm unsure. This was based on PendingAnimations
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation...),
which does not have any such logic that I'm aware of. I believe the destructors
of the garbage-collected Animations/WorkletAnimation classes are simply meant to
take care of it.

We should understand our own lifecycle better though. I will work on this with
majidvp@ and see if we can get a better answer for you.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:84:
String animator_name,
On 2017/09/08 03:58:22, nhiroki wrote:
> const String& ?

Done.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.cpp:107:
play_state_ = Animation::kPending;
On 2017/09/08 01:11:23, haraken wrote:
> 
> Is there any restriction about the state transition of play_state_? For
example,
> is it valid to call play() twice in a row?

As of yet, no. It is valid to call play() twice in a row I believe (though it is
a no-op the second time), so I fixed the logic for that. However we will
definitely have some incorrect logic until we can get the back-end of
AnimationWorklet hooked up and an end-to-end test running.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:21: class
MODULES_EXPORT WorkletAnimation : public WorkletAnimationBase,
On 2017/09/08 03:58:22, nhiroki wrote:
> Can you add class-level comments? Which thread does this lives on?
> 
> Worker/Worklet components are heavily threaded, so comments about thread model
> and checks like DCHECK(IsMainThread()) at the beginning of each public
function
> would be really helpful for code readers.

Added some comments + DCHECKS, let me know if they answer your questions.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:26:
USING_PRE_FINALIZER(WorkletAnimation, Dispose);
On 2017/09/08 01:11:23, haraken wrote:
> 
> Would you help me understand why you need to call Dispose in a pre-finalizer?
> 
> In general it's not encouraged to do complex operations in a pre-finalizer. Is
> there any other explicit point where you can call Dispose()?

My understanding is that you cannot access any heap member during your
destructor (correct?). Based on that understanding, how else would one remove a
WorkletAnimation object from the CompositorTimeline when the last reference to
the object goes away? (Which, one day, will only happen when the user script
holds no references to the WorkletAnimation *and* when the animation subsystem
is done with it.).

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.h:74: String
animator_name_;
On 2017/09/08 03:58:22, nhiroki wrote:
> const?

Done.

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl
(right):

https://codereview.chromium.org/2869183002/diff/680001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/compositorworker/WorkletAnimation.idl:10:
sequence<(DocumentTimeline or ScrollTimeline)> timelines,
On 2017/09/07 15:59:13, flackr wrote:
> These construction parameters have become obsolete - we can take a single
effect
> or sequence of effects and only a single timeline for now. We should at least
> have a TODO but if possible it'd be nice to match the spec parameters.

I was previously specifically told not to update these yet as the spec here was
considered 'in flux'. Has that situation changed? Are we sure of the new
construction parameters?

Added a TODO for now at least.

[nhiroki, 2017-09-12 00:10:37]

LGTM from worker/worklet infra POV.

https://codereview.chromium.org/2869183002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp
(right):

https://codereview.chromium.org/2869183002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp:16:
WorkletAnimationBase& animation) {
DCHECK(IsMainThread()) here and in other functions?

[haraken, 2017-09-12 07:36:25]

LGTM

[smcgruer, 2017-09-12 13:31:24]

Description was changed from

==========
Initial implementation of main thread WorkletAnimation IDL

This change adds a WorkletAnimation object accessible in the main js scope
when the CompositorWorker (the old name for AnimationWorklet) blink runtime 
eature is enabled. WorkletAnimation objects are hooked up to the document's 
nimation lifecycle, owned by a WorkletAnimationController. The controller
ill attempt to start them on the compositor, but this currently fails due
to lack of support in the animation logic.

BUG=720080
==========

to

==========
Initial implementation of main thread WorkletAnimation IDL

This change adds a WorkletAnimation object accessible in the main js scope,
when the CompositorWorker (the old name for AnimationWorklet) Blink runtime 
feature is enabled. WorkletAnimation objects are hooked up to the document's 
animation lifecycle, owned by a WorkletAnimationController. The controller
will attempt to start them on the compositor, but this currently fails due
to lack of support in the animation logic.

BUG=720080
==========

[smcgruer, 2017-09-12 14:10:26]

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

[commit-bot: I haz the power, 2017-09-12 14:10:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[smcgruer, 2017-09-12 15:46:02]

https://codereview.chromium.org/2869183002/diff/700001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp
(right):

https://codereview.chromium.org/2869183002/diff/700001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/animation/WorkletAnimationController.cpp:16:
WorkletAnimationBase& animation) {
On 2017/09/12 00:10:36, nhiroki wrote:
> DCHECK(IsMainThread()) here and in other functions?

Done.

[commit-bot: I haz the power, 2017-09-12 15:49:23]

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

[commit-bot: I haz the power, 2017-09-12 15:49:25]

Dry run: This issue passed the CQ dry run.

[smcgruer, 2017-09-12 15:49:56]

The CQ bit was checked by smcgruer@chromium.org

[smcgruer, 2017-09-12 15:49:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
majidvp@chromium.org, ericwilligers@chromium.org, haraken@chromium.org,
nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/2869183002/#ps740001
(title: "Add more DCHECKs")

[commit-bot: I haz the power, 2017-09-12 15:50:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-09-12 15:56:54]

CQ is committing da patch.

Bot data: {"patchset_id": 740001, "attempt_start_ts": 1505231396733860,
"parent_rev": "f7f91782c6a16a5b7a9ba30b5a5c21a9671b757b", "commit_rev":
"c7af2f07e7458da6bab556dd3a2f2dcac3f73276"}

[commit-bot: I haz the power, 2017-09-12 15:57:07]

Description was changed from

==========
Initial implementation of main thread WorkletAnimation IDL

This change adds a WorkletAnimation object accessible in the main js scope,
when the CompositorWorker (the old name for AnimationWorklet) Blink runtime 
feature is enabled. WorkletAnimation objects are hooked up to the document's 
animation lifecycle, owned by a WorkletAnimationController. The controller
will attempt to start them on the compositor, but this currently fails due
to lack of support in the animation logic.

BUG=720080
==========

to

==========
Initial implementation of main thread WorkletAnimation IDL

This change adds a WorkletAnimation object accessible in the main js scope,
when the CompositorWorker (the old name for AnimationWorklet) Blink runtime 
feature is enabled. WorkletAnimation objects are hooked up to the document's 
animation lifecycle, owned by a WorkletAnimationController. The controller
will attempt to start them on the compositor, but this currently fails due
to lack of support in the animation logic.

BUG=720080

Review-Url: https://chromiumcodereview.appspot.com/2869183002
Cr-Commit-Position: refs/heads/master@{#501292}
Committed:
https://chromium.googlesource.com/chromium/src/+/c7af2f07e7458da6bab556dd3a2f...
==========

[commit-bot: I haz the power, 2017-09-12 15:57:09]

Committed patchset #39 (id:740001) as
https://chromium.googlesource.com/chromium/src/+/c7af2f07e7458da6bab556dd3a2f...