Closed Bug 1332211 Opened 8 years ago Closed 8 years ago

Animation utility for compositor

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: pchang, Assigned: pchang)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files)

For Quantum render project, we still need the OMTA but there is no 'layer' concept in parent(WebRenderBridgeParent). In order to share the code between gecko and quantum render, I would like to put animation related functions (without using layer) into animation utility for composition. For example, I will move the following functions to gfx/layer/composition/AnimationComposition. [1] Layer::SetAnimations [2] SampleAnimationForEachNode [3] SampleValue [1]http://searchfox.org/mozilla-central/source/gfx/layers/Layers.cpp#454 [2]http://searchfox.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#696 [3]http://searchfox.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#585
Brian and Hiro, please let me know your thought about this and any naming suggestion for this utility, like gfx/layer/composite/AnimationComposition or gfx/layer/composite/AnimationHelper.
Whiteboard: [gfx-noted]
Blocks: 1328231
Assignee: nobody → howareyou322
(In reply to Peter Chang[:pchang] from comment #1) > Brian and Hiro, please let me know your thought about this and any naming > suggestion for this utility, like > gfx/layer/composite/AnimationComposition or > gfx/layer/composite/AnimationHelper. In layout XXUtils is popular name for such purpose but XXHelper seems to be popular in gfx (There are Helper and Helpers though). So, +1 for AnimationHelper. AnimationComposition looks like it contains a real class.
I also prefer AnimationHelper or AnimationUtils.
Comment on attachment 8830631 [details] Bug 1332211 - Move Layer::SetAnimation into AnimationHelper, https://reviewboard.mozilla.org/r/107374/#review108502 Looks good to me, but MozReview does not show moving lines at all, so I maybe miss something... Anyway, thanks! ::: gfx/layers/AnimationHelper.h:7 (Diff revision 1) > +#ifndef mozilla_animationhelper_h > +#define mozilla_animationhelper_h This should be mozilla_layers_AnimationHelper_h. ::: gfx/layers/AnimationHelper.h:12 (Diff revision 1) > +#ifndef mozilla_animationhelper_h > +#define mozilla_animationhelper_h > + > +#include "mozilla/ComputedTimingFunction.h" // for ComputedTimingFunction > +#include "mozilla/TimeStamp.h" // for TimeStamp > +#include "mozilla/StyleAnimationValue.h" // for StyleAnimationValue, etc Use forward declaration instead of including this header. ::: gfx/layers/AnimationHelper.h:15 (Diff revision 1) > +#include "mozilla/ComputedTimingFunction.h" // for ComputedTimingFunction > +#include "mozilla/TimeStamp.h" // for TimeStamp > +#include "mozilla/StyleAnimationValue.h" // for StyleAnimationValue, etc > + > +namespace mozilla { > +class ComputedTimingFunction; I wonder if this forward declaration is necessary. ::: gfx/layers/AnimationHelper.h:20 (Diff revision 1) > +class AnimationData; > +class TimingFunction; These two classes are necessary? ::: gfx/layers/AnimationHelper.h:33 (Diff revision 1) > + InfallibleTArray<Maybe<mozilla::ComputedTimingFunction>> mFunctions; > +}; > + > +class AnimationHelper > +{ > + Nit: drop a blank line here. ::: gfx/layers/AnimationHelper.h:38 (Diff revision 1) > + > +public: > + static void > + SetAnimations(AnimationArray& aAnimations, > + InfallibleTArray<AnimData>& aAnimData, > + StyleAnimationValue& aValue); Please rename |aValue| to |aBaseAnimationStyle|. ::: gfx/layers/AnimationHelper.cpp:8 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "AnimationHelper.h" > +#include "mozilla/dom/Animation.h" // for ComputedTimingFunction Nit: Include ComputedTimingFunction.h instead of dom/Animation.h. ::: gfx/layers/AnimationHelper.cpp:193 (Diff revision 1) > +} > + > +void > +AnimationHelper::SetAnimations(AnimationArray& aAnimations, > + InfallibleTArray<AnimData>& aAnimData, > + StyleAnimationValue& aValue) Please rename |aValue| to |aBaseAnimationStyle|. ::: gfx/layers/Layers.h:105 (Diff revision 1) > class SpecificLayerAttributes; > class Compositor; > class FrameUniformityData; > class PersistentBufferProvider; > class GlyphArray; > +class WebRenderLayerManager; I wonder where this class came from. ::: gfx/layers/composite/AsyncCompositionManager.cpp:24 (Diff revision 1) > #include "mozilla/dom/KeyframeEffectBinding.h" // for dom::IterationComposite > #include "mozilla/gfx/BaseRect.h" // for BaseRect > #include "mozilla/gfx/Point.h" // for RoundedToInt, PointTyped > #include "mozilla/gfx/Rect.h" // for RoundedToInt, RectTyped > #include "mozilla/gfx/ScaleFactor.h" // for ScaleFactor > +#include "mozilla/layers/AnimationHelper.h" Nit: this change should be in the next patch?
Attachment #8830631 - Flags: review?(hikezoe) → review+
Comment on attachment 8830632 [details] Bug 1332211 - refactor animation code in AsyncCompositionManager, https://reviewboard.mozilla.org/r/107376/#review108508 LGTM. Thanks for doing this!
Attachment #8830632 - Flags: review?(hikezoe) → review+
Comment on attachment 8830631 [details] Bug 1332211 - Move Layer::SetAnimation into AnimationHelper, https://reviewboard.mozilla.org/r/107374/#review108502 > Nit: this change should be in the next patch? I still keep the following function in AsyncCompositionManager.cpp. So this header is needed because of the declaration AnimationData static void ApplyAnimatedValue(Layer* aLayer, nsCSSPropertyID aProperty, const AnimationData& aAnimationData, const StyleAnimationValue& aValue)
Comment on attachment 8830631 [details] Bug 1332211 - Move Layer::SetAnimation into AnimationHelper, https://reviewboard.mozilla.org/r/107374/#review110204 ::: gfx/layers/Layers.h:105 (Diff revision 1) > class SpecificLayerAttributes; > class Compositor; > class FrameUniformityData; > class PersistentBufferProvider; > class GlyphArray; > +class WebRenderLayerManager; I should not include this.
(In reply to Peter Chang[:pchang] from comment #13) > Submit another try to fix unified build problem. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=68ffb9e3624f737e3f6a8a16bc1240f751d5b9e7 Build got passed.
Attachment #8833286 - Flags: review?(mtseng) → review+
Pushed by pchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4fe2c1b809e8 Move Layer::SetAnimation into AnimationHelper, r=hiro https://hg.mozilla.org/integration/autoland/rev/61dc0a725d0f refactor animation code in AsyncCompositionManager, r=hiro https://hg.mozilla.org/integration/autoland/rev/8c4afac0dcba fix build break because of unified build, r=mtseng
Blocks: 1333355
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: