Closed Bug 1340322 Opened 3 years ago Closed 3 years ago

stylo: Update CSS animations with servo's computed values instead of nsStyleContext

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(14 files, 1 obsolete file)

59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
26.55 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
birtles
: review+
Details
59 bytes, text/x-review-board-request
birtles
: review+
Details
In this bug I am going to rewrite nsAnimationManager::UpdateAnimations() with servo's computed values (actually I will add another variant of UpdateAnimations()).
One this finished up, we can call the UpdateAnimations during StyleDocument() except handling nsAutoAnimationMutationBatch.
Rough flow is like this:

1. Call nsAnimationManager::UpdateAnimations() with servo's computed values from ServoStyleSet::GetContext() 
  * this is tentative, we will call the UpdateAnimations() from servo side (in bug 1335938).
2. In nsAnimationManager::UpdateAnimations()
  2-1. Set up nsAutoAnimationMutationBatch
  2-2. Set up ServoCSSAnimationBuilder which inherits CSSAnimationBuilder.
  2-3. Call Servo side UpdateCSSAnimations() function with the ServoCSSAnimationBuilder.
3. In Servo_UpdateCSSAnimations()
  3-1. Iterate box style animation names
  3-2. If we have the same name in keyframe rules, then
   3-2-1. Generate nsTArray<Keyframe> from the rule.
   3-2-2. Call Gecko side UpdateCSSAnimation() with the ServoCSSAnimationBuilder.
4. In UpdateCSSAnimation()
 4-1. Do almost the same thing what we do CSSAnimationBuilder::Build().

Note that ServoCSSAnimationBuilder has servo's computed values instead of nsStyleContext.
I'm really unfamiliar with the setup but I somehow imagined that we would accumulate new animations while restyling in parallel in Servo and then apply them as part of the second (non-parallel) phase in Gecko?
Yeah, currently we do something similar.

Let me clarify what you mean the first and the second phase.

In current stylo
- the first phase:
  apply css animation properties (e.g. duration, delay, etc.)
- the second phase: (Generating nsStyleContext in Gecko, I guess this is the second phase what you meant.)
  create CSSAnimation object but the initial styles are not applied yet, since in Gecko (IIUC) this can be done by PostRestyle(eRestyle_CSSAnimations) but stylo does not allow eRestyle_CSSAnimations in this phase.
(In reply to Brian Birtles (:birtles) from comment #2)
> I'm really unfamiliar with the setup but I somehow imagined that we would
> accumulate new animations while restyling in parallel in Servo and then
> apply them as part of the second (non-parallel) phase in Gecko?

I think in this way there are some cases that we can't get correct animating values on child element. For example:

@keyframes anim {
  from { color: red; }
  to { color: blue; }
}

<div id="parent">
 <div id="child"></div>
</div>

parent.style.animation = "anim 1s";
getComptuedStyle(child).color;  

From my understandings, we have to compute the animating color in the first phase and compute the child element styles with this color value in the *first* phase.
Yes, I believe you're right. I'm currently trying to understand exactly how this should work since there are some interop issues in this area. I believe Blink accumulates all animation and transition changes for an element in a stack-based RAII class and then applies them at once (presumably applying them in the appropriate order, e.g. font-size first). We might need to take a similar approach to avoid new CSS Animations / finishing CSS animations from triggering transitions. I'm looking into this in bug 1192592 but it will take a little longer yet.
Blocks: 1341985
In this bug I am going to introduce two new classes inheriting CSSAnimationBuilder.

* GeckoCSSAnimationBuilder: generates keyframes from nsStyleContext*
* ServoCSSAnimationBuilder: generates keyframes from servo's computed values 

I will also introduce another variant of nsAnimationManager::UpdateAnimation() for servo's computed values.
Comment on attachment 8841459 [details]
Bug 1340322 - Part 12: Add another variant of nsAnimationManager::UpdateAnimations for stylo.

https://reviewboard.mozilla.org/r/115684/#review117048

::: servo/ports/geckolib/glue.rs:1532
(Diff revision 1)
> +pub extern "C" fn Servo_GetStyleAnimations(style: ServoComputedValuesBorrowed)
> +    -> RawGeckoStyleAnimationListBorrowed {
> +    let style = ComputedValues::as_arc(&style);
> +    let box_style = style.get_box();
> +
> +    &box_style.style_animations()
> +}

Here I'd like to add an FFI to get 'nsStyleAutoArray<mozilla::StyleAnimation>' from computed values
(Honestly I'd like to get const reference though).

But this causes a lifetime problem:

0:02.10 error: borrowed value does not live long enough
0:02.10     --> /home/ikezoe/stylo/servo/ports/geckolib/glue.rs:1537:6
0:02.10      |
0:02.10 1537 |     &box_style.style_animations()
0:02.10      |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ does not live long enough
0:02.10 1538 | }
0:02.10      | - temporary value only lives until here
0:02.10      |
0:02.10 note: borrowed value must be valid for the lifetime 'a as defined on the body at 1533:50...
0:02.10     --> /home/ikezoe/stylo/servo/ports/geckolib/glue.rs:1533:51
0:02.10      |
0:02.10 1533 |       -> &'a RawGeckoStyleAnimationListBorrowed<'a> {
0:02.10      |  ___________________________________________________^ starting here...
0:02.10 1534 | |     let style = ComputedValues::as_arc(&style);
0:02.10 1535 | |     let box_style = style.get_box();
0:02.10 1536 | |
0:02.10 1537 | |     &box_style.style_animations()
0:02.10 1538 | | }
0:02.10      | |_^ ...ending here

I think I am on the wrong track. How can we implement such function?
Manish, any hints about this?
Flags: needinfo?(manishearth)
This will fix it for you. Please r?mystor or emilio on the resultant patch.


diff --git a/servo/components/style/gecko_bindings/sugar/ownership.rs b/servo/components/style/gecko_bindings/sugar/ownership.rs
index abcf5c6..5f99a68 100644
--- a/servo/components/style/gecko_bindings/sugar/ownership.rs
+++ b/servo/components/style/gecko_bindings/sugar/ownership.rs
@@ -138,6 +138,19 @@ pub unsafe trait HasArcFFI : HasFFI {
             }
         }
     }
