Summary: | Introduce an isCSSAnimated flag on RenderElement for performance | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | Animations | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Enhancement | CC: | barraclough, buildbot, commit-queue, dbates, dino, dstockwell, esprehn+autocc, glenn, kling, kondapallykalyan, rniwa, simon.fraser | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 137786 | ||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2014-10-09 16:44:36 PDT
Created attachment 239581 [details]
Patch
Comment on attachment 239581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239581&action=review > Source/WebCore/page/animation/AnimationControllerPrivate.h:89 > + const CompositeAnimation* animationForRenderer(const RenderElement&) const; I don't think CompositeAnimation should leak out of AnimationController. > Source/WebCore/rendering/RenderLayerBacking.cpp:35 > +#include "CompositeAnimation.h" I don't like this. CompositeAnimation is internal to AnimationController.. (In reply to comment #3) > (From update of attachment 239581 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=239581&action=review > > > Source/WebCore/page/animation/AnimationControllerPrivate.h:89 > > + const CompositeAnimation* animationForRenderer(const RenderElement&) const; > > I don't think CompositeAnimation should leak out of AnimationController. > > > Source/WebCore/rendering/RenderLayerBacking.cpp:35 > > +#include "CompositeAnimation.h" > > I don't like this. CompositeAnimation is internal to AnimationController.. I see, I did not know that, thanks. I will try to deal with this some other way then. Will submit another proposal. Created attachment 239690 [details]
Patch
Comment on attachment 239690 [details] Patch Attachment 239690 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6128418127085568 New failing tests: compositing/layer-creation/animation-overlap-with-children.html compositing/layer-creation/overlap-animation-container.html compositing/layer-creation/overlap-animation-clipping.html compositing/layer-creation/overlap-animation.html Created attachment 239691 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 239690 [details] Patch Attachment 239690 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6206560762068992 New failing tests: compositing/layer-creation/animation-overlap-with-children.html compositing/layer-creation/overlap-animation-container.html compositing/layer-creation/overlap-animation-clipping.html compositing/layer-creation/overlap-animation.html Created attachment 239694 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 239695 [details]
Patch
Simon, do you have any thought on my latest iteration? Comment on attachment 239695 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239695&action=review > Source/WebCore/page/animation/AnimationBase.h:42 > +enum class AnimationQueryFilter { Any, AcceleratedOnly }; We often use the term "FooOrNot" for thee kinds of enums. I find that a bit more readable than "queryfilter". > Source/WebCore/page/animation/AnimationBase.h:44 > +enum AnimatedCSSProperties { These are really properties that can have accelerated animations. So I'd call them AcceleratedAnimationPropertyFlags or something. > Source/WebCore/page/animation/AnimationController.cpp:254 > + AnimatedCSSPropertyMask animatedProperties = 0; > + if (animation.isAnimatingProperty(CSSPropertyOpacity, filter, runningState)) > + animatedProperties |= AnimatingOpacityProperty; > + if (animation.isAnimatingProperty(CSSPropertyWebkitFilter, filter, runningState)) > + animatedProperties |= AnimatingWebKitFilterProperty; > + if (animation.isAnimatingProperty(CSSPropertyWebkitTransform, filter, runningState)) > + animatedProperties |= AnimatingWebKitTransformProperty; > + return animatedProperties; This is a bit deceptive. animatedProperties makes it sound like a set of all properties that are animating, but it's really animating properties that happen to be acceleratable. > Source/WebCore/rendering/RenderElement.h:164 > + bool hasCompositeAnimation() const { return m_hasCompositeAnimation; } > + void setHasCompositeAnimation(bool b) { m_hasCompositeAnimation = b; } I think this should be "isCSSAnimating" or something. The fact that it's a composite animation (crappy name) is just an imp. detail of AnimationController. Created attachment 239778 [details]
Patch
This latest patch includes only the flag on RenderElement. Getting the AnimationController API right is difficult, especially while maintaining the performance. Introducing the flag on RenderElement is sufficient to greatly improve the performance when loading ebay.com. Simon, is it any better now? :) Comment on attachment 239778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=239778&action=review > Source/WebCore/page/animation/AnimationController.cpp:246 > + const CompositeAnimation& animation = *m_compositeAnimations.get(renderer); I sure hope we always get non-null here. Comment on attachment 239778 [details] Patch Clearing flags on attachment: 239778 Committed r174703: <http://trac.webkit.org/changeset/174703> All reviewed patches have been landed. Closing bug. |