1239945 - Clean up animation code in layout/style

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Birtles (:birtles)]

After all the refactoring to support script-generated animations there is a lot of animation-related code we can clean up in layout/style.

Comment 1

[Brian Birtles (:birtles)]

I have a pretty long patch series for this but a lot of it has bit-rotted now. I'm going to start putting them up for review in the hope I can land of few of them soonish before they bitrot even further.

Comment 2

[Brian Birtles (:birtles)]

Attachment: part 1 - Drop a number of animation manager/collection-related methods that are unused (and in some cases undefined)

Comment 3

[Brian Birtles (:birtles)]

Attachment: part 2 - Drop CommonAnimationManager::ContentOrAncestorHasAnimation

This was added in bug 780692 to work around assertions that arose due to the
inconsistent state introduced by mini-flushes. However, that workaround
no longer seems necessary. In particular, the crashtest for bug 813372 no
longer reports failed assertions when we remove this method and nor do any
other tests.

I'm not sure exactly what changed about how we do mini-flushes but I suspect
it was bug 960465 or one of the related follow-ups.

Comment 4

[Brian Birtles (:birtles)]

Attachment: part 3 - Move AnimationCollection to a separate file

In this bug we will trim off unnecessary functionality from the animation
managers and make AnimationCollection into an independent data type
so in this patch we separate it into its own file.

