Closed
Bug 1311257
Opened 8 years ago
Closed 8 years ago
stylo: support missing keyframe whose computed offset is 0 or 1 for servo backend
Categories
(Core :: DOM: Animation, defect, P3)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
(Blocks 1 open bug)
Details
Attachments
(11 files, 4 obsolete files)
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
heycam
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
We implement to support missing keyframes for Gecko in bug 1305325.
To support missing keyframes, we need base values for each element.
I guess cascade_style in KeyframesAnimationState is the base value.
http://hg.mozilla.org/incubator/stylo/file/9f7b709e71b7/servo/components/style/animation.rs#l71
Comment 1•8 years ago
|
||
Moving this to block bug 1317211 instead since this is not required to bootstrap CSS animations/transitions.
Updated•8 years ago
|
Priority: -- → P3
Summary: stylo: suport missing keyframe whose computed offset is 0 or 1 for servo backend → stylo: support missing keyframe whose computed offset is 0 or 1 for servo backend
Comment 2•8 years ago
|
||
After applying patches of bug 1317209, we start to get this test failed:
Assertion failure: !aStyleContext->StyleSource().IsServoComputedValues() (Bug 1311257: Servo backend does not support the base value yet), at /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:955
TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1323114-2.html | application terminated with exit code 11
REFTEST PROCESS-CRASH | file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/crashtests/1323114-2.html | application crashed [@ mozilla::EffectCompositor::GetBaseStyle]
And I think this bug can fix this assertion.
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Boris Chiou [:boris] (away 1/27 ~ 2/1) from comment #2)
> After applying patches of bug 1317209, we start to get this test failed:
>
> Assertion failure: !aStyleContext->StyleSource().IsServoComputedValues()
> (Bug 1311257: Servo backend does not support the base value yet), at
> /home/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:955
>
> TEST-UNEXPECTED-FAIL |
> file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/
> crashtests/1323114-2.html | application terminated with exit code 11
> REFTEST PROCESS-CRASH |
> file:///home/worker/workspace/build/tests/reftest/tests/dom/animation/test/
> crashtests/1323114-2.html | application crashed [@
> mozilla::EffectCompositor::GetBaseStyle]
>
>
> And I think this bug can fix this assertion.
Bug 1329878 instead of this bug?
Assignee | ||
Updated•8 years ago
|
Blocks: stylo-css-animations
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•8 years ago
|
||
I am going to add an FFI which does a bunch of stuff what we currently do in KeyframeEffectReadOnly::ComposeStyleRule(). To do this, we need to expose AnimationPropertySegment and ComputedTiming, and another FFI to call ComputedTimingFunction::GetPortion().
Assignee | ||
Comment 5•8 years ago
|
||
I just realized that we need to expose the hash table which stores base styles for each properties.
Assignee | ||
Comment 6•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•8 years ago
|
||
Sorry, I'm not going to be able to get to these reviews before the weekend but I had a quick glance and I'm curious about the "Add FFI functions to get progress value and current position in a segment" part. Are they calculations we could just do on the Servo side? I guess we currently still do all timing calculations in Gecko so that wouldn't make sense, right?
Assignee | ||
Comment 22•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #21)
> Sorry, I'm not going to be able to get to these reviews before the weekend
> but I had a quick glance and I'm curious about the "Add FFI functions to get
> progress value and current position in a segment" part. Are they
> calculations we could just do on the Servo side?
We could, but we need to expose Maybe<> and Nullable<>, and handle them in Rust.
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8853171 [details]
Bug 1311257 - Add a function that returns a base computed values (i.e. computed values without any animations rules).
https://reviewboard.mozilla.org/r/125236/#review128370
::: layout/style/ServoBindingList.h:326
(Diff revision 1)
> mozilla::TraversalRootBehavior root_behavior)
>
> // Assert that the tree has no pending or unconsumed restyles.
> SERVO_BINDING_FUNC(Servo_AssertTreeIsClean, void, RawGeckoElementBorrowed root)
>
> +// Returns computed values for the given element withour any animations rules.
without
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.
https://reviewboard.mozilla.org/r/125240/#review128366
::: dom/animation/KeyframeEffectReadOnly.cpp:455
(Diff revision 1)
> void
> KeyframeEffectReadOnly::EnsureBaseStyles(
> + const ServoComputedValuesWithParent& aServoValues,
> + const nsTArray<AnimationProperty>& aProperties)
> +{
> + if (!mTarget) {
> + return;
> + }
> +
> + RefPtr<ServoComputedValues> baseComputedValues;
> +
> + nsPresContext* presContext =
> + nsContentUtils::GetContextForContent(mTarget->mElement);
> + MOZ_ASSERT(presContext);
> +
> + for (const AnimationProperty& property : aProperties) {
Don't we need to clear mBaseStyleValuesForServo somewhere here? Otherwise will it just keep being added to?
::: dom/animation/KeyframeEffectReadOnly.cpp:479
(Diff revision 1)
> + }
> +
> + if (!baseComputedValues) {
> + baseComputedValues =
> + presContext->StyleSet()->AsServo()->
> + GetBaseComputedValuesForElement(mTarget->mElement, nullptr);
Don't we want to pass the pseudo element type here instead of nullptr?
Comment 25•8 years ago
|
||
mozreview-review |
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.
https://reviewboard.mozilla.org/r/125246/#review128376
This looks fine but could we get a commit message explaining what this is for? It seems unfortunate to have to cross the FFI boundary for these trivial calculations but if they don't happen very frequently (e.g. only when creating a transitions, as opposed to testing if a transition is necessary etc.) then it's probably fine.
::: layout/style/ServoBindings.cpp:505
(Diff revision 1)
> + double positionInSegment =
> + (aComputedTiming->mProgress.Value() - aSegment->mFromKey) /
> + (aSegment->mToKey - aSegment->mFromKey);
Is it ok to assume that aSegment->mToKey != aSegment->mFromKey ?
If so, we should assert that. If not, we should handle that case (presumably by returning 1.0?)
Comment 26•8 years ago
|
||
mozreview-review |
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.
https://reviewboard.mozilla.org/r/125250/#review128378
::: layout/style/ServoBindings.cpp:518
(Diff revision 1)
> + reinterpret_cast<nsRefPtrHashtable<nsUint32HashKey, RawServoAnimationValue>*>
> + (aBaseStyles);
I'm pretty sure that we should prefer static_cast here
::: servo/ports/geckolib/glue.rs:261
(Diff revision 1)
> use style::properties::animated_properties::AnimationValueMap;
>
> - let property: TransitionProperty = property.into();
> + let property: TransitionProperty = css_property.into();
> let value_map = RwLock::<AnimationValueMap>::as_arc(&raw_value_map);
>
> - let from_value = unsafe { &*segment.mFromValue.mServo.mRawPtr };
> + // Get the underlying value from previous composed value or cached base values.
"If either of the segment endpoints are null, get the underlying value to use from the current value in the values map (set by a lower-priority effect), or, if there is no current value, look up the cached base value for this property."
?
::: servo/ports/geckolib/glue.rs:273
(Diff revision 1)
> + if (segment.mFromValue.mServo.mRawPtr.is_null() ||
> + segment.mToValue.mServo.mRawPtr.is_null()) &&
> + underlying_value.is_none() {
> + // This is a case that servo does not support a given property as animatable?
> + return;
> + }
What sort of cases cause this to happen?
::: servo/ports/geckolib/glue.rs:280
(Diff revision 1)
> + underlying_value.is_none() {
> + // This is a case that servo does not support a given property as animatable?
> + return;
> + }
> +
> + // Declare for making derefenced raw pointer alive out side the if block.
Nit: s/out side/outside/
(Although I wonder if this comment is necessary, or if there is a neater way of doing this?)
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8853179 [details]
Bug 1311257 - getKeyframes() returns base computed values in missing keyframes.
https://reviewboard.mozilla.org/r/125252/#review128386
Attachment #8853179 -
Flags: review?(bbirtles) → review+
Comment 28•8 years ago
|
||
mozreview-review |
Comment on attachment 8853180 [details]
Bug 1311257 - Support missing keyframes handling for stylo.
https://reviewboard.mozilla.org/r/125254/#review128388
::: dom/animation/KeyframeUtils.cpp:472
(Diff revision 1)
> - // FIXME: Bug 1311257: Support missing keyframes for Servo backend.
> + if (!AnimationUtils::IsCoreAPIEnabled() &&
> - if ((!AnimationUtils::IsCoreAPIEnabled() ||
> - aDocument->IsStyledByServo()) &&
Is this right? We don't support additive animation yet do we?
Comment 29•8 years ago
|
||
There are a lot of patches so I haven't checked them all, but I notice there are references to this bug in:
KeyframeUtils.cpp
nsLayoutUtils.cpp
nsDisplayList.cpp
nsComputedDOMStyle.cpp
and I wonder if this bug drops / updates them all.
Comment 30•8 years ago
|
||
mozreview-review |
Comment on attachment 8853168 [details]
Bug 1311257 - Make TransitionProperty::from_declaration() convert PropertyDeclaration::{CSSWideKeyword,WithVariables} as well.
https://reviewboard.mozilla.org/r/125230/#review128418
::: commit-message-f9798:1
(Diff revision 1)
> +Bug 1311257 - TransitionProperty::from_declaration() converts PropertyDeclaration::{CSSWideKeyword,WithVariables} as well. r?heycam
Nit: Better to write the commit message in terms of what it's changing, rather than the state of the code after it's changed. So:
Make TransitionProperty::from_declaration() convert PropertyDeclaration::{CSSWideKeyword,WithVariables} as well.
Attachment #8853168 -
Flags: review?(cam) → review+
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8853169 [details]
Bug 1311257 - Make cascade_with_rules take SharedStyleContext instead StyleContext.
https://reviewboard.mozilla.org/r/125232/#review128420
Is this just a cleanup, since cascade_with_rules doesn't need anything on the StyleContext itself?
::: commit-message-9726c:1
(Diff revision 1)
> +Bug 1311257 - cascade_with_rules takes SharedStyleContext instead StyleContext. r?heycam
Nit: similarly here: "Make cascade_with_rules take ...".
Attachment #8853169 -
Flags: review?(cam) → review+
Comment 32•8 years ago
|
||
mozreview-review |
Comment on attachment 8853170 [details]
Bug 1311257 - Make cascade_with_rules() take a boolean representing whether the cascade is for pseudo element or not.
https://reviewboard.mozilla.org/r/125234/#review128422
::: commit-message-7091c:1
(Diff revision 1)
> +Bug 1311257 - cascade_with_rules() takes a boolean representing whether the cascade is for pseudo element or not. r?heycam
Nit: and here.
Attachment #8853170 -
Flags: review?(cam) → review+
Comment 33•8 years ago
|
||
mozreview-review |
Comment on attachment 8853171 [details]
Bug 1311257 - Add a function that returns a base computed values (i.e. computed values without any animations rules).
https://reviewboard.mozilla.org/r/125236/#review128424
::: layout/style/ServoBindingList.h:331
(Diff revision 1)
> +// Returns computed values for the given element withour any animations rules.
> +SERVO_BINDING_FUNC(Servo_StyleSet_GetBaseComputedValuesForElement,
> + ServoComputedValuesStrong,
> + RawServoStyleSetBorrowed set,
> + RawGeckoElementBorrowed element,
> + nsIAtom* pseudoTag)
Nit: this should really be "pseudo_tag" to follow Rust identifier naming style. Also, blank line after this.
::: servo/components/style/matching.rs:1225
(Diff revision 1)
> });
> }
> }
> +
> + /// Returns computed values without animation and transition rules.
> + fn get_base_computed_values(&self,
Nit: Even though this function does something similar to get_after_change_style (i.e., computes styles with certain rule nodes removed), its naming doesn't match. Should we call this one get_base_style?
Attachment #8853171 -
Flags: review?(cam) → review+
Comment 34•8 years ago
|
||
mozreview-review |
Comment on attachment 8853172 [details]
Bug 1311257 - Add an FFI function that returns an AnimationValue for a given nsCSSPropertyID from computed values.
https://reviewboard.mozilla.org/r/125238/#review128426
Attachment #8853172 -
Flags: review?(cam) → review+
Assignee | ||
Comment 35•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853169 [details]
Bug 1311257 - Make cascade_with_rules take SharedStyleContext instead StyleContext.
https://reviewboard.mozilla.org/r/125232/#review128420
Yes, that's right.
Assignee | ||
Comment 36•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.
https://reviewboard.mozilla.org/r/125240/#review128366
> Don't we want to pass the pseudo element type here instead of nullptr?
I did totally foget this. When I was writing this code, I couldn't remember the function name to get pseudo nsIAtom, so I thought, at that time, I write it later. Thanks!
Assignee | ||
Comment 37•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.
https://reviewboard.mozilla.org/r/125246/#review128376
> Is it ok to assume that aSegment->mToKey != aSegment->mFromKey ?
>
> If so, we should assert that. If not, we should handle that case (presumably by returning 1.0?)
Yeah, we should assert.
Assignee | ||
Comment 38•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.
https://reviewboard.mozilla.org/r/125250/#review128378
> What sort of cases cause this to happen?
This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.
Assignee | ||
Comment 39•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853180 [details]
Bug 1311257 - Support missing keyframes handling for stylo.
https://reviewboard.mozilla.org/r/125254/#review128388
> Is this right? We don't support additive animation yet do we?
No, we don't support additive animation yet. But the function, RequiresAdditiveAnimation, does not check that composite is 'add', it just checks there is missing keyframe.
Comment 40•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.
https://reviewboard.mozilla.org/r/125250/#review128378
> This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.
I still don't understand. Does this ever happen? When? Why?
Assignee | ||
Comment 41•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #40)
> Comment on attachment 8853178 [details]
> Bug 1311257 - Use underlying value for missing keyframes.
>
> https://reviewboard.mozilla.org/r/125250/#review128378
>
> > This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.
>
> I still don't understand. Does this ever happen? When? Why?
This is a result of discussion with us in a security bug, I don't refer to the bug here because it's still not in public.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 57•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #29)
> There are a lot of patches so I haven't checked them all, but I notice there
> are references to this bug in:
>
> KeyframeUtils.cpp
> nsLayoutUtils.cpp
> nsDisplayList.cpp
> nsComputedDOMStyle.cpp
>
> and I wonder if this bug drops / updates them all.
Thanks, I will update comments in an additional patch here.
Comment 58•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> (In reply to Brian Birtles (:birtles) from comment #40)
> > Comment on attachment 8853178 [details]
> > Bug 1311257 - Use underlying value for missing keyframes.
> >
> > https://reviewboard.mozilla.org/r/125250/#review128378
> >
> > > This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.
> >
> > I still don't understand. Does this ever happen? When? Why?
>
> This is a result of discussion with us in a security bug, I don't refer to
> the bug here because it's still not in public.
Then please CC me on the bug.
Assignee | ||
Comment 59•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #58)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #41)
> > (In reply to Brian Birtles (:birtles) from comment #40)
> > > Comment on attachment 8853178 [details]
> > > Bug 1311257 - Use underlying value for missing keyframes.
> > >
> > > https://reviewboard.mozilla.org/r/125250/#review128378
> > >
> > > > This case should not happen ideally, but we'd prefer *turbulent* animations rather than assertion or crash.
> > >
> > > I still don't understand. Does this ever happen? When? Why?
> >
> > This is a result of discussion with us in a security bug, I don't refer to
> > the bug here because it's still not in public.
>
> Then please CC me on the bug.
After discussion with Brian, we decided to add a warning here.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 70•8 years ago
|
||
mozreview-review |
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.
https://reviewboard.mozilla.org/r/125240/#review128812
This looks fine but I should probably check once more if you decide to make any of the following changes.
::: dom/animation/KeyframeEffectReadOnly.cpp:464
(Diff revision 3)
> + mBaseStyleValuesForServo.Clear();
> +
> + RefPtr<ServoComputedValues> baseComputedValues;
> +
> + nsPresContext* presContext =
> + nsContentUtils::GetContextForContent(mTarget->mElement);
> + MOZ_ASSERT(presContext);
> +
> + nsIAtom* pseudoAtom = mTarget->mPseudoType < CSSPseudoElementType::Count
> + ? nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType)
> + : nullptr;
> + for (const AnimationProperty& property : aProperties) {
It seems like baseComputedValues is a long way from where it is used? I wonder if this would be easier to read if we put the part where we fetch the pres context sooner?
e.g.
mBaseStyleValuesForServo.Clear();
nsPresContext* presContext =
nsContentUtils::GetContextForContent(mTarget->mElement);
MOZ_ASSERT(presContext);
RefPtr<ServoComputedValues> baseComputedValues;
nsIAtom* pseudoAtom = mTarget->mPseudoType < CSSPseudoElementType::Count
? nsCSSPseudoElements::GetPseudoAtom(mTarget->mPseudoType)
: nullptr;
for (const AnimationProperty& property : aProperties) {
EnsureBaseStyle(property, presContext, baseComputedValues);
}
::: dom/animation/KeyframeEffectReadOnly.cpp:468
(Diff revision 3)
> + nsPresContext* presContext =
> + nsContentUtils::GetContextForContent(mTarget->mElement);
> + MOZ_ASSERT(presContext);
Why is this guaranteed to be non-null?
::: dom/animation/KeyframeEffectReadOnly.cpp:475
(Diff revision 3)
> + for (const AnimationProperty& property : aProperties) {
> + for (const AnimationPropertySegment& segment : property.mSegments) {
> + if (segment.HasReplaceableValues()) {
> + continue;
> + }
> +
> + if (!baseComputedValues) {
> + baseComputedValues =
> + presContext->StyleSet()->AsServo()->
> + GetBaseComputedValuesForElement(mTarget->mElement, pseudoAtom);
> + }
> + RefPtr<RawServoAnimationValue> baseValue =
> + Servo_ComputedValues_ExtractAnimationValue(
> + baseComputedValues, property.mProperty).Consume();
> + mBaseStyleValuesForServo.Put(property.mProperty, baseValue);
> + break;
> + }
> + }
Nested for loops with breaks are a bit hard to read (although I see we do that in the Gecko-version of this method). Do you think it makes sense to factor out a method for the inner loop?
Attachment #8853173 -
Flags: review?(bbirtles)
Comment 71•8 years ago
|
||
mozreview-review |
Comment on attachment 8853177 [details]
Bug 1311257 - Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust.
https://reviewboard.mozilla.org/r/125248/#review128824
::: servo/ports/geckolib/glue.rs:264
(Diff revision 3)
> + let from_value = AnimationValue::as_arc(&from_value).as_ref();
> + let to_value = unsafe { &*segment.mToValue.mServo.mRawPtr };
> + let to_value = AnimationValue::as_arc(&to_value).as_ref();
> +
> + if segment.mToKey == segment.mFromKey {
> + if unsafe { Gecko_ComputedTimingProgress(computed_timing) } < 0.5 {
This should be 0 not 0.5 right?
Comment 72•8 years ago
|
||
mozreview-review |
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.
https://reviewboard.mozilla.org/r/125246/#review128822
This seems fine but I wonder if it makes sense to just create a method that converts a Gecko ComputedTiming object into something that Servo can deal with? And then instead of having:
double Gecko_ComputedTimingProgress(aComputedTiming)
double Gecko_SegmentGetPosition(aSegment, aComputedTiming)
have:
ServoComputedTiming Gecko_GetComputedTiming(aComputedTiming)
double Gecko_GetPositionInSegment(aSegment, aProgress, aBeforeFlag)
?
::: layout/style/ServoBindings.h:190
(Diff revision 3)
> +double Gecko_ComputedTimingProgress(RawGeckoComputedTimingBorrowed aComputedTiming);
> +double Gecko_SegmentGetPosition(RawGeckoAnimationPropertySegmentBorrowed aSegment,
I wonder if the names could be more clear.
For the first one:
Gecko_GetProgressFromComputedTiming?
Gecko_GetProgress?
For the second one:
Gecko_GetPositionInSegment?
Gecko_GetSegmentPortion?
::: layout/style/ServoBindings.cpp:523
(Diff revision 3)
> + MOZ_ASSERT(aSegment->mFromKey != aSegment->mToKey,
> + "The segment from and to keys should be different");
Perhaps we should just assert that mFromKey < mToKey?
Comment 73•8 years ago
|
||
mozreview-review |
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.
https://reviewboard.mozilla.org/r/125250/#review128830
::: layout/style/ServoBindings.cpp:539
(Diff revision 3)
>
> +RawServoAnimationValueBorrowedOrNull
> +Gecko_AnimationGetBaseStyle(void* aBaseStyles, nsCSSPropertyID aProperty)
> +{
> + auto base =
> + reinterpret_cast<nsRefPtrHashtable<nsUint32HashKey, RawServoAnimationValue>*>
I think you changed the wrong reinterpret_cast to a static_cast? (Although we probably should change the other one too unless I'm mistaken.)
::: servo/ports/geckolib/glue.rs:260
(Diff revision 3)
> use style::properties::animated_properties::AnimationValueMap;
>
> - let property: TransitionProperty = property.into();
> + let property: TransitionProperty = css_property.into();
> let value_map = RwLock::<AnimationValueMap>::as_arc(&raw_value_map);
>
> - let from_value = unsafe { &*segment.mFromValue.mServo.mRawPtr };
> + // if either of segment endpoints are null, get the underlying value to use
s/if/If/
s/of segments/of the segments/
::: servo/ports/geckolib/glue.rs:278
(Diff revision 3)
> + // This should not happen. But if it happened, we'd prefer *turbulent*
> + // animation rather than crash.
> + return;
As discussed, let's add a warning here and drop the comment.
Attachment #8853178 -
Flags: review?(bbirtles) → review+
Comment 74•8 years ago
|
||
mozreview-review |
Comment on attachment 8853180 [details]
Bug 1311257 - Support missing keyframes handling for stylo.
https://reviewboard.mozilla.org/r/125254/#review128832
Attachment #8853180 -
Flags: review?(bbirtles) → review+
Comment 75•8 years ago
|
||
mozreview-review |
Comment on attachment 8854202 [details]
Bug 1311257 - Update comment refering to bug 1311257.
https://reviewboard.mozilla.org/r/126156/#review128836
Attachment #8854202 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 76•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.
https://reviewboard.mozilla.org/r/125246/#review128822
We need to get |aComputedTiming|'s mProgress in Rust side, and mProgress is Nullable<double> in ComputedTiming. Are you suggesting we pass mProgress as an argument of Servo_AnimationCompose()?
Comment 77•8 years ago
|
||
I was just suggesting it would be neater to be able to convert the whole struct at once. Then when we calculate the portion of a segment, just pass in the required info: progress and before flag.
Assignee | ||
Comment 78•8 years ago
|
||
Ah, OK. I think I understand it now.
We can already get mBeforeFlag from ComputedTiming in Rust, so how about this:
let progress = unsafe { Gecko_ProgressFromComputedTimingProgress(computed_timing); };
...
let position = unsafe { Gecko_PositionInSegment(segment, progress, computed_timing.mBeforeFlag); }
?
Comment 79•8 years ago
|
||
Oh, I see. Yes, that's probably better (although I assume Gecko_ProgressFromComputedTimingProgress is a typo). If we support Nullable in the future then we can just drop the extra call.
Assignee | ||
Comment 80•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #79)
> Oh, I see. Yes, that's probably better (although I assume
> Gecko_ProgressFromComputedTimingProgress is a typo).
Yeah, it means we have been made *much* progress. ;-)
Comment 81•8 years ago
|
||
mozreview-review |
Comment on attachment 8853174 [details]
Bug 1311257 - Move AnimationPropertySegment in a separate header and expose it in FFI.
https://reviewboard.mozilla.org/r/125242/#review129154
Attachment #8853174 -
Flags: review?(cam) → review+
Comment 82•8 years ago
|
||
mozreview-review |
Comment on attachment 8853175 [details]
Bug 1311257 - Expose ComputedTiming to FFI.
https://reviewboard.mozilla.org/r/125244/#review129156
::: commit-message-2b78a:1
(Diff revision 3)
> +Bug 1311257 - Expose ComptuedTiming to FFI. r?heycam
ComputedTiming
Attachment #8853175 -
Flags: review?(cam) → review+
Assignee | ||
Comment 83•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.
https://reviewboard.mozilla.org/r/125240/#review128812
> Why is this guaranteed to be non-null?
This EnsureBaseStyles() is called right after getting servo's computed values, we can't get the servo's computed values without nsPresContext.
Assignee | ||
Comment 84•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853177 [details]
Bug 1311257 - Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust.
https://reviewboard.mozilla.org/r/125248/#review128824
> This should be 0 not 0.5 right?
Exactly. It's a terrible mistake. Thank you!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 100•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.
https://reviewboard.mozilla.org/r/125240/#review128812
> This EnsureBaseStyles() is called right after getting servo's computed values, we can't get the servo's computed values without nsPresContext.
We need either a comment or an assertion message to say that.
Comment 101•8 years ago
|
||
mozreview-review |
Comment on attachment 8853176 [details]
Bug 1311257 - Add FFI functions to get progress value and current position in a segment.
https://reviewboard.mozilla.org/r/125246/#review129172
::: commit-message-b8e90:4
(Diff revision 4)
> +Bug 1311257 - Add FFI functions to get progress value and current position in a segment. r?birtles
> +
> +Two functions added in this patch get progress value from ComputedTiming
> +or get position in a given AnimationPropertySegment with ComputedTiming.
(This comment is no longer quite accurate. It should just say 'get the position in a given AnimationPropertySegment' or perhaps 'get the position in a given AnimationPropertySegment given the relevant timing properties'.)
::: layout/style/ServoBindings.cpp:524
(Diff revision 4)
> + MOZ_ASSERT(aSegment->mFromKey < aSegment->mToKey,
> + "The segment to key should be greater than from key");
(It's a bit odd that the code and message are opposites. Mind making them the same?)
Attachment #8853176 -
Flags: review?(bbirtles) → review+
Comment 102•8 years ago
|
||
mozreview-review |
Comment on attachment 8853177 [details]
Bug 1311257 - Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust.
https://reviewboard.mozilla.org/r/125248/#review129182
::: dom/animation/KeyframeEffectReadOnly.cpp
(Diff revision 4)
> }
>
> - // Special handling for zero-length segments
> + Servo_AnimationCompose(&aAnimationValues,
> - if (aSegment.mToKey == aSegment.mFromKey) {
> - if (aComputedTiming.mProgress.Value() < 0) {
> - Servo_AnimationValueMap_Push(&aAnimationValues,
Are any of these Servo_Animation* functions now unused, and can be removed?
Attachment #8853177 -
Flags: review?(cam) → review+
Assignee | ||
Comment 103•8 years ago
|
||
(In reply to Cameron McCormack (:heycam) (away Apr 1-4) from comment #102)
> Comment on attachment 8853177 [details]
> Bug 1311257 - Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust.
>
> https://reviewboard.mozilla.org/r/125248/#review129182
>
> ::: dom/animation/KeyframeEffectReadOnly.cpp
> (Diff revision 4)
> > }
> >
> > - // Special handling for zero-length segments
> > + Servo_AnimationCompose(&aAnimationValues,
> > - if (aSegment.mToKey == aSegment.mFromKey) {
> > - if (aComputedTiming.mProgress.Value() < 0) {
> > - Servo_AnimationValueMap_Push(&aAnimationValues,
>
> Are any of these Servo_Animation* functions now unused, and can be removed?
Probably. But I am not yet convinced which one will not be used for SMIL either. So I'd like to leave them for now.
Comment 104•8 years ago
|
||
mozreview-review |
Comment on attachment 8853178 [details]
Bug 1311257 - Use underlying value for missing keyframes.
https://reviewboard.mozilla.org/r/125250/#review129190
::: layout/style/ServoBindingList.h:211
(Diff revision 4)
> SERVO_BINDING_FUNC(Servo_AnimationCompose, void,
> RawServoAnimationValueMapBorrowed,
> + void*,
> nsCSSPropertyID property,
> RawGeckoAnimationPropertySegmentBorrowed,
> RawGeckoComputedTimingBorrowed)
Please document what the void* argument is for. (It might be nice to give names to this and the other arguments, too.)
::: layout/style/ServoBindings.h:196
(Diff revision 4)
> +RawServoAnimationValueBorrowedOrNull Gecko_AnimationGetBaseStyle(
> + void* aBaseStyles,
> + nsCSSPropertyID aProperty);
And document it here too.
Attachment #8853178 -
Flags: review?(cam) → review+
Comment 105•8 years ago
|
||
mozreview-review |
Comment on attachment 8853181 [details]
Bug 1311257 - Update test expectations.
https://reviewboard.mozilla.org/r/125256/#review129194
Attachment #8853181 -
Flags: review?(cam) → review+
Comment 106•8 years ago
|
||
mozreview-review |
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.
https://reviewboard.mozilla.org/r/125240/#review129232
::: dom/animation/KeyframeEffectReadOnly.cpp:474
(Diff revision 4)
> + for (const AnimationProperty& property : aProperties) {
> + for (const AnimationPropertySegment& segment : property.mSegments) {
> + if (segment.HasReplaceableValues()) {
> + continue;
> + }
> +
> + EnsureBaseStyle(property.mProperty,
> + pseudoAtom,
> + presContext,
> + baseComputedValues);
> + break;
> + }
> + }
> +}
> +
> +void
> +KeyframeEffectReadOnly::EnsureBaseStyle(
> + nsCSSPropertyID aProperty,
> + nsIAtom* aPseudoAtom,
> + nsPresContext* aPresContext,
> + RefPtr<ServoComputedValues>& aBaseComputedValues)
> +{
> + if (!aBaseComputedValues) {
> + aBaseComputedValues =
> + aPresContext->StyleSet()->AsServo()->
> + GetBaseComputedValuesForElement(mTarget->mElement, aPseudoAtom);
> + }
> + RefPtr<RawServoAnimationValue> baseValue =
> + Servo_ComputedValues_ExtractAnimationValue(aBaseComputedValues,
> + aProperty).Consume();
> + mBaseStyleValuesForServo.Put(aProperty, baseValue);
> +}
Oh, I actually meant to factor out a method for the inner for-loop so we don't have a break within a nested for-loop.
Does that work?
Attachment #8853173 -
Flags: review?(bbirtles)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 117•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #106)
> Comment on attachment 8853173 [details]
> Bug 1311257 - Store base styles for stylo.
>
> https://reviewboard.mozilla.org/r/125240/#review129232
>
> ::: dom/animation/KeyframeEffectReadOnly.cpp:474
> (Diff revision 4)
> > + for (const AnimationProperty& property : aProperties) {
> > + for (const AnimationPropertySegment& segment : property.mSegments) {
> > + if (segment.HasReplaceableValues()) {
> > + continue;
> > + }
> > +
> > + EnsureBaseStyle(property.mProperty,
> > + pseudoAtom,
> > + presContext,
> > + baseComputedValues);
> > + break;
> > + }
> > + }
> > +}
> > +
> > +void
> > +KeyframeEffectReadOnly::EnsureBaseStyle(
> > + nsCSSPropertyID aProperty,
> > + nsIAtom* aPseudoAtom,
> > + nsPresContext* aPresContext,
> > + RefPtr<ServoComputedValues>& aBaseComputedValues)
> > +{
> > + if (!aBaseComputedValues) {
> > + aBaseComputedValues =
> > + aPresContext->StyleSet()->AsServo()->
> > + GetBaseComputedValuesForElement(mTarget->mElement, aPseudoAtom);
> > + }
> > + RefPtr<RawServoAnimationValue> baseValue =
> > + Servo_ComputedValues_ExtractAnimationValue(aBaseComputedValues,
> > + aProperty).Consume();
> > + mBaseStyleValuesForServo.Put(aProperty, baseValue);
> > +}
>
> Oh, I actually meant to factor out a method for the inner for-loop so we
> don't have a break within a nested for-loop.
>
> Does that work?
Oops. I did push revised patch coincidently. Yeah, that should work. I will revise it.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 128•8 years ago
|
||
mozreview-review |
Comment on attachment 8853173 [details]
Bug 1311257 - Store base styles for stylo.
https://reviewboard.mozilla.org/r/125240/#review129732
Sorry for the delay. I didn't notice that this review had been updated.
::: dom/animation/KeyframeEffectReadOnly.h:426
(Diff revision 6)
> // base style context will be resolved and stored in
> // |aCachedBaseStyleContext|.
> void EnsureBaseStyle(nsCSSPropertyID aProperty,
> nsStyleContext* aStyleContext,
> RefPtr<nsStyleContext>& aCachedBaseStyleContext);
> + // Stylo version of the above function.
Stylo version of the above function that also first checks for an additive value in |aProperty|'s list of segments.
?
::: dom/animation/KeyframeEffectReadOnly.cpp:455
(Diff revision 6)
> void
> KeyframeEffectReadOnly::EnsureBaseStyles(
> + const ServoComputedValuesWithParent& aServoValues,
> + const nsTArray<AnimationProperty>& aProperties)
> +{
In the header file, the Gecko version comes first followed by the Servo version but in the cpp file its the reverse. Can we move these two methods down below the Gecko versions?
::: dom/animation/KeyframeEffectReadOnly.cpp:492
(Diff revision 6)
> + for (const AnimationPropertySegment& segment : aProperty.mSegments) {
> + if (segment.HasReplaceableValues()) {
> + continue;
> + }
> +
> + if (!aBaseComputedValues) {
> + aBaseComputedValues =
> + aPresContext->StyleSet()->AsServo()->
> + GetBaseComputedValuesForElement(mTarget->mElement, aPseudoAtom);
> + }
> + RefPtr<RawServoAnimationValue> baseValue =
> + Servo_ComputedValues_ExtractAnimationValue(aBaseComputedValues,
> + aProperty.mProperty).Consume();
> + mBaseStyleValuesForServo.Put(aProperty.mProperty, baseValue);
> + return;
> + }
(Now that this is a separate method, I think it would be more clear to just write:
bool hasAdditiveValues = false;
for (const AnimationPropertySegment& segment : aProperty.mSegments) {
if (!segment.HasReplaceableValues()) {
hasAdditiveValues = true;
break;
}
}
if (!hasAdditiveValues) {
return;
}
...
But it's up to you which you prefer.)
Attachment #8853173 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 129•8 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #128)
> ::: dom/animation/KeyframeEffectReadOnly.cpp:492
> (Diff revision 6)
> > + for (const AnimationPropertySegment& segment : aProperty.mSegments) {
> > + if (segment.HasReplaceableValues()) {
> > + continue;
> > + }
> > +
> > + if (!aBaseComputedValues) {
> > + aBaseComputedValues =
> > + aPresContext->StyleSet()->AsServo()->
> > + GetBaseComputedValuesForElement(mTarget->mElement, aPseudoAtom);
> > + }
> > + RefPtr<RawServoAnimationValue> baseValue =
> > + Servo_ComputedValues_ExtractAnimationValue(aBaseComputedValues,
> > + aProperty.mProperty).Consume();
> > + mBaseStyleValuesForServo.Put(aProperty.mProperty, baseValue);
> > + return;
> > + }
>
> (Now that this is a separate method, I think it would be more clear to just
> write:
>
> bool hasAdditiveValues = false;
>
> for (const AnimationPropertySegment& segment : aProperty.mSegments) {
> if (!segment.HasReplaceableValues()) {
> hasAdditiveValues = true;
> break;
> }
> }
>
> if (!hasAdditiveValues) {
> return;
> }
Oh, thanks. This looks more consistent with our coding rules.
Assignee | ||
Comment 130•8 years ago
|
||
A final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e289630e5d738e2ddadee29e465e9943c3908ed4
I will pull binding changes from this try, and send a PR with the binding changes.
Assignee | ||
Comment 131•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #130)
> A final try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=e289630e5d738e2ddadee29e465e9943c3908ed4
>
> I will pull binding changes from this try, and send a PR with the binding
> changes.
A commit for servo directory was missed in the try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=43c5ad8e17e655785296f98b77350d026f0212c4
Assignee | ||
Comment 132•8 years ago
|
||
The try looks good so far.
https://github.com/servo/servo/pull/16280
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8853168 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8853169 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8853170 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8853175 -
Attachment is obsolete: true
Comment 144•8 years ago
|
||
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f9acbd78af1
Add a function that returns a base computed values (i.e. computed values without any animations rules). r=heycam
https://hg.mozilla.org/integration/autoland/rev/b66975fb5bde
Add an FFI function that returns an AnimationValue for a given nsCSSPropertyID from computed values. r=heycam
https://hg.mozilla.org/integration/autoland/rev/96818915960d
Store base styles for stylo. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ae42776a3d46
Move AnimationPropertySegment in a separate header and expose it in FFI. r=heycam
https://hg.mozilla.org/integration/autoland/rev/71b603b0f1cd
Add FFI functions to get progress value and current position in a segment. r=birtles
https://hg.mozilla.org/integration/autoland/rev/f19d868126f3
Move stuff of KeyframeEffectReadOnly::ComposeStyleRule in Rust. r=heycam
https://hg.mozilla.org/integration/autoland/rev/17e382a02976
Use underlying value for missing keyframes. r=birtles,heycam
https://hg.mozilla.org/integration/autoland/rev/c1f723812558
getKeyframes() returns base computed values in missing keyframes. r=birtles
https://hg.mozilla.org/integration/autoland/rev/d5b7c664155e
Support missing keyframes handling for stylo. r=birtles
https://hg.mozilla.org/integration/autoland/rev/7fc07c5261d7
Update test expectations. r=heycam
https://hg.mozilla.org/integration/autoland/rev/bab909e1e4d3
Update comment refering to bug 1311257. r=birtles
Comment 145•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f9acbd78af1
https://hg.mozilla.org/mozilla-central/rev/b66975fb5bde
https://hg.mozilla.org/mozilla-central/rev/96818915960d
https://hg.mozilla.org/mozilla-central/rev/ae42776a3d46
https://hg.mozilla.org/mozilla-central/rev/71b603b0f1cd
https://hg.mozilla.org/mozilla-central/rev/f19d868126f3
https://hg.mozilla.org/mozilla-central/rev/17e382a02976
https://hg.mozilla.org/mozilla-central/rev/c1f723812558
https://hg.mozilla.org/mozilla-central/rev/d5b7c664155e
https://hg.mozilla.org/mozilla-central/rev/7fc07c5261d7
https://hg.mozilla.org/mozilla-central/rev/bab909e1e4d3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•