[webkit-reviews] review requested: [Bug 83283] [chromium] Should synchronize running animations if the impl thread controller has just been recreated : [Attachment 135977] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 83283: [chromium] Should synchronize running animations if the impl thread
controller has just been recreated
https://bugs.webkit.org/show_bug.cgi?id=83283

Attachment 135977: Patch
https://bugs.webkit.org/attachment.cgi?id=135977&action=review

------- Additional Comments from vollick at chromium.org
Ensures that we do push running animation to the impl thread when necessary.(In
reply to comment #2)
> (From update of attachment 135833 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=135833&action=review
>
> > Source/WebCore/platform/graphics/chromium/cc/CCActiveAnimation.cpp:-135
> > -void CCActiveAnimation::synchronizeProperties(CCActiveAnimation* other)
>
> not used any longer?
Nope, it's dead code.
>
> >
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:-25
0
> > -	     if (!m_activeAnimations[i]->needsSynchronizedStartTime())
>
> Hoist m_activeAnimations[i] to a temp var.
Done.
>
> Possibly, move the body of this loop to CCActiveAnimation where pushProps
used to was? Not sure... but this all feels like animation "push" logic.
Hmm. I can't see a nice way of doing that. The function would conditionally
create the new animation and return it and we would add it to the impl thread
controller in this loop?
>
> >
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:252
> > +	     if (controllerImpl->m_initialized &&
!m_activeAnimations[i]->needsSynchronizedStartTime())
>
> Can you come up with a better name. ActivatedOnImplThread?
I renamed it m_hasSynchronizedWithMainThread since it's meant to indicate that
we've done at least one sync.
>
> Having a hard time understanding why this is conditional on initialize.
Document why.
Updated the comment.
>
> >
Source/WebCore/platform/graphics/chromium/cc/CCLayerAnimationController.cpp:258
> > +	     // If controllerImpl is uninitialized and the animation does not
need a synchronized start
>
> Again, unitialized isn't conveying purpose here. This whole comment block
really breaks my head. Your comment text says if (!uninitialized) but then your
branch is actually the reverse, which makes it hard to associate the
explanation with the code.
Renamed the variable and updated the comment.