+
+    #[inline]
+    /// Converts a borrowed FFI reference to a borrowed regular reference
+    ///
+    /// GeckoType -> &ServoType
+    fn as_ref<'a>(ptr: &'a Self::FFIType) -> &'a Self {
+        let arc = Self::as_arc(&ptr);
+        // Lengthen the lifetime. While the &Arc is only valid for as long as the
+        // double-ref passed to as_arc, the inner value is valid for at least as
+        // long as 'a, so we can do this.
+        let ponce_de_arc: &'a Arc<_> = unsafe { transmute::<&Arc<_>, &Arc<_>>(arc) };
+        return &*ponce_de_arc
+    }
 }
 
 #[repr(C)]
diff --git a/servo/ports/geckolib/glue.rs b/servo/ports/geckolib/glue.rs
index e0a83fa..9a356a1 100644
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -1531,7 +1531,7 @@ pub extern "C" fn Servo_GetStyleAnimationNameCount(style: ServoComputedValuesBor
 #[no_mangle]
 pub extern "C" fn Servo_GetStyleAnimations(style: ServoComputedValuesBorrowed)
     -> RawGeckoStyleAnimationListBorrowed {
-    let style = ComputedValues::as_arc(&style);
+    let style = ComputedValues::as_ref(style);
     let box_style = style.get_box();
 
     &box_style.style_animations()
Flags: needinfo?(manishearth)
Thank you Manish! Amazing!
I had a quick look at this and the things that I'm not sure about so far are:

a) Why do we use inheritance for CSSAnimationBuilder? It seems like it's just for sharing utility code and we should use composition here instead. It would be nice not to add more virtual function calls in here too. (And in general it seems like CSSAnimationBuilder is doing more than just building now, so perhaps we should be splitting off another object, rather than cramming more functionality into CSSAnimationBuilder.)

b) I wonder if we need to pass nsAnimationManager to CSSAnimationBuilder in part 4. That feels odd and makes me wonder if we should divide up the problem differently.

c) The destructor for CSSAnimationBuilder added here is quite heavy. It would be better to avoid that if possible since it's easy to get that wrong (and we should definitely be declaring the dtors of the derived classes virtual).

