Bug 74229 - background-image transitions trigger between equivalent images
Summary: background-image transitions trigger between equivalent images
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-12-09 17:03 PST by Tim Horton
Modified: 2011-12-09 17:28 PST (History)
2 users (show)

See Also:


Attachments
patch (9.24 KB, patch)
2011-12-09 17:09 PST, Tim Horton
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2011-12-09 17:03:45 PST
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>
Comment 1 Tim Horton 2011-12-09 17:03:59 PST
Simon and I have a patch.
Comment 2 Tim Horton 2011-12-09 17:09:20 PST
Created attachment 118668 [details]
patch
Comment 3 Darin Adler 2011-12-09 17:16:03 PST
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.
Comment 4 Darin Adler 2011-12-09 17:16:25 PST
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.
Comment 5 Tim Horton 2011-12-09 17:26:59 PST
Landed with Darin's changes in 102498.
Comment 6 Chris Marrin 2011-12-09 17:28:02 PST
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.