Closed
Bug 1328787
Opened 7 years ago
Closed 7 years ago
Stylo: Convert Servo's animation keyframes and store them into Gecko's keyframes
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
(Blocks 1 open bug)
Details
Attachments
(13 files)
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
No description provided.
Assignee | ||
Updated•7 years ago
|
Blocks: stylo-css-animations
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•7 years ago
|
||
The basic idea here what I am doing to convert keyframes and store them in Gecko side is: * Call a function named Servo_StyleSet_FillKeyframesForName in CSSAnimationBuilder::Build() to fill nsTArray<Keyframe> in for each animation's name in servo side. * Call Gecko_AnimationAppendKeyframe from the Servo_StyleSet_FillKeyframesForName to append a new Gecko's Keyframe into the nsTArray. That's because the Keyframe struct has Maybe<ComputedTimingFunction> (ComputedTimingFunction ≠ nsTimingFunction), if we handle it in servo side, we need other FFI functions to initialize Maybe<> and ComputedTimingFunction. * Call nsAnimationManager->UpdateAnimations() in ServoStyleSet::GetContext(). (I am not confident this place is the right place we should invoke CSS animations in stylo.)
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 15•7 years ago
|
||
I did not include generated binding stuff (structs_debug.rs and structs_release.rs) in this patch set, because, on my environment it results something weird (it causes an error without adding mozilla::binding_danger::AssertAndSuppressCleanupPolicy and mozilla::binding_danger::TErrorResult in whitelist.).
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8828676 [details] Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations(). https://reviewboard.mozilla.org/r/105988/#review106954 ::: layout/base/ServoRestyleManager.cpp:44 (Diff revision 1) > - // We allow posting change hints during restyling, but not restyle hints > - // themselves, since those would require us to re-traverse the tree. > - MOZ_ASSERT_IF(mInStyleRefresh, aRestyleHint == 0); > + // We allow posting change hints without any restyles hints during restyling, > + // since those would require us to re-traverse the tree, and allow animation > + // only restyles without any change hints. > + MOZ_ASSERT_IF(mInStyleRefresh, aRestyleHint == 0 || > + (aRestyleHint == eRestyle_CSSAnimations && > + !aMinChangeHint)); Hmm, so the problem here is that any restyle hints that are generated from UpdateAnimations() won't get handled, and actually we can't really handle them properly under ServoStyleSet::RecreateStyleContexts, since we have already restyled the entire tree, and we're just creating nsStyleContexts. The assertion here was checking that we only generate change hints, since they don't require a re-traversal of the tree, which handling a restyle hint would. I don't recall what the plan was to handle this. The worst case would be to just do another whole tree traversal later to handle the eRestyle_CSSAnimations restyle hint, potentially wasting a lot of restyle work done on descendants of that element. Did we have a plan we discussed in Hawaii, where we would do something like: stop the Servo restyle at an element, do the animation updating back in ServoStyleSet::RecreateStyleContexts, then return to Servo to restyle the element and its descendants? Or am I misremembering a different conversation? ::: layout/style/ServoStyleSet.cpp:144 (Diff revision 1) > + // Update/build CSS animations in the case where animation properties are > + // changed. > + mPresContext->AnimationManager()->UpdateAnimations(result, element); I think this should be in the other GetContext one, since that's the deepest GetContext call (and matches where we do it in nsStyleSet.) Some questions about this: 1. nsStyleSet::GetContext checks |PresContext()->IsDynamic() && aElementForAnimation->IsInComposedDoc()| to see if it should skip animation rule updating. Should we do that here too? 2. Can you add some comments in here saying that we need to return a new style context in case the UpdateAnimations call cause changes to animations? I assume we'll do that work in a later bug.
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8828664 [details] Bug 1328787 - Part 1: Factor out Keyframe and PropertyValuePair into Keyframe.h. https://reviewboard.mozilla.org/r/105964/#review106956 ::: dom/animation/Keyframe.h:1 (Diff revision 1) > +#include "nsCSSPropertyID.h" Please add the license / modeline boilerplate and #ifdef guards to this file.
Attachment #8828664 -
Flags: review?(cam) → review+
Comment 18•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15) > I did not include generated binding stuff (structs_debug.rs and > structs_release.rs) in this patch set, because, on my environment it results > something weird (it causes an error without adding > mozilla::binding_danger::AssertAndSuppressCleanupPolicy and > mozilla::binding_danger::TErrorResult in whitelist.). Hmm, I thought I fixed that. I'll look at it in the next mozilla-central -> incubator/stylo merge.
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8828665 [details] Bug 1328787 - Part 2: Don't pass nsCSSKeyframesRule* to CSSAnimationBuild::Build. https://reviewboard.mozilla.org/r/105966/#review106960
Attachment #8828665 -
Flags: review?(cam) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8828668 [details] Bug 1328787 - Part 5: Implement Servo_StyleSet_FillKeyframesForName. https://reviewboard.mozilla.org/r/105972/#review106964 ::: servo/ports/geckolib/glue.rs:1111 (Diff revision 1) > +pub extern "C" fn Servo_StyleSet_FillKeyframesForName(raw_data: RawServoStyleSetBorrowed, > + name: *const nsACString, > + style: ServoComputedValuesBorrowed, > + keyframes: RawGeckoKeyframeListBorrowedMut) -> bool { > + let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut(); > + let device = data.stylist.device.clone(); Do you need to clone() the Arc<Device> here? Probably |let device = data.stylist.device.as_ref();| is sufficient. ::: servo/ports/geckolib/glue.rs:1115 (Diff revision 1) > + let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut(); > + let device = data.stylist.device.clone(); > + let name = unsafe { name.as_ref().unwrap().as_str_unchecked() }; > + let style = ComputedValues::as_arc(&style); > + > + let timing_function = style.get_box().animation_timing_function_mod(0); I don't understand why we just look at the first animation-timing-function value. Don't we want to look at the index the the |name| was found at in animation-name? ::: servo/ports/geckolib/glue.rs:1118 (Diff revision 1) > + for sheet in data.stylesheets.iter() { > + sheet.effective_keyframes_rules(&device, |ref keyframes_rule| { data.stylesheets is stored in cascade order, with UA sheets coming first and document sheets coming last. So, I think we should iterate over data.stylesheets in reverse order, and then quit as soon as we find a matching keyframes rule.
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8828666 [details] Bug 1328787 - Part 3: Add functions for filling each servo's animation keyframes into nsTArray<Keyframe>. https://reviewboard.mozilla.org/r/105968/#review106962 r=me but this maybe needs to change if my comments about the animation-timing-function on the part 5 patch are right. ::: layout/style/ServoStyleSet.cpp:562 (Diff revision 1) > + > + Nit: probably don't need two blank lines here.
Attachment #8828666 -
Flags: review?(cam) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8828667 [details] Bug 1328787 - Part 4: Add Gecko_AnimationAppendKeyframe. https://reviewboard.mozilla.org/r/105970/#review106970
Attachment #8828667 -
Flags: review?(cam) → review+
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8828669 [details] Bug 1328787 - Part 6: Update build_gecko.rs and bindings. https://reviewboard.mozilla.org/r/105974/#review106972
Attachment #8828669 -
Flags: review?(cam) → review+
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8828670 [details] Bug 1328787 - Part 7: Implement From for specified TransitionTimingFunction. https://reviewboard.mozilla.org/r/105976/#review106974 ::: servo/components/style/gecko_bindings/sugar/ns_timing_function.rs:15 (Diff revision 1) > +use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction; > use std::mem; > > -impl From<TransitionTimingFunction> for nsTimingFunction { > - fn from(function: TransitionTimingFunction) -> nsTimingFunction { > - let mut tf: nsTimingFunction = unsafe { mem::zeroed() }; > +impl nsTimingFunction { > + fn set_as_step(&mut self, function_type: nsTimingFunction_Type, steps: u32) { > + self.mType = function_type; Please debug_assert!() that function_type is one of the step types.
Attachment #8828670 -
Flags: review?(cam) → review+
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8828671 [details] Bug 1328787 - Part 8: Animation timing function can be overridden by animation-timing-function specified in keyframe. https://reviewboard.mozilla.org/r/105978/#review106976 ::: servo/components/style/keyframes.rs:204 (Diff revision 1) > value: value, > declared_timing_function: declared_timing_function, > } > } > + > + /// Return specified TransitionTimingFunction if this KeyframesSteps has 'animation-timing-funtion'. s/funtion/function/ ::: servo/components/style/keyframes.rs:218 (Diff revision 1) > + // Use the first value. > + Some(value.0[0]) Again, why the first value? ::: servo/components/style/keyframes.rs:221 (Diff revision 1) > + match *value { > + DeclaredValue::Value(ref value) => { > + // Use the first value. > + Some(value.0[0]) > + }, > + _ => None Nit: rust style is to not leave off a comma from the last item. ::: servo/components/style/keyframes.rs:228 (Diff revision 1) > + }, > + _ => panic!() > + } > + }, > + KeyframesStepValue::ComputedValues => { > + panic!("Shouldn't happen to set aniamtion-timing-function in missing keyframes") s/aniamtion/animation/ ::: servo/ports/geckolib/glue.rs:1128 (Diff revision 1) > + let mut timing_function = style_timing_function; > + if let Some(val) = step.get_animation_timing_function() { > + timing_function = val > + } > + > let tf = timing_function.into(); This is fine, but I think it might look more natural as: let timing_function = step.get_animation_timing_function().unwrap_or(style_timing_function);
Attachment #8828671 -
Flags: review?(cam) → review+
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8828672 [details] Bug 1328787 - Part 9: Set Keyframe.mPropertyValues for the case where keyframe is specified. https://reviewboard.mozilla.org/r/105980/#review106980 ::: servo/components/style/properties/helpers/animated_properties.mako.rs:110 (Diff revision 1) > + <% > + def to_nscsspropertyid(ident): > + if ident == "word_wrap": > + return "nsCSSPropertyID_eCSSPropertyAlias_WordWrap" > + > + if ident == "float": > + ident = "float_" > + return "nsCSSPropertyID::eCSSProperty_" + ident > + %> Hmm, can we just use the similar functions from properties.mako.rs, and put them somewhere we can call them from here? ::: servo/ports/geckolib/glue.rs:1156 (Diff revision 1) > + .filter(|&&(ref declaration, _)| { > + declaration.is_animatable() > + }); > + for (index, &(ref declaration, _)) in animatable.enumerate() { > + unsafe { > + (*keyframe).mPropertyValues.set_len((index+1) as u32); Nit: spaces around "+".
Attachment #8828672 -
Flags: review?(cam) → review+
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8828673 [details] Bug 1328787 - Part 10: Set Keyframe.mPropertyValues for the case where keyframe is not specified. https://reviewboard.mozilla.org/r/105982/#review106984 ::: servo/ports/geckolib/glue.rs:1146 (Diff revision 1) > match step.value { > KeyframesStepValue::ComputedValues => { > - unimplemented!(); > + for (index, property) in animation.properties_changed.iter().enumerate() { > + let block = style.to_declaration_block(property.clone().into()); > + unsafe { > + (*keyframe).mPropertyValues.set_len((index+1) as u32); Nit: spaces around +.
Attachment #8828673 -
Flags: review?(cam) → review+
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8828674 [details] Bug 1328787 - Part 11: The last keyframe's KeyframePercentage is 1.0. https://reviewboard.mozilla.org/r/105984/#review106986
Attachment #8828674 -
Flags: review?(cam) → review+
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8828675 [details] Bug 1328787 - Part 12: Fill Gecko's Keyframe. https://reviewboard.mozilla.org/r/105986/#review106988 ::: layout/style/nsAnimationManager.cpp:606 (Diff revision 1) > + ServoStyleSet* styleSet = aPresContext->StyleSet()->GetAsServo(); > + MOZ_ASSERT(styleSet); Just do aPresContext->StyleSet()->AsServo()->... below.
Attachment #8828675 -
Flags: review?(cam) → review+
Assignee | ||
Comment 30•7 years ago
|
||
Thanks for the review! (In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #16) > ::: layout/base/ServoRestyleManager.cpp:44 > (Diff revision 1) > > - // We allow posting change hints during restyling, but not restyle hints > > - // themselves, since those would require us to re-traverse the tree. > > - MOZ_ASSERT_IF(mInStyleRefresh, aRestyleHint == 0); > > + // We allow posting change hints without any restyles hints during restyling, > > + // since those would require us to re-traverse the tree, and allow animation > > + // only restyles without any change hints. > > + MOZ_ASSERT_IF(mInStyleRefresh, aRestyleHint == 0 || > > + (aRestyleHint == eRestyle_CSSAnimations && > > + !aMinChangeHint)); > > Hmm, so the problem here is that any restyle hints that are generated from > UpdateAnimations() won't get handled, and actually we can't really handle > them properly under ServoStyleSet::RecreateStyleContexts, since we have > already restyled the entire tree, and we're just creating nsStyleContexts. > The assertion here was checking that we only generate change hints, since > they don't require a re-traversal of the tree, which handling a restyle hint > would. Oh, I was totally misunderstanding stylo's styling mechanism. > I don't recall what the plan was to handle this. The worst case would be to > just do another whole tree traversal later to handle the > eRestyle_CSSAnimations restyle hint, potentially wasting a lot of restyle > work done on descendants of that element. Did we have a plan we discussed > in Hawaii, where we would do something like: stop the Servo restyle at an > element, do the animation updating back in > ServoStyleSet::RecreateStyleContexts, then return to Servo to restyle the > element and its descendants? Or am I misremembering a different > conversation? As far as I remember, there was nothing about CSS animations. > ::: layout/style/ServoStyleSet.cpp:144 > (Diff revision 1) > > + // Update/build CSS animations in the case where animation properties are > > + // changed. > > + mPresContext->AnimationManager()->UpdateAnimations(result, element); > > I think this should be in the other GetContext one, since that's the deepest > GetContext call (and matches where we do it in nsStyleSet.) Thanks. Will do. > Some questions about this: > > 1. nsStyleSet::GetContext checks |PresContext()->IsDynamic() && > aElementForAnimation->IsInComposedDoc()| to see if it should skip animation > rule updating. Should we do that here too? Yes, I will do it. > 2. Can you add some comments in here saying that we need to return a new > style context in case the UpdateAnimations call cause changes to animations? > I assume we'll do that work in a later bug. Thanks, now I understood what we should do here.
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8828676 [details] Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations(). https://reviewboard.mozilla.org/r/105988/#review106954 > Hmm, so the problem here is that any restyle hints that are generated from UpdateAnimations() won't get handled, and actually we can't really handle them properly under ServoStyleSet::RecreateStyleContexts, since we have already restyled the entire tree, and we're just creating nsStyleContexts. The assertion here was checking that we only generate change hints, since they don't require a re-traversal of the tree, which handling a restyle hint would. > > I don't recall what the plan was to handle this. The worst case would be to just do another whole tree traversal later to handle the eRestyle_CSSAnimations restyle hint, potentially wasting a lot of restyle work done on descendants of that element. Did we have a plan we discussed in Hawaii, where we would do something like: stop the Servo restyle at an element, do the animation updating back in ServoStyleSet::RecreateStyleContexts, then return to Servo to restyle the element and its descendants? Or am I misremembering a different conversation? It turns out that the eRestyle_CSSAnimations is caused by a bug 1332958 that now I filed. I will fix that bug first.
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 45•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #20) > ::: servo/ports/geckolib/glue.rs:1115 > (Diff revision 1) > > + let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut(); > > + let device = data.stylist.device.clone(); > > + let name = unsafe { name.as_ref().unwrap().as_str_unchecked() }; > > + let style = ComputedValues::as_arc(&style); > > + > > + let timing_function = style.get_box().animation_timing_function_mod(0); > > I don't understand why we just look at the first animation-timing-function > value. Don't we want to look at the index the the |name| was found at in > animation-name? Thanks, yes, right. Fixed. To use the correct timing function (that matches to the index of the |name|), I did pass const* nsTimingFunction to Servo_StyleSet_FillKeyframesForName. We can't get the correct timing function with the index in servo side, since there is a case that we have the same animation name but different timing functions. E.g. style.animation = "anim1 ease; anim1 linear"; Whereas in case of animation-timing-function in @keyframes: > ::: servo/components/style/keyframes.rs:218 > (Diff revision 1) > > + // Use the first value. > > + Some(value.0[0]) > > Again, why the first value? This is intentional, because we use the first function in Gecko [1]. [1] https://hg.mozilla.org/mozilla-central/file/bd0cd9af94d9/layout/style/nsAnimationManager.cpp#l854
Assignee | ||
Comment 46•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #16) > 2. Can you add some comments in here saying that we need to return a new > style context in case the UpdateAnimations call cause changes to animations? > I assume we'll do that work in a later bug. If I understand correctly what you mean here, I think we don't need this comment if there is no extra eRestyle_CSSAnimations once bug 1332958 is fixed. Right? Let me know if I am still misunderstanding. Thanks!
Comment 47•7 years ago
|
||
mozreview-review |
Comment on attachment 8828668 [details] Bug 1328787 - Part 5: Implement Servo_StyleSet_FillKeyframesForName. https://reviewboard.mozilla.org/r/105972/#review107352 r=me with that fixed. ::: servo/ports/geckolib/glue.rs:1119 (Diff revision 2) > + let device = data.stylist.device.as_ref(); > + let name = unsafe { name.as_ref().unwrap().as_str_unchecked() }; > + let style_timing_function = unsafe { timing_function.as_ref().unwrap() }; > + > + fn find_animation(rules: &[CssRule], device: &Device, name: &str) -> Option<KeyframesAnimation> { > + for rule in rules { Don't we need to go through |rules| in reverse too?
Attachment #8828668 -
Flags: review?(cam) → review+
Comment 48•7 years ago
|
||
mozreview-review |
Comment on attachment 8828676 [details] Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations(). https://reviewboard.mozilla.org/r/105988/#review107368 ::: layout/style/ServoStyleSet.cpp:167 (Diff revision 2) > + mPresContext->AnimationManager()->UpdateAnimations(result, > + aElementForAnimation); Please add a comment in here saying that this probably isn't the right place to be calling UpdateAnimations, due to the issues I mentioned in the previous review (i.e., this can post restyle hints, which we can't handle during ServoRestyleManager::RecreateStyleContetxs). Actually, why do we no longer hit the assertion about restyle hints being generated during RecreateStyleContexts? ::: layout/style/ServoStyleSet.cpp:246 (Diff revision 2) > - return GetContext(computedValues.forget(), aParentContext, pseudoTag, aType); > + bool isBeforeOrAfter = (aType == CSSPseudoElementType::before) || > + (aType == CSSPseudoElementType::after); Nit: don't need the parens around these. ::: layout/style/ServoStyleSet.cpp:484 (Diff revision 2) > - if (computedValues && > - (pseudoTag == nsCSSPseudoElements::before || > + bool isBeforeOrAfter = (pseudoTag == nsCSSPseudoElements::before || > + pseudoTag == nsCSSPseudoElements::after); Nit: here too.
Comment 49•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8828676 [details] Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations(). https://reviewboard.mozilla.org/r/105988/#review107368 > Please add a comment in here saying that this probably isn't the right place to be calling UpdateAnimations, due to the issues I mentioned in the previous review (i.e., this can post restyle hints, which we can't handle during ServoRestyleManager::RecreateStyleContetxs). > > Actually, why do we no longer hit the assertion about restyle hints being generated during RecreateStyleContexts? I see your comment about bug 1332958. Does that mean we will never generate eRestyle_CSSAnimations during the RecreateStyleContexts call?
Assignee | ||
Comment 50•7 years ago
|
||
Comment on attachment 8828676 [details] Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations(). (In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #49) > Comment on attachment 8828676 [details] > Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations(). > > https://reviewboard.mozilla.org/r/105988/#review107368 > > > Please add a comment in here saying that this probably isn't the right place to be calling UpdateAnimations, due to the issues I mentioned in the previous review (i.e., this can post restyle hints, which we can't handle during ServoRestyleManager::RecreateStyleContetxs). > > > > Actually, why do we no longer hit the assertion about restyle hints being generated during RecreateStyleContexts? > > I see your comment about bug 1332958. Does that mean we will never generate > eRestyle_CSSAnimations during the RecreateStyleContexts call? I was wrong. We still see another eRestyle_CSSAnimations during the RecreateStyleContexts call. I need to figure out a machinery to process the eRestyle_CSSAnimations.
Attachment #8828676 -
Flags: review?(cam)
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 64•7 years ago
|
||
mozreview-review |
Comment on attachment 8828668 [details] Bug 1328787 - Part 5: Implement Servo_StyleSet_FillKeyframesForName. https://reviewboard.mozilla.org/r/105972/#review108808 ::: servo/ports/geckolib/glue.rs:1155 (Diff revision 3) > + if let Some(ref animation) = data.stylist.animations().get(&name) { > + for step in &animation.steps { > + let timing_function = *style_timing_function; As Cameron taught me, I did use stylist.animations() here instead of traversing stylesheets.
Assignee | ||
Comment 65•7 years ago
|
||
(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #48) > ::: layout/style/ServoStyleSet.cpp:167 > (Diff revision 2) > > + mPresContext->AnimationManager()->UpdateAnimations(result, > > + aElementForAnimation); > > Please add a comment in here saying that this probably isn't the right place > to be calling UpdateAnimations, due to the issues I mentioned in the > previous review (i.e., this can post restyle hints, which we can't handle > during ServoRestyleManager::RecreateStyleContetxs). After some more thought, we can't avoid the recursive eRestyle_CSSAnimations for now. So, I just skipped it in ServoRestyleManager::PostRestyleEvent(). Because, I think cascade_node() in servo side (or other functions around this function) is a properer place to update css animations, but to do that we need to make UpdateAnimations (and relevant functions) independent from nsStyleContext and also make those processes thread safe. I am guessing that we should write equivalent code in servo side instead of refactoring those processes.
Comment 66•7 years ago
|
||
mozreview-review |
Comment on attachment 8828676 [details] Bug 1328787 - Part 13: Call nsAnimationManager.UpdateAnimations(). https://reviewboard.mozilla.org/r/105988/#review108822
Attachment #8828676 -
Flags: review?(cam) → review+
Assignee | ||
Comment 67•7 years ago
|
||
Thank you Cameron for reviewing! Try on mozilla-central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a3944c15d48d67a0240789e24a6dc5071a447c2 Try on stylo: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b01e102ec6ecaa111588ec0f58f4e5ecd65d45f&exclusion_profile=false Crashtest is orange but all of failures are not related to these changes. For example there are two TEST-UNEXPECTED-PASSed tests in dom/animation but they have been already passed (See bug 1324691 comment 14). For comparison here is another try on stylo without these patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a922db3616844c55569b08e60ce6a870dbf8b2e3
Comment 68•7 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/31a6ffc60945 Part 1: Factor out Keyframe and PropertyValuePair into Keyframe.h. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/f6330abee716 Part 2: Don't pass nsCSSKeyframesRule* to CSSAnimationBuild::Build. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/70432998700d Part 3: Add functions for filling each servo's animation keyframes into nsTArray<Keyframe>. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/2162612b5857 Part 4: Add Gecko_AnimationAppendKeyframe. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/224a64d93324 Part 12: Fill Gecko's Keyframe. r=heycam https://hg.mozilla.org/integration/mozilla-inbound/rev/2fe14ee4e5a8 Part 13: Call nsAnimationManager.UpdateAnimations(). r=heycam
Assignee | ||
Comment 69•7 years ago
|
||
Servo PR: https://github.com/servo/servo/pull/15287
Comment 70•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/31a6ffc60945 https://hg.mozilla.org/mozilla-central/rev/f6330abee716 https://hg.mozilla.org/mozilla-central/rev/70432998700d https://hg.mozilla.org/mozilla-central/rev/2162612b5857 https://hg.mozilla.org/mozilla-central/rev/224a64d93324 https://hg.mozilla.org/mozilla-central/rev/2fe14ee4e5a8
Status: ASSIGNED → RESOLVED
Closed: 7 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
•