I'll have a deeper look from here on but I mention those things in case there's something I'm misunderstanding about this patch series.
Yeah, your feeling is quite correct. As for (b) and (c), it's some kind of the remnants when we had not yet decided to use SequentialTask to update CSS animations.  At that time, I separate processes that are not thread safe to do such processes in safety somewhere.  But now bug 1341518 has been fixed, I think now we can split the process in the dtor can be a function of nsAnimationManager, and call the function in UpdateAnimations() for both of Gecko and Stylo. But still, if the process is in the dtor, we don't need to call it respectively.
Blocks: 1343165
I had a look into the concerns I raised in comment 34 and I'm pretty sure we can do this more cleanly. This patch shows an alternative refactoring that should at least build on Windows.

The downside of this approach is marginally more generated code due to use of templates (which will go away when we drop the non-servo code path) but otherwise I think it's a smaller and simpler change. It also avoids adding virtual method calls and I think it's easier to follow since we don't do the real work in the (base class) dtor.
Just to clarify, attachment 8842300 [details] [diff] [review] is supposed to cover roughly parts 1-9; parts 10-12 are still be needed in order to build. And it's still probably worth splitting up further for reviewing. It's just supposed to demonstrate another approach.
I should have not assigned this bug to me. :-) As for display:none subtree, as of this bug, we don't seem to call UpdateAnimations().  We call UpdateAnimations with null computed values for elements in display:none subtree in bug 1341985.
You did all the work -- I just tweaked your patches to see if another approach would work!
I should also note that we have to do similar thing (i.e. generating keyframes from computed values) for transition.  I think there is no more stuff that can be shared with, but if you notice something, please let me know!
See Also: → 1343753
Comment on attachment 8841450 [details]
Bug 1340322 - Part 1: Split CSSAnimationBuilder::Build off as a static function.

https://reviewboard.mozilla.org/r/115666/#review117658

Clearing review here as per comment 34.
Attachment #8841450 - Flags: review?(bbirtles)
Attachment #8841451 - Flags: review?(bbirtles)
Attachment #8841452 - Flags: review?(bbirtles)
Attachment #8841453 - Flags: review?(bbirtles)
Attachment #8841454 - Flags: review?(bbirtles)
Attachment #8841455 - Flags: review?(bbirtles)
Attachment #8841456 - Flags: review?(bbirtles)
Attachment #8841457 - Flags: review?(bbirtles)
Attachment #8841458 - Flags: review?(bbirtles)
Comment on attachment 8841798 [details]
Bug 1340322 - Part 12: Split nsStyleAutoArray into a new header to avoid including nsStyleStruct.h in ServoBindingTypes.h.

https://reviewboard.mozilla.org/r/115902/#review118108
Attachment #8841798 - Flags: review?(bbirtles) → review+
Comment on attachment 8841459 [details]
Bug 1340322 - Part 12: Add another variant of nsAnimationManager::UpdateAnimations for stylo.

https://reviewboard.mozilla.org/r/115684/#review118110

Clearing my review on this too since I think it will change based on the suggested changes to underlying parts.

(I think Emilio would be fine to go ahead and review the Servo/bindings parts though since I assume they won't change.)
Attachment #8841459 - Flags: review?(bbirtles)
Comment on attachment 8841460 [details]
Bug 1340322 - Part 13: Update CSS Animations with servo's computed values.

https://reviewboard.mozilla.org/r/115686/#review118112

This too, I guess will need to be updated.
Attachment #8841460 - Flags: review?(bbirtles)
Comment on attachment 8841459 [details]
Bug 1340322 - Part 12: Add another variant of nsAnimationManager::UpdateAnimations for stylo.

https://reviewboard.mozilla.org/r/115684/#review118224

Clearing review for now based on the above discussion, but feel free to re-request when needed :)

