Bug 74229 – background-image transitions trigger between equivalent images

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Tim Horton]

When testing equality for implicit animations, we're not testing equivalence of StyleImages, we're testing pointer equality. This isn't useful and ends up causing a ton of extra implicit animations which needn't happen.

This is especially visible on YouTube, which has -webkit-transition: all set, and causes excessive loads there.

<rdar://problem/10558627>

[Tim Horton]

Simon and I have a patch.

[Tim Horton]

Created attachment 118668 [details]
patch

[Darin Adler]

Comment on attachment 118668 [details]
patch

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

> Source/WebCore/page/animation/AnimationBase.cpp:385
> +       if ((!a && !b) || a == b)

No need for the first half of this expression. Just a == b will do.

> Source/WebCore/page/animation/AnimationBase.cpp:698
> +       if ((!a && !b) || a == b)

Ditto.

[Darin Adler]

Comment on attachment 118668 [details]
patch

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

> Source/WebCore/page/animation/AnimationBase.cpp:377
> +    : RefCountedPropertyWrapper<StyleImage>(prop, getter, setter)

In WebKit style this is indented four additional spaces.

[Tim Horton]

Landed with Darin's changes in 102498.

[Chris Marrin]

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

> Source/WebCore/page/animation/AnimationBase.cpp:384
> +       // If either is null, return false. If both are null, return true.

This comment doesn't seem to be adding value. You're just stating in text what the code is actually doing. Either get rid of the comment or say why you're doing all these tests. It may be that the only comment needed is why a null style is never equal to a non-null. Isn't it possible that the non-null style would have the same effect as a null style?

> Source/WebCore/page/animation/AnimationBase.cpp:392
> +        return StyleImage::imagesEquivalent(imageA, imageB);

A better name would be good. What does "equivalent" mean? Are you doing a pixel by pixel test? Assuming you're actually just saying that they both point to the same underlying bitmap, saying imagesSame might be simpler (I think we use "same" in other places).

> Source/WebCore/page/animation/AnimationBase.cpp:697
> +       // If either is null, return false. If both are null, return true.

ibid.