Bug 74229

Summary: background-image transitions trigger between equivalent images
Product: WebKit Reporter: Tim Horton <thorton>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarrin, simon.fraser
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch darin: review+

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.