::: servo/ports/geckolib/glue.rs:1539
(Diff revision 2)
> +pub extern "C" fn Servo_GetStyleAnimations(style: ServoComputedValuesBorrowed)
> +    -> RawGeckoStyleAnimationListBorrowed {
> +    let style = ComputedValues::as_ref(&style);
> +    let box_style = style.get_box();
> +
> +    &box_style.style_animations()

Isn't this taking a reference to a temporary? I'm unsure why you need the as_ref thing tbh, RawGeckoStyleAnimationListBorrowed should just be a pointer right?

I _think_ this works because rust autodereferences it for you, but still seems to me that just returning box_style.style_animations() (without the `&`) should suffice?
Attachment #8841459 - Flags: review?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #45)

> ::: servo/ports/geckolib/glue.rs:1539
> (Diff revision 2)
> > +pub extern "C" fn Servo_GetStyleAnimations(style: ServoComputedValuesBorrowed)
> > +    -> RawGeckoStyleAnimationListBorrowed {
> > +    let style = ComputedValues::as_ref(&style);
> > +    let box_style = style.get_box();
> > +
> > +    &box_style.style_animations()
> 
> Isn't this taking a reference to a temporary? I'm unsure why you need the
> as_ref thing tbh, RawGeckoStyleAnimationListBorrowed should just be a
> pointer right?
> 
> I _think_ this works because rust autodereferences it for you, but still
> seems to me that just returning box_style.style_animations() (without the
> `&`) should suffice?

Thanks! I don't remember why I put the '&' there...
I will update patches after landing bug 1339332.
Thanks!
Comment on attachment 8841459 [details]
Bug 1340322 - Part 12: Add another variant of nsAnimationManager::UpdateAnimations for stylo.

https://reviewboard.mozilla.org/r/115684/#review118756

::: servo/ports/geckolib/glue.rs:1526
(Diff revision 3)
>      }
>      false
>  }
>  
> +#[no_mangle]
> +pub extern "C" fn Servo_GetStyleAnimationNameCount(style: ServoComputedValuesBorrowed)

Also, why all these one-off functions instead of using `Servo_GetStyleBox` directly?

