Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2745823002:
[animation-worklet] Implement registerAnimator in worklet scope (Closed)
|
Created:
3 years, 9 months ago by majidvp Modified:
3 years, 7 months ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, shimazu+worker_chromium.org, kinuko+worker_chromium.org, darktears, blink-reviews, horo+watch_chromium.org, falken+watch_chromium.org, blink-worker-reviews_chromium.org, Eric Willigers, smcgruer Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement |registerAnimator| for AnimationWorklet
|registerAnimator| creates a simple definition model after validation.
It also instantiates at least one animator based on it for testing.
BUG=666019
Review-Url: https://codereview.chromium.org/2745823002
Cr-Commit-Position: refs/heads/master@{#474282}
Committed: https://chromium.googlesource.com/chromium/src/+/f09a33ec69c0ad5a9e59c49a9e39291d8fe795d9
Patch Set 1 #Patch Set 2 : Improve layout test #Patch Set 3 : remove empty array #
Total comments: 19
Patch Set 4 : feedback #
Total comments: 11
Patch Set 5 : rebase (after Blink rename) #Patch Set 6 : Address feedback and use new animationWorklet.addModule syntax #
Total comments: 13
Patch Set 7 : Address nhiroki feedback #Dependent Patchsets: Messages
Total messages: 44 (31 generated)
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
majidvp@chromium.org changed reviewers: + flackr@chromium.org
flackr@ would you mind taking a look at the approach in this patch. I am still thinking about ways to improve the tests to avoid the timeout. But other than that things should be correct.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/animation-worklet-animator-registration.html (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/animation-worklet-animator-registration.html:58: // to complete its tasks instead of a timeout. animationWorklet.import should return a promise which doesn't resolve until the AnimationWorklet has been loaded. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:74: v8::Local<v8::Function>::Cast(ctorValue.v8Value()); Shouldn't this be another exception state? What happens if you try to registerAnimator with a non function? https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:100: if (isUndefinedOrNull(animateValue)) { Does it make sense to check the animate at registration time when it could be changed at any point in time? We probably need to throw these errors when we try to run the animator later if they don't exist anyways. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:118: // Immediately instantiate and animator for the registered definition. s/and animator/an animator https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/Animator.cpp (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/Animator.cpp:31: void Animator::animate() const { Is animate ever called? Can we test it? https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.cpp (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.cpp:19: return new AnimatorDefinition(scriptState, constructor, animate); I think error checking the animate function on the prototype would make more sense to put here where the animate function is used. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.cpp:62: if (block.HasCaught()) Should we not be catching this error? Or logging it somewhere? https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:34: // Invoke the the Javascript 'animate' callback one the given instance. nit: s/one/on https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:45: // This object keeps the constructor function, and animate function alive. Isn't the animate function kept alive through the constructor's prototype? Or is this how we ensure that the animate function doesn't change?
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/animation-worklet-animator-registration.html (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/animation-worklet-animator-registration.html:58: // to complete its tasks instead of a timeout. On 2017/03/13 19:04:51, flackr wrote: > animationWorklet.import should return a promise which doesn't resolve until the > AnimationWorklet has been loaded. The above runeSequence chains those promises. The problem I think is that the test checks the console.log messages from threaded context which may not be ready yet despite promise getting resolved. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:74: v8::Local<v8::Function>::Cast(ctorValue.v8Value()); On 2017/03/13 19:04:51, flackr wrote: > Shouldn't this be another exception state? What happens if you try to > registerAnimator with a non function? there is a test for this. The IDL defines is as a function which is checked by binding generator code. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:100: if (isUndefinedOrNull(animateValue)) { On 2017/03/13 19:04:51, flackr wrote: > Does it make sense to check the animate at registration time when it could be > changed at any point in time? We probably need to throw these errors when we try > to run the animator later if they don't exist anyways. Hmmm, you suggestion makes sense. Though this is the same pattern as used in Paint Worklet. @ikilpatrick, wdyt? should we check the |animate| function when we first instantiate it. Also should we even keep a reference to it rather than looking it up on JS object instance which means the function can by changed. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:118: // Immediately instantiate and animator for the registered definition. On 2017/03/13 19:04:51, flackr wrote: > s/and animator/an animator Done. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/Animator.cpp (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/Animator.cpp:31: void Animator::animate() const { On 2017/03/13 19:04:51, flackr wrote: > Is animate ever called? Can we test it? Removed to be added in a later patch. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:34: // Invoke the the Javascript 'animate' callback one the given instance. On 2017/03/13 19:04:52, flackr wrote: > nit: s/one/on removed. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:45: // This object keeps the constructor function, and animate function alive. On 2017/03/13 19:04:52, flackr wrote: > Isn't the animate function kept alive through the constructor's prototype? Or is > this how we ensure that the animate function doesn't change? Mainly so it does not change. We can choose to look it up per each invocation though. We allows some interesting dynamic behavior. But I want to check to see Houdini's thinking here.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ikilpatrick@chromium.org changed reviewers: + ikilpatrick@chromium.org, nhiroki@chromium.org
https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:100: if (isUndefinedOrNull(animateValue)) { On 2017/03/13 22:07:53, majidvp wrote: > On 2017/03/13 19:04:51, flackr wrote: > > Does it make sense to check the animate at registration time when it could be > > changed at any point in time? We probably need to throw these errors when we > try > > to run the animator later if they don't exist anyways. > > Hmmm, you suggestion makes sense. Though this is the same pattern as used in > Paint Worklet. @ikilpatrick, wdyt? should we check the |animate| function > when we first instantiate it. Also should we even keep a reference to it > rather than looking it up on JS object instance which means the function > can by changed. We should do this here - firstly consistency with the rest of the platform, i.e. custom-elements, paint, etc. It prevents code like: class Animator { get animate() { return Math.random() > 0.5 ? function() { } : 55; } } If we allow this, you have to additionally answer a bunch of specification questions like, if an animate function was invalid one frame, do we keep on trying to call them as they may be valid the next frame? etc. Better to fail early here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a couple nits. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:74: v8::Local<v8::Function>::Cast(ctorValue.v8Value()); On 2017/03/13 22:07:53, majidvp wrote: > On 2017/03/13 19:04:51, flackr wrote: > > Shouldn't this be another exception state? What happens if you try to > > registerAnimator with a non function? > > there is a test for this. The IDL defines is as a function which is checked > by binding generator code. Acknowledged. Thanks. https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h (right): https://codereview.chromium.org/2745823002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:45: // This object keeps the constructor function, and animate function alive. On 2017/03/13 22:07:53, majidvp wrote: > On 2017/03/13 19:04:52, flackr wrote: > > Isn't the animate function kept alive through the constructor's prototype? Or > is > > this how we ensure that the animate function doesn't change? > > Mainly so it does not change. We can choose to look it up per each > invocation though. We allows some interesting dynamic behavior. > But I want to check to see Houdini's thinking here. Let's call out in the comment that we keep the original animate function to prevent runtime changes to it. https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:110: } I still think the assertions about the animate function should be checked in AnimatorDefinition::create where it will later use the animate function. Note this isn't a timing change, it's still checked at registration time, just keeping code locality - checking errors on the animate function in the place where we will later invoke the animate function.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/Animator.h (right): https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/Animator.h:28: RefPtr<ScriptState> m_scriptState; Is this used? https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/Animator.h:32: // AnimationWorkletGlobalScope. Mention that it is broken at AnimationWorkletGlobalScope::dispose(). https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h (right): https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:21: // 'animate' callback one a given instance. on a given https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:32: Animator* createInstance(); For consistency with https://codereview.chromium.org/2727733002/, I'd prefer moving createInstance to AnimationWorkletGlobalScope. Then we can drop m_scriptState from this class. https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:43: // AnimationWorkletGlobalScope. Mention that it is broken at AnimationWorkletGlobalScope::dispose().
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
haraken@: PTAL. (I was on vacation which is why this took a while :) ) https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/Animator.h (right): https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/Animator.h:28: RefPtr<ScriptState> m_scriptState; On 2017/03/14 20:33:02, haraken wrote: > > Is this used? not currently. Removed! https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/Animator.h:32: // AnimationWorkletGlobalScope. On 2017/03/14 20:33:02, haraken wrote: > > Mention that it is broken at AnimationWorkletGlobalScope::dispose(). Done. https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h (right): https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:21: // 'animate' callback one a given instance. On 2017/03/14 20:33:02, haraken wrote: > > on a given Done. https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:32: Animator* createInstance(); On 2017/03/14 20:33:02, haraken wrote: > > For consistency with https://codereview.chromium.org/2727733002/, I'd prefer > moving createInstance to AnimationWorkletGlobalScope. > > Then we can drop m_scriptState from this class. Done. https://codereview.chromium.org/2745823002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:43: // AnimationWorkletGlobalScope. On 2017/03/14 20:33:02, haraken wrote: > > Mention that it is broken at AnimationWorkletGlobalScope::dispose(). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/Animator.h (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/Animator.h:32: ScopedPersistent<v8::Object> m_instance; Can we use the wrapper tracing? You can trace m_instance from AnimationWorkletGlobalScope. That way we don't need to worry about the reference cycle. (We should make the same change to AudioWorkletGlobalScope.) https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:41: ScopedPersistent<v8::Function> m_animate; Ditto. We should consider using the wrapper tracing. https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:41: ScopedPersistent<v8::Function> m_animate; m_animate is not used...?
LGTM from worklet POV https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:59: ExceptionState& exceptionState) { Can you add a thread check? DCHECK(IsContextThread()); https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:126: Animator* AnimationWorkletGlobalScope::CreateInstance(const String& name) { Ditto(thread check) https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/Animator.cpp (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/Animator.cpp:15: return new Animator(isolate, definition, instance); How about exposing the ctor instead of this factory function? https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.cpp (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.cpp:19: return new AnimatorDefinition(isolate, constructor, animate); Ditto: How about removing this factory function?
The CQ bit was checked by majidvp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:59: ExceptionState& exceptionState) { On 2017/05/22 00:20:20, nhiroki (slow) wrote: > Can you add a thread check? > > DCHECK(IsContextThread()); Done. https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/AnimationWorkletGlobalScope.cpp:126: Animator* AnimationWorkletGlobalScope::CreateInstance(const String& name) { On 2017/05/22 00:20:20, nhiroki (slow) wrote: > Ditto(thread check) Done. https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/Animator.cpp (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/Animator.cpp:15: return new Animator(isolate, definition, instance); On 2017/05/22 00:20:20, nhiroki (slow) wrote: > How about exposing the ctor instead of this factory function? Done. I suppose unlike ref-counted object we don't need to use factory method + private constructor. https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/Animator.h (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/Animator.h:32: ScopedPersistent<v8::Object> m_instance; On 2017/05/20 00:35:29, haraken wrote: > > Can we use the wrapper tracing? You can trace m_instance from > AnimationWorkletGlobalScope. That way we don't need to worry about the reference > cycle. > > (We should make the same change to AudioWorkletGlobalScope.) > > That makes sense. I didn't know we have wrapper tracing already implemented \o/ I will switch all worklets to wrapper tracing in a follow up patch. Here is my WIP: https://codereview.chromium.org/2903703003 Feedback is welcome as I am starting to familiarize myself with the patterns. https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.cpp (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.cpp:19: return new AnimatorDefinition(isolate, constructor, animate); On 2017/05/22 00:20:20, nhiroki (slow) wrote: > Ditto: How about removing this factory function? Done. https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h (right): https://codereview.chromium.org/2745823002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/compositorworker/AnimatorDefinition.h:41: ScopedPersistent<v8::Function> m_animate; On 2017/05/20 00:35:29, haraken wrote: > > m_animate is not used...? The patch is adding the validation code for |m_animate| as part of the registration process for an animator. The function is used in a follow up patch. Here is WIP for that: https://codereview.chromium.org/2894233002/
Description was changed from ========== Implement registerAnimator for AnimationWorklet registerAnimator creates a simple definition model and then instantiates at least one animator based on it. BUG=666019 ========== to ========== Implement registerAnimator for AnimationWorklet registerAnimator creates a simple definition model and then instantiates at least one animator based on it. BUG=666019 ==========
Description was changed from ========== Implement registerAnimator for AnimationWorklet registerAnimator creates a simple definition model and then instantiates at least one animator based on it. BUG=666019 ========== to ========== Implement |registerAnimator| for AnimationWorklet |registerAnimator| creates a simple definition model after validation. It also instantiates at least one animator based on it for testing. BUG=666019 ==========
The CQ bit was checked by majidvp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/2745823002/#ps120001 (title: "Address nhiroki feedback")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495635988471880,
"parent_rev": "e82f2876b31f32df8a61edc4e5e9935efc54e46b", "commit_rev":
"f09a33ec69c0ad5a9e59c49a9e39291d8fe795d9"}
Message was sent while issue was closed.
Description was changed from ========== Implement |registerAnimator| for AnimationWorklet |registerAnimator| creates a simple definition model after validation. It also instantiates at least one animator based on it for testing. BUG=666019 ========== to ========== Implement |registerAnimator| for AnimationWorklet |registerAnimator| creates a simple definition model after validation. It also instantiates at least one animator based on it for testing. BUG=666019 Review-Url: https://codereview.chromium.org/2745823002 Cr-Commit-Position: refs/heads/master@{#474282} Committed: https://chromium.googlesource.com/chromium/src/+/f09a33ec69c0ad5a9e59c49a9e39... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f09a33ec69c0ad5a9e59c49a9e39...
Message was sent while issue was closed.
LGTM |