It is also generally easier to navigate the source code and eliminate
cyclic dependencies between header files when there is a rough
correspondance between class names and file names (e.g. rather than having
#include "AnimationCommon.h" // For mozilla::AnimationCollection).

We move *most* of the implementation for AnimationCollection into the header
file since in a subsequent patch we will make AnimationCollection a templatized
class in which the implementation will live in the header file. By moving
this code to the header file we avoid moving it twice hopefully making
the code easier to review and simplifying blame history.

The one exception is AnimationCollection::UpdateCheckGeneration which we put in
a cpp file now since it requires including RestyleManager which introduces
a cyclic dependency with nsTransitionManager.h. We will hopefully move this to
the header file and drop AnimationCollection.cpp when we templatize this class.

This patch also makes a few simplifications to include dependencies since
they're a bit of a mess (making it hard to move code around). The changes to
IncrementalClearCOMRuleArray.cpp are due to the changes to the unified build
introduced by adding AnimationCollection.cpp exposing a missing include from
that file.

Comment 5

[Brian Birtles (:birtles)]

Attachment: part 4 - Remove the pointer from an AnimationCollection to its manager since it is no longer used

Comment 6

[Brian Birtles (:birtles)]

Attachment: part 5 - Use MayHaveAnimations() to return early in CommonAnimationManager::GetAnimationCollection

Currently, CommonAnimationManager::GetAnimationCollection returns early when
the referenced list of animation collections is empty. So, for example, if
we try to get the collection of CSS animations on an element on a page with no
CSS animations, we will quickly return null without possibly expensive property
lookup. However, if there is just one CSS animation on the page, we will do the
property lookup for every element in the page where this method is called.

In this bug, we would like to remove the linked list of animation collections
since this is now the only place where it is used. So, in place if this
optimization, we introduce quite a different one based on the changes from bug
1226091 which makes MayHaveAnimations() apply to animations on the element
itself as well as pseudo elements. Using this, we can return early for any
element that has never had any kind of animation on it. The page may have
dozens of other animations but we will still return early. However, if the
element has ever had any kind of animation on it, we will not return early. It
is expected that this optimization is at least as good as the one it replaces.

Comment 7

[Brian Birtles (:birtles)]

Hi Daniel, sorry to load you up with all these reviews at the start of the week. If you're busy I can probably assign some of the simpler ones to others.

In terms of priority, this is groundwork for bug 1245748 which I need to get into Firefox 47 but it will take me some time to finish that bug. If you are able to look at just the first 4 patches quickly that would help since they are likely to bitrot quickly and a mostly quite simple changes.

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 8721868 [details] [diff] [review]
part 3 - Move AnimationCollection to a separate file

Clearing review request because I think I'll leave the implementation in the .cpp file and just explicitly instantiate the two template classes we need. This is because in subsequent patches we can't get the entire implementation in the .h file due to cyclic dependencies with RestyleManager.h that cause problems on Linux and OS X.

Comment 9

[Brian Birtles (:birtles)]

Attachment: part 3 - Move AnimationCollection to a separate file

In this bug we will trim off unnecessary functionality from the animation
managers and make AnimationCollection into an independent data type
so in this patch we separate it into its own file.

It is also generally easier to navigate the source code and eliminate
cyclic dependencies between header files when there is a rough
correspondance between class names and file names (e.g. rather than having
#include "AnimationCommon.h" // For mozilla::AnimationCollection).

This patch also makes a few simplifications to include dependencies since
they're a bit of a mess (making it hard to move code around). The changes to
IncrementalClearCOMRuleArray.cpp are due to the changes to the unified build
introduced by adding AnimationCollection.cpp exposing a missing include from
that file.

Comment 10

[Brian Birtles (:birtles)]

Attachment: part 6 - Templatize AnimationCollection based on the concrete type of Animation stored

This patch templatizes the type of Animation stored in an AnimationCollection.
This allows us to remove a number AsCSSAnimation() calls in nsAnimationManager.

This patch also removes the AnimationPtrArray typedef. In its place we
introduce OwningCSSAnimationPtrArray and OwningCSSTransitionPtrArray but we
don't use these as widely. There was some comment previously that the typedefs
in animation code make it hard to read, particularly when these typedefs don't
make it clear if the data type is an owning reference or not.

In doing this we need to templatize CommonAnimationManager as well and move the
implementation of its (few) methods to the header file. We may be able to
remove the need for templatizing CommonAnimationManager later in this patch
series depending on how we ultimately decide to handle the lifetime of
AnimationCollection objects.

CommonAnimationManager::GetAnimationCollection is a bit messy but this will be
significantly tidied up in subsequent patches in this series.

Comment 11

[Brian Birtles (:birtles)]

Attachment: part 7 - Move GetAnimationCollection to AnimationCollection

By moving GetAnimationCollection to AnimationCollection itself, we can remove
a bunch of virtual methods on the animation managers, simplify call sites,
and provide better type safety by ensuring a correspondence between element
property names and concrete animation types.

One change in behavior, however, is that in doing this we can no longer
add any newly-created AnimationCollection to the corresponding manager's linked
list of collections inside GetAnimationCollection. Instead we take a bool
outparam to indicate if a new collection was created and leave managing the
linked list to the manager. This is just a temporary measure, however, since
by the end of this patch series will will eliminate this linked list altogether
along with this flag.

Comment 12

[Brian Birtles (:birtles)]

Attachment: part 8 - Add GetOrCreateAnimationCollection

Rather than passing around a bool flag to indicate if we should be creating
an AnimationCollection when none is found, it would be a lot easier to read
if we simply introduce a separate method for this.

Comment 13

[Brian Birtles (:birtles)]

There is more to this patch series but that's probably enough for the time being.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8721888 [details] [diff] [review]
part 3 - Move AnimationCollection to a separate file

Review of attachment 8721888 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, nits below:

::: layout/style/AnimationCollection.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Nit:
s/tab-width: 2/tab-width: 8/ to match our "For new files, use this" coding-style guideline:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line

(I believe the reason for using 8 is to make tab characters more obvious, and hence easier to kill.)

::: layout/style/AnimationCollection.h
@@ +1,1 @@
> +/* vim: set shiftwidth=2 tabstop=8 autoindent cindent expandtab: */

This new file is missing an emacs modeline -- please include our standard "for new files" emacs modeline from here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Mode_Line
...which is:
/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

@@ +19,5 @@
> +
> +namespace mozilla {
> +
> +class CommonAnimationManager;
> +typedef InfallibleTArray<RefPtr<dom::Animation>> AnimationPtrArray;

I think you need an
  #include "mozilla/dom/Animation.h"
to define the dom::Animation type, before this line.

(You might be getting it for free via some other header or via unified builds, but let's not depend on that.)

::: layout/style/AnimationCommon.h
@@ -7,5 @@
>  #define mozilla_css_AnimationCommon_h
>  
>  #include <algorithm> // For <std::stable_sort>
> -#include "nsChangeHint.h"
> -#include "nsCSSProperty.h"

This file still uses nsCSSProperty, e.g.
   static bool ExtractComputedValueForTransition(
                   nsCSSProperty aProperty,
...so we should not be removing this include.

@@ -13,3 @@
>  #include "mozilla/AnimationComparator.h"
>  #include "mozilla/EventDispatcher.h"
> -#include "mozilla/LinkedList.h"

This file still uses LinkedList:
  class CommonAnimationManager {
    [...]
    LinkedList<AnimationCollection> mElementCollections;

...so we should not be removing this include.

::: layout/style/moz.build
@@ +80,5 @@
>      'nsStyleUtil.h',
>  ]
>  
>  EXPORTS.mozilla += [
> +    'AnimationCollection.h',

You're adding this header to two different lists here -- EXPORTS and EXPORTS.mozilla. Does that make sense?

(From a quick skim/spot-check, it looks like other headers only exist in one list or the other.)

We probably want to get rid of the first (EXPORTS) entry, I imagine, to follow the New Way of having everything mozilla-namespaced.

::: layout/style/nsTransitionManager.h
@@ -12,2 @@
>  #include "mozilla/ContentEvents.h"
> -#include "mozilla/EffectCompositor.h" // For EffectCompositor::CascadeLevel

This file still uses the enum type "EffectCompositor::CascadeLevel":
    return IsTiedToMarkup() ?
           EffectCompositor::CascadeLevel::Transitions :
           EffectCompositor::CascadeLevel::Animations;
...so we should not be removing this include.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14163554db50
https://hg.mozilla.org/integration/mozilla-inbound/rev/c98725a7f74e
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f552b246eb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3a832dbdba
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a24d67478b
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5153ecb53ba

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14163554db50
https://hg.mozilla.org/mozilla-central/rev/c98725a7f74e
https://hg.mozilla.org/mozilla-central/rev/1f552b246eb4
https://hg.mozilla.org/mozilla-central/rev/4e3a832dbdba
https://hg.mozilla.org/mozilla-central/rev/f0a24d67478b
https://hg.mozilla.org/mozilla-central/rev/c5153ecb53ba

Comment 17

[Brian Birtles (:birtles)]

Attachment: part 6 - Templatize AnimationCollection based on the concrete type of Animation stored

Fix bitrot

Comment 18

[Brian Birtles (:birtles)]

Attachment: part 7 - Move GetAnimationCollection to AnimationCollection

Fix bitrot

Comment 19

[Brian Birtles (:birtles)]

Attachment: part 8 - Add GetOrCreateAnimationCollection

Fix bitrot

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8723934 [details] [diff] [review]
part 6 - Templatize AnimationCollection based on the concrete type of Animation stored

Sorry for the review lag here -- I had another large patch-stack to get through, which took me longer than expected (plus a few days of planned PTO scattered in).

(Hopefully the lag didn't block you too much -- looks like bug 1245748 (which this bug is in the service of per comment 7) is still at the WIP stage, which is partly why I let this slip a bit.)

Just one nit on part 6:
>+++ b/layout/style/AnimationCollection.h
>   ~AnimationCollection()
>   {
>     MOZ_ASSERT(mCalledPropertyDtor,
>                "must call destructor through element property dtor");
>     MOZ_COUNT_DTOR(AnimationCollection);
>-    remove();
>+    LinkedListElement<self_type>::remove();

Nit: this one-line change (adding "LinkedListElement<self_type>::" before "remove()" here) doesn't seem necessary.  There's only one "remove()" function available here, AFAICT -- the one we inherit from LinkedListElement<self_type>.  So, the old "remove()" call should be OK on its own. At least, I can build just fine locally, if I apply this patch & revert this one-line change.

(Maybe you're just making this change for clarification about what "remove" is? If so, it's fine to keep it if you like. Just pointing it out in case you added it as a hackaround for a build failure at some intermediate point -- you might not need it for that anymore.)

Comment 21

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> Sorry for the review lag here -- I had another large patch-stack to get
> through, which took me longer than expected (plus a few days of planned PTO
> scattered in).
> 
> (Hopefully the lag didn't block you too much -- looks like bug 1245748
> (which this bug is in the service of per comment 7) is still at the WIP
> stage, which is partly why I let this slip a bit.)

Thanks Daniel! I was a bit worried this would bitrot but it seems fine. Also, I saw you had the big queue of grid patches that you were working your way through so I figured you'd get to this before long.

> Just one nit on part 6:
> >+++ b/layout/style/AnimationCollection.h
> >   ~AnimationCollection()
> >   {
> >     MOZ_ASSERT(mCalledPropertyDtor,
> >                "must call destructor through element property dtor");
> >     MOZ_COUNT_DTOR(AnimationCollection);
> >-    remove();
> >+    LinkedListElement<self_type>::remove();
> 
> Nit: this one-line change (adding "LinkedListElement<self_type>::" before
> "remove()" here) doesn't seem necessary.  There's only one "remove()"
> function available here, AFAICT -- the one we inherit from
> LinkedListElement<self_type>.  So, the old "remove()" call should be OK on
> its own. At least, I can build just fine locally, if I apply this patch &
> revert this one-line change.

I certainly ran into build errors without this change at one point but I might have shuffled the patch queue just before putting it up for review so it might not be necessary any more--or it might be a platform-specific thing. I'll check again.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8723935 [details] [diff] [review]
part 7 - Move GetAnimationCollection to AnimationCollection

Review of attachment 8723935 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/AnimationCollection.h
@@ +21,5 @@
>  
>  namespace mozilla {
>  
>  template <class AnimationType>
> +struct AnimationTypeTraits { };

Please add a comment explaining the point of this empty struct.

(After poking around a bit, it looks like you provide templated specializations of this struct (elsewhere) for each AnimationType (css transitions vs. css animations), and those specializations provide some functions to return property-table keys.)

(Your explanatory comment here should hint at this ^^.)

@@ +47,5 @@
>    {
>      MOZ_ASSERT(mCalledPropertyDtor,
>                 "must call destructor through element property dtor");
>      MOZ_COUNT_DTOR(AnimationCollection);
> +    LinkedListElement<SelfType>::remove();

(This is just replacing self_type with SelfType.)

As noted in previous comment, I think this could still just be "remove();" (as it is in current mozilla-central).

@@ +65,5 @@
> +  static AnimationCollection<AnimationType>*
> +    GetAnimationCollection(dom::Element* aElement,
> +                           CSSPseudoElementType aPseudoType,
> +                           bool aCreateIfNeeded,
> +                           bool* aCreatedCollection = nullptr);

These "aCreateIfNeeded" and "aCreatedCollection" params allow for some confusing nonsensical combinations, and they perhaps want to be combined.  (For example: if someone called this function with aCreateIfNeeded=true but didn't pass in an |aCreatedCollection| outparam, that'd probably be a bug, because the caller should want to know if the collection was actually created, I expect.)

So: consider dropping aCreateIfNeeded entirely, and just checking |aCreatedCollection| (the pointer-value) in its place. (and document that aCreatedCollection has this double-meaning -- i.e. if it's non-null, then we'll create if needed & set it to indicate whether we succeeded.)

This will simplify the API a bit, and will remove a bunch of "false /* aCreateIfNeeded */" boilerplate from callsites.

This could happen in a followup, if you like. (Or, feel free to disregard this; but I think it's worth considering.)

@@ +69,5 @@
> +                           bool* aCreatedCollection = nullptr);
> +
> +  // Given the frame |aFrame| with possibly animated content, finds its
> +  // associated collection of animations. If it is a generated content
> +  // frame, it may examine the parent frame to search for such animations.

Mixed usage of "it" here, making this hard to read/interpret. I think you mean:
 "If it [the frame] ..., it [this function] may examine..."

s/it may/this function may/

::: layout/style/nsAnimationManager.cpp
@@ +433,5 @@
> +                                                     true /*aCreateIfNeeded*/,
> +                                                     &createdCollection);
> +    if (!collection) {
> +      NS_WARNING("allocating collection failed");
> +      return;

Please add something like this inside of this clause:
  MOZ_ASSERT(!createdCollection, "outparam should agree with return value");

::: layout/style/nsAnimationManager.h
@@ -326,5 @@
>    void StopAnimationsForElement(mozilla::dom::Element* aElement,
>                                  mozilla::CSSPseudoElementType aPseudoType);
>  
> -protected:
> -  virtual ~nsAnimationManager() {}

You need to keep this destructor declaration (and it needs to be private or protected).  Otherwise, this patch will make us fail the following static_assert, on my local machine:

  nsAnimationManager.h:298:3: error: static_assert failed "Reference-counted
  class nsAnimationManager should not have a public destructor. Make this
  class's destructor non-public"

(I'm building with clang, if it matters.  This comes from MOZ_ASSERT_TYPE_OK_FOR_REFCOUNTING, which gets pulled in via the REFCOUNTING macro for this class.)

(Note also that the destructor here is already virtual, since it's virtual in the parent class. So adding back this line won't add back any otherwise-removed virtualness.)

::: layout/style/nsTransitionManager.cpp
@@ +711,2 @@
>      if (!aElementTransitions) {
> +      NS_WARNING("allocating collection failed");

Similar to above -- please add something like this inside of this clause:
  MOZ_ASSERT(!createdCollection, "outparam should agree with return value");

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> These "aCreateIfNeeded" and "aCreatedCollection" params allow for some
> confusing nonsensical combinations, and they perhaps want to be combined.

(Ah, I see part 8 addresses this in a different way. Cool.)

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8723936 [details] [diff] [review]
part 8 - Add GetOrCreateAnimationCollection

Review of attachment 8723936 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/AnimationCollection.cpp
@@ +93,5 @@
> +
> +  auto collection = static_cast<AnimationCollection<AnimationType>*>(
> +                      aElement->GetProperty(propName));
> +  if (!collection) {
> +    collection = new AnimationCollection<AnimationType>(aElement, propName);

Previously we had this right after the "new" here:
  // FIXME: Consider arena-allocating?
But that comment is gone in this moved version of the code.

Unless we've ruled out arena allocation here, let's keep the comment around for now.

@@ +101,5 @@
> +                            false);
> +    if (NS_FAILED(rv)) {
> +      NS_WARNING("SetProperty failed");
> +      // The collection must be destroyed via PropertyDtor, otherwise
> +     // mCalledPropertyDtor assertion is triggered in destructor.

Fix indentation on the 2nd line of this comment.

@@ +108,5 @@
> +      return nullptr;
> +    }
> +
> +    *aCreatedCollection = true;
> +    aElement->SetMayHaveAnimations();

(Side note: we should consider renaming this function with s/May/Might/.  When skimming this just now, I confused myself about what this line does -- I was reading "may" as talking about permission ("is this element allowed to have animations"), rather than about possibility ("is it possible that this element has animations").)

::: layout/style/AnimationCollection.h
@@ +72,5 @@
>    static AnimationCollection<AnimationType>* GetAnimationCollection(
>      const nsIFrame* aFrame);
>  
> +  // Get the collection of animations for the given |aElement| and
> +  // |aPseudoType| or create it if it does not already exist.

Please add some brief documentation to clarify that this method is allowed to fail, and to explain how the outparam ties into that.

e.g.:
  // We'll set the outparam |aCreatedCollection| to true if we have
  // to create the collection & we successfully do so. Otherwise,
  // we'll set it to false.

Comment 25

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #21)
> (In reply to Daniel Holbert [:dholbert] from comment #20)
> > Nit: this one-line change (adding "LinkedListElement<self_type>::" before
> > "remove()" here) doesn't seem necessary.  There's only one "remove()"
> > function available here, AFAICT -- the one we inherit from
> > LinkedListElement<self_type>.  So, the old "remove()" call should be OK on
> > its own. At least, I can build just fine locally, if I apply this patch &
> > revert this one-line change.
> 
> I certainly ran into build errors without this change at one point but I
> might have shuffled the patch queue just before putting it up for review so
> it might not be necessary any more--or it might be a platform-specific
> thing. I'll check again.

It turns out we do need this clarification. Without it I get the following error on Linux and Windows:

/builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/dist/include/mozilla/AnimationCollection.h:54:12: error: too few arguments to function 'int remove(const char*)'

The conflict appears to be with remove() defined in stdio.h. Perhaps you're not hitting this because you're using clang?

I notice we've encountered this elsewhere and worked around it with the same clarification:

  https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/dom/canvas/WebGLBuffer.cpp#66
  https://dxr.mozilla.org/mozilla-central/rev/05c087337043dd8e71cc27bdb5b9d55fd00aaa26/dom/canvas/WebGLSync.cpp#31

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/565778f033af
https://hg.mozilla.org/integration/mozilla-inbound/rev/e646421ae231
https://hg.mozilla.org/integration/mozilla-inbound/rev/32a37cf602d8

Comment 27

[Brian Birtles (:birtles)]

Leaving leave-open set since there are still more patches in this series.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/565778f033af
https://hg.mozilla.org/mozilla-central/rev/e646421ae231
https://hg.mozilla.org/mozilla-central/rev/32a37cf602d8

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:birtles, maybe it's time to close this bug?

Comment 30

[Brian Birtles (:birtles)]

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #29)
> The leave-open keyword is there and there is no activity for 6 months.
> :birtles, maybe it's time to close this bug?

Yes, good idea. I had a few more patches lying around for this bug but I can't see me landing them any time soon.

I'd like to mark this as fixed in Firefox 48 but I don't seem to have that option in the "Target" dropdown.