Bug 144196 – Modernize animations code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Modernize animations code

[Simon Fraser (smfr)]

Created attachment 251648 [details]
Patch

[WebKit Commit Bot]

Attachment 251648 [details] did not pass style-queue:


ERROR: Source/WebCore/page/animation/CompositeAnimation.cpp:89:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 251648 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251648&action=review

> Source/WebCore/page/animation/AnimationBase.cpp:73
> +AnimationBase::AnimationBase(const Animation& animation, RenderElement* renderer, CompositeAnimation* compositeAnimation)

Can either of those be references instead of pointers?

> Source/WebCore/page/animation/AnimationBase.cpp:76
> +    , m_animation(const_cast<Animation&>(animation))

If we are keeping a reference, maybe we should be taking a reference, not a const reference.

> Source/WebCore/page/animation/AnimationBase.h:53
> +    WTF_MAKE_FAST_ALLOCATED;

What changed here? Are we now allocating these on the heap for the first time? Or were we using the wrong alloc?

> Source/WebCore/page/animation/AnimationBase.h:150
> +    void setAnimation(const Animation& animation) { m_animation = const_cast<Animation&>(animation); }

I don’t understand why this takes a const Animation& and not an Animation&.

> Source/WebCore/page/animation/CompositeAnimation.cpp:66
> +        for (auto& it : m_transitions) {
> +            ImplicitAnimation* transition = it.value.get();

This should be:

    for (auto& transition : m_transitions.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:67
> +            animationController().animationWillBeRemoved(transition);

This will need to do transition.get(), but otherwise should be fine. Of course, animationWillBeRemoved should take a reference, and then it would be *transition.

> Source/WebCore/page/animation/CompositeAnimation.cpp:74
> +        for (auto& it : m_keyframeAnimations) {
> +            KeyframeAnimation* anim = it.value.get();

for (auto* animation : m_keyframeAnimations.values() {

> Source/WebCore/page/animation/CompositeAnimation.cpp:89
> +    for (auto& it : m_transitions)

for (auto& transition : m_transitions.values())

> Source/WebCore/page/animation/CompositeAnimation.cpp:187
> +    for (auto& it : m_transitions) {
> +        ImplicitAnimation* anim = it.value.get();

for (auto& animation : m_transitions.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:211
> +        for (auto& it : m_keyframeAnimations) {

for (auto& animation : m_keyframeAnimations.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:217
> +        for (auto& it : m_keyframeAnimations)

for (auto& animation : m_keyframeAnimations.values())

> Source/WebCore/page/animation/CompositeAnimation.cpp:290
> +    for (auto& it : m_keyframeAnimations) {
> +        KeyframeAnimation* keyframeAnim = it.value.get();

for (auto& animation : m_keyframeAnimations.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:317
> +            for (auto& it : m_transitions) {

for (auto& animation : m_transitions.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:326
> +    for (auto& it : m_keyframeAnimationOrderMap) {

The string here should be named something other than "it". It’s not an iterator. Its type is AtomicStringImpl*, so I think maybe it’s animationName?

> Source/WebCore/page/animation/CompositeAnimation.cpp:340
> +    for (auto& it : m_transitions) {

for (auto& animation : m_transitions.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:364
> +        for (auto& it : m_transitions) {
> +            ImplicitAnimation* transition = it.value.get();

for (auto& animation : m_transitions.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:396
> +        for (auto& it : m_keyframeAnimations) {
> +            RefPtr<KeyframeAnimation> anim = it.value;

for (auto& animation : m_keyframeAnimations.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:451
> +        for (auto& it : m_keyframeAnimations) {
> +            if (KeyframeAnimation* anim = it.value.get())

for (auto& animation : m_keyframeAnimations.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:474
> +        for (auto& it : m_keyframeAnimations) {
> +            KeyframeAnimation* anim = it.value.get();

for (auto& animation : m_keyframeAnimations.values()) {for (auto& animation : m_keyframeAnimations.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:482
> +        for (auto& it : m_transitions) {
> +            ImplicitAnimation* anim = it.value.get();

for (auto& animation : m_transitions.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:493
> +        for (auto& it : m_transitions) {
> +            ImplicitAnimation* anim = it.value.get();

for (auto& animation : m_transitions.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:516
> +        for (auto& it : m_keyframeAnimations) {
> +            KeyframeAnimation* anim = it.value.get();

for (auto& animation : m_transitions.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:524
> +        for (auto& it : m_transitions) {
> +            ImplicitAnimation* anim = it.value.get();

for (auto& animation : m_transitions.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:585
> +        for (auto& it : m_keyframeAnimations) {
> +            KeyframeAnimation* anim = it.value.get();

for (auto& animation : m_keyframeAnimations.values()) {

> Source/WebCore/page/animation/CompositeAnimation.cpp:593
> +        for (auto& it : m_transitions) {
> +            ImplicitAnimation* anim = it.value.get();

for (auto& animation : m_transitions.values()) {

> Source/WebCore/page/animation/KeyframeAnimation.cpp:241
> +        return downcast<RenderBoxModelObject>(*m_object).startAnimation(timeOffset, &m_animation.get(), m_keyframes);

Not sure about the types here, but I think this can use m_animation.ptr() instead of &m_animation.get().

> Source/WebCore/rendering/RenderLayer.cpp:3753
> +    for (auto& it : overlapTestRequests)

for (auto& widget : overlapTestRequests.keys())

> Source/WebCore/rendering/RenderLayer.cpp:3823
> +    for (auto& it : overlapTestRequests) {

Probably should name this "request" rather than "it".

[Simon Fraser (smfr)]

Created attachment 251699 [details]
For EWS

[Simon Fraser (smfr)]

Comment on attachment 251699 [details]
For EWS

Bring Darin review forward.

[WebKit Commit Bot]

Attachment 251699 [details] did not pass style-queue:


ERROR: Source/WebCore/page/animation/CompositeAnimation.cpp:87:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 251699 [details]
For EWS

Clearing flags on attachment: 251699

Committed r183364: <http://trac.webkit.org/changeset/183364>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.