|Summary:||background-image transitions trigger between equivalent images|
|Product:||WebKit||Reporter:||Tim Horton <thorton>|
|Version:||528+ (Nightly build)|
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 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.