Closed
Bug 1332211
Opened 8 years ago
Closed 8 years ago
Animation utility for compositor
Categories
(Core :: Graphics: WebRender, defect)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla54
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
Assignee | ||
Comment 1•8 years ago
|
||
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.
Assignee | ||
Updated•8 years ago
|
Whiteboard: [gfx-noted]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → howareyou322
Comment 2•8 years ago
|
||
(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.
Comment 3•8 years ago
|
||
I also prefer AnimationHelper or AnimationUtils.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
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 7•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 8•8 years ago
|
||
mozreview-review-reply |
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)
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
Assignee | ||
Comment 13•8 years ago
|
||
Submit another try to fix unified build problem.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=68ffb9e3624f737e3f6a8a16bc1240f751d5b9e7
Assignee | ||
Comment 14•8 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8833286 [details]
Bug 1332211 - fix build break because of unified build,
https://reviewboard.mozilla.org/r/109532/#review111070
Attachment #8833286 -
Flags: review?(mtseng) → review+
Comment 19•8 years ago
|
||
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
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4fe2c1b809e8
https://hg.mozilla.org/mozilla-central/rev/61dc0a725d0f
https://hg.mozilla.org/mozilla-central/rev/8c4afac0dcba
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•