::: servo/ports/geckolib/glue.rs:1531
(Diff revision 3)
> +pub extern "C" fn Servo_GetStyleAnimationNameCount(style: ServoComputedValuesBorrowed)
> +    -> u32 {
> +    let style = ComputedValues::as_arc(&style);
> +    let box_style = style.get_box();
> +
> +    box_style.animation_name_count() as u32

I'm curious about this, no action needed... Why does gecko always use at least one for mAnimationNameCount? Is it because of a particular reason, or just convenience when serializing and similar?

Because it is definitely something somewhat annoying for Servo (Xidorn aligned Servo to Gecko here, but I'm wondering why did he do it that way around and not the other)

::: servo/ports/geckolib/glue.rs:1537
(Diff revision 3)
> +}
> +
> +#[no_mangle]
> +pub extern "C" fn Servo_GetStyleAnimations(style: ServoComputedValuesBorrowed)
> +    -> RawGeckoStyleAnimationListBorrowed {
> +    let style = ComputedValues::as_ref(&style);

Are you totally sure the `as_ref` is needed? I don't think it should be now, right?
Attachment #8841459 - Flags: review?(emilio+bugs)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #45)
> ::: servo/ports/geckolib/glue.rs:1539
> (Diff revision 2)
> > +pub extern "C" fn Servo_GetStyleAnimations(style: ServoComputedValuesBorrowed)
> > +    -> RawGeckoStyleAnimationListBorrowed {
> > +    let style = ComputedValues::as_ref(&style);
> > +    let box_style = style.get_box();
> > +
> > +    &box_style.style_animations()
> 
> Isn't this taking a reference to a temporary? I'm unsure why you need the
> as_ref thing tbh, RawGeckoStyleAnimationListBorrowed should just be a
> pointer right?

Yeah, I thought.

In struct_debug.rs:

    pub type RawGeckoStyleAnimationList =                                                                               
        root::nsStyleAutoArray<root::mozilla::StyleAnimation>;
    pub type RawGeckoStyleAnimationListBorrowed =                                                                       
        *const root::RawGeckoStyleAnimationList;

But in bindings.rs:

    pub type RawGeckoStyleAnimationListBorrowed<'a> = &'a RawGeckoStyleAnimationList;

How can we solve this?
Flags: needinfo?(emilio+bugs)
I don't think we're supposed to return references from FFI functions. Manish wrote that code, perhaps he has a different oppinion?
Flags: needinfo?(emilio+bugs) → needinfo?(manishearth)
We're supposed to ignore the FooBarBorrowed types in structs_*.rs


It's okay to return references from FFI. The as_ref is needed; there's otherwise no way to get a reference to the contents of the arc with the same lifetime as the original passed-in reference to the arc.
Flags: needinfo?(manishearth)
Thank you Manish and Emilio!

(In reply to Emilio Cobos Álvarez [:emilio] from comment #61)
> ::: servo/ports/geckolib/glue.rs:1526
> (Diff revision 3)
> >      }
> >      false
> >  }
> >  
> > +#[no_mangle]
> > +pub extern "C" fn Servo_GetStyleAnimationNameCount(style: ServoComputedValuesBorrowed)
> 
> Also, why all these one-off functions instead of using `Servo_GetStyleBox`
> directly?

Now I understand this comment. We have already Servo_GetStyleDisplay which returns const nsStyleDisplay*. 
We can use this function instead of adding two new FFIs. 

Thank you both!
Attachment #8841798 - Attachment is obsolete: true
Comment on attachment 8841450 [details]
Bug 1340322 - Part 1: Split CSSAnimationBuilder::Build off as a static function.

https://reviewboard.mozilla.org/r/115666/#review118980
Attachment #8841450 - Flags: review?(bbirtles) → review+
Comment on attachment 8841451 [details]
Bug 1340322 - Part 2: Use NonOwningAnimationTarget instead of Element and aStyleContext()->GetPseudoType().

https://reviewboard.mozilla.org/r/115668/#review118978

::: layout/style/nsAnimationManager.cpp:572
(Diff revision 4)
>                        nsTArray<PropertyValuePair>& aPropertyValues);
>    nsCSSValue GetComputedValue(nsPresContext* aPresContext,
>                                nsCSSPropertyID aProperty);
>  
>    RefPtr<nsStyleContext> mStyleContext;
> -  RefPtr<dom::Element> mTarget;
> +  const NonOwningAnimationTarget& mTarget;

It seems like it would be better to just make this a NonOwningAnimationTarget (i.e. copy the object)? That way it's safe to use even if the passed-in reference changes (but not if the Element itself is destroyed)?
Attachment #8841451 - Flags: review?(bbirtles) → review+
Comment on attachment 8841452 [details]
Bug 1340322 - Part 3: Make BuildAnimations static function.

https://reviewboard.mozilla.org/r/115670/#review118982

::: layout/style/nsAnimationManager.h:357
(Diff revision 4)
>  private:
>  
> -  void BuildAnimations(nsStyleContext* aStyleContext,
> -                       const mozilla::NonOwningAnimationTarget& aTarget,
> -                       CSSAnimationCollection* aCollection,
> -                       OwningCSSAnimationPtrArray& aAnimations);
> -
>    mozilla::DelayedEventDispatcher<mozilla::AnimationEventInfo> mEventDispatcher;

Nit: Drop the blank line after 'private'?
Attachment #8841452 - Flags: review?(bbirtles) → review+
Comment on attachment 8841453 [details]
Bug 1340322 - Part 4: Make BuildAnimations() independent from nsStyleDisplay .

https://reviewboard.mozilla.org/r/115672/#review118984
Attachment #8841453 - Flags: review?(bbirtles) → review+
Comment on attachment 8841454 [details]
Bug 1340322 - Part 5: Split off BuildKeyframes()

https://reviewboard.mozilla.org/r/115674/#review118986
Attachment #8841454 - Flags: review?(bbirtles) → review+
Comment on attachment 8841455 [details]
Bug 1340322 - Part 6: Add CSSAnimationBuilder::SetKeyframes().

https://reviewboard.mozilla.org/r/115676/#review118988
Attachment #8841455 - Flags: review?(bbirtles) → review+
Comment on attachment 8841456 [details]
Bug 1340322 - Part 7: Make BuildAnimation() and BuildAnimations() independent from nsStyleContext.

https://reviewboard.mozilla.org/r/115678/#review118990
Attachment #8841456 - Flags: review?(bbirtles) → review+
Comment on attachment 8841457 [details]
Bug 1340322 - Part 8: Split off some processes that will be used for servo's computed values in UpdateAnimations().

https://reviewboard.mozilla.org/r/115680/#review118994

::: layout/style/nsAnimationManager.h:359
(Diff revision 4)
>  private:
>  
> +  void DoUpdateAnimations(
> +    const mozilla::NonOwningAnimationTarget& aTarget,
> +    const nsStyleDisplay& aStyleDisplay,
> +    CSSAnimationBuilder& aBuilder);

Nit: Drop the blank line between 'private:' and DoUpdateAnimations

::: layout/style/nsAnimationManager.cpp:1092
(Diff revision 4)
> +  DoUpdateAnimations(target, *disp, builder);
> +}
> +
> +void
> +nsAnimationManager::DoUpdateAnimations(
> +  const mozilla::NonOwningAnimationTarget& aTarget,

Nit: I don't think we need 'mozilla::' here since we have 'using namespace mozilla' at the top of this file.
Attachment #8841457 - Flags: review?(bbirtles) → review+
Comment on attachment 8841458 [details]
Bug 1340322 - Part 9: Templatize functions that will be used with servo's computed values.

https://reviewboard.mozilla.org/r/115682/#review118996
Attachment #8841458 - Flags: review?(bbirtles) → review+
Comment on attachment 8843160 [details]
Bug 1340322 - Part 10: Rename CSSAnimationBuilder to GeckoCSSAnimationBuilder.

https://reviewboard.mozilla.org/r/116918/#review118998
Attachment #8843160 - Flags: review?(bbirtles) → review+
Comment on attachment 8843160 [details]
Bug 1340322 - Part 10: Rename CSSAnimationBuilder to GeckoCSSAnimationBuilder.

https://reviewboard.mozilla.org/r/116918/#review119000

The commit message should be updated to have s/GeckoAnimationBuilder/GeckoCSSAnimationBuilder/
Comment on attachment 8843161 [details]
Bug 1340322 - Part 11: Introduce ServoCSSAnimationBuilder.

https://reviewboard.mozilla.org/r/116920/#review119002
Attachment #8843161 - Flags: review?(bbirtles) → review+
Comment on attachment 8841459 [details]
Bug 1340322 - Part 12: Add another variant of nsAnimationManager::UpdateAnimations for stylo.

https://reviewboard.mozilla.org/r/115684/#review119004

::: layout/style/nsAnimationManager.cpp:1130
(Diff revision 4)
>    DoUpdateAnimations(target, *disp, builder);
>  }
>  
> +void
> +nsAnimationManager::UpdateAnimations(
> +  mozilla::dom::Element* aElement,

Again, I don't think we need mozilla:: here

::: layout/style/nsAnimationManager.cpp:1147
(Diff revision 4)
> +  // Currently we don't seem to call this UpdateAnimations for elements in
> +  // display:none subtree. We will call this function for such elements with
> +  // null computed values in bug 1341985.

Is it possible to add an assertion here that we're not in a display:none subtree? I guess not?

::: layout/style/nsAnimationManager.cpp:1150
(Diff revision 4)
> +  ServoCSSAnimationBuilder builder(aComputedValues, aParentComputedValues);
> +
> +  // Currently we don't seem to call this UpdateAnimations for elements in
> +  // display:none subtree. We will call this function for such elements with
> +  // null computed values in bug 1341985.
> +  const nsStyleDisplay *disp = Servo_GetStyleDisplay(aComputedValues);

(It looks like this is assumed to be non-null elsewhere in our codebase. I wonder why we have 'Get' in the name if it can't return false.)
Attachment #8841459 - Flags: review?(bbirtles) → review+
Comment on attachment 8841460 [details]
Bug 1340322 - Part 13: Update CSS Animations with servo's computed values.

https://reviewboard.mozilla.org/r/115686/#review119006

::: layout/style/ServoStyleSet.cpp:172
(Diff revision 4)
>        aElementForAnimation &&
>        aElementForAnimation->IsInComposedDoc()) {
>      // Update/build CSS animations in the case where animation properties are
>      // changed.
> -    // FIXME: This isn't right place to update CSS animations. We should do it
> -    // , presumably, in cascade_node() in servo side and process the initial
> +    // FIXME: Bug 1341985: This isn't right place to update CSS animations.
> +    // We should do it in a SequentialTask and kick the second traversal for

(Nit: s/kick/trigger/)

::: layout/style/nsAnimationManager.cpp:642
(Diff revision 4)
>  bool
>  GeckoCSSAnimationBuilder::BuildKeyframes(nsPresContext* aPresContext,
>                                           const StyleAnimation& aSrc,
>                                           nsTArray<Keyframe>& aKeyframes)
>  {
>    MOZ_ASSERT(aPresContext);

Should we be adding MOZ_ASSERT(aPresContext->StyleSet()->IsGecko()) here?
Attachment #8841460 - Flags: review?(bbirtles) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #61)
> ::: servo/ports/geckolib/glue.rs:1531
> (Diff revision 3)
> > +pub extern "C" fn Servo_GetStyleAnimationNameCount(style: ServoComputedValuesBorrowed)
> > +    -> u32 {
> > +    let style = ComputedValues::as_arc(&style);
> > +    let box_style = style.get_box();
> > +
> > +    box_style.animation_name_count() as u32
> 
> I'm curious about this, no action needed... Why does gecko always use at
> least one for mAnimationNameCount? Is it because of a particular reason, or
> just convenience when serializing and similar?
> 
> Because it is definitely something somewhat annoying for Servo (Xidorn
> aligned Servo to Gecko here, but I'm wondering why did he do it that way
> around and not the other)

It's not a Gecko thing, but a spec thing. The default value for animation-name is a list with the single value 'none'.

Do we do something different for background-image?
(In reply to Brian Birtles (:birtles) from comment #91)
> Comment on attachment 8841459 [details]
> Bug 1340322 - Part 12: Add another variant of
> nsAnimationManager::UpdateAnimations for stylo.
> 
> https://reviewboard.mozilla.org/r/115684/#review119004
> 
> ::: layout/style/nsAnimationManager.cpp:1130
> (Diff revision 4)
> >    DoUpdateAnimations(target, *disp, builder);
> >  }
> >  
> > +void
> > +nsAnimationManager::UpdateAnimations(
> > +  mozilla::dom::Element* aElement,
> 
> Again, I don't think we need mozilla:: here
> 
> ::: layout/style/nsAnimationManager.cpp:1147
> (Diff revision 4)
> > +  // Currently we don't seem to call this UpdateAnimations for elements in
> > +  // display:none subtree. We will call this function for such elements with
> > +  // null computed values in bug 1341985.
> 
> Is it possible to add an assertion here that we're not in a display:none
> subtree? I guess not?

Right, no, as far as I can tell.

In stylo we drop all the style data (computed values)[1] if the element is display:none subtree. Also currently (until bug 1341985 landing) we do use nsStyleContext here and the nsStyleContext is genereted from a valid computed values. That means we don't reach here with the element in display:none subtree.  This is my understandings.

[1] https://hg.mozilla.org/mozilla-central/file/8d026c601510/servo/components/style/traversal.rs#l434
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d6b4177095e
Part 1: Split CSSAnimationBuilder::Build off as a static function. r=birtles
https://hg.mozilla.org/integration/autoland/rev/9e162c4475cb
Part 2: Use NonOwningAnimationTarget instead of Element and aStyleContext()->GetPseudoType(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/ce3017376156
Part 3: Make BuildAnimations static function. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ab3ba7fa8608
Part 4: Make BuildAnimations() independent from nsStyleDisplay r=birtles.
https://hg.mozilla.org/integration/autoland/rev/4d1293edbdb7
Part 5: Split off BuildKeyframes() r=birtles
https://hg.mozilla.org/integration/autoland/rev/e7a8cd3a2eba
Part 6: Add CSSAnimationBuilder::SetKeyframes(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/65f75e9d80fe
Part 7: Make BuildAnimation() and BuildAnimations() independent from nsStyleContext. r=birtles
https://hg.mozilla.org/integration/autoland/rev/7d02b6e56202
Part 8: Split off some processes that will be used for servo's computed values in UpdateAnimations(). r=birtles
https://hg.mozilla.org/integration/autoland/rev/7bacb513bfc1
Part 9: Templatize functions that will be used with servo's computed values. r=birtles
https://hg.mozilla.org/integration/autoland/rev/a04bbcdfda14
Part 10: Rename CSSAnimationBuilder to GeckoCSSAnimationBuilder. r=birtles
https://hg.mozilla.org/integration/autoland/rev/6f6d0b68ef23
Part 11: Introduce ServoCSSAnimationBuilder. r=birtles
https://hg.mozilla.org/integration/autoland/rev/33d72dd74490
Part 12: Add another variant of nsAnimationManager::UpdateAnimations for stylo. r=birtles
https://hg.mozilla.org/integration/autoland/rev/5233a7d951da
Part 13: Update CSS Animations with servo's computed values. r=birtles
You need to log in before you can comment on or make changes to this bug.