Closed Bug 1195523 Opened 5 years ago Closed 5 years ago

Switch AnimationCommon::mElementCollections & AnimationCollection struct to be LinkedList<>-based, not PRCList-based

Categories

(Core :: DOM: Animation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
[d'oh, hit enter while typing bug summary]

Ever since nsTransitionManager was introduced, ages ago, we've used a PRClist to manage a list of transitions/animations. (nowadays in AnimationCommon::mElementCollections).

This requires ugly boilerplate macros and static_casts to initialize/clean-up/iterate over the list.

Nowadays, we have the MFBT templated LinkedList class. We should use that here instead, for cleaner more readable code.  (Like PRCList, it's a doubly-linked list. It's not circular, but I'm pretty sure we don't actually care about having this particular list be circular.)
Summary: Switch AnimationCommon → Switch AnimationCommon::mElementCollections & AnimationCollection struct to be LinkedList<>-based, not PRCList-based
Attached patch strawman (obsolete) — Splinter Review
Here's a strawman patch -- just manually ported all of the PRCList stuff to LinkedList & got it to compile.  Firefox starts fine, and a few animations in the UI played just fine too. Haven't sanity-checked this beyond that yet.

Try run: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(In reply to Daniel Holbert [:dholbert] from comment #1)
> Ever since nsTransitionManager was introduced, ages ago, we've used a
> PRClist to manage a list of transitions/animations.

For reference, that was in this cset from 2009:
 http://hg.mozilla.org/mozilla-central/rev/ff00a422b6f1#l24.153

> (nowadays in AnimationCommon::mElementCollections).

This was introduced in this cset from 2011:
 http://hg.mozilla.org/mozilla-central/rev/548241dd0c12#l2.151

And then the MFBT LinkedList class was added in 2012, in bug 715405.)

(This ^^ backstory is the answer to the question that prompted me to file this bug: "why are we using PRCList instead of e.g. LinkedList here?")
(My Try run hit some issues from something observing the refreshdriver longer than it should. Hopefully that's not too tough to investigate/solve though.)
Flags: needinfo?(dholbert)
Comment on attachment 8648977 [details] [diff] [review]
strawman

>   ~AnimationCollection()
>   {
>     MOZ_ASSERT(mCalledPropertyDtor,
>                "must call destructor through element property dtor");
>     MOZ_COUNT_DTOR(AnimationCollection);
>-    PR_REMOVE_LINK(this);
>     mManager->ElementCollectionRemoved();
>   }

Brian correctly guessed that the problem is here ^^.  AnimationCollection's superclass (LinkedListElement) will remove us from the list in its destructor, but we need to be removed *before* the call to ElementCollectionRemoved here, or else we're left with the manager (or something) observing the refresh driver.

Local test runs seem to get past the Try run's issues, with that changed.
Attachment #8648977 - Attachment is obsolete: true
Flags: needinfo?(dholbert)
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Try run: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound

Er, clearly I copypasted the URL from the wrong treeherder tab here ^^^. :)  I meant to post this link, for my first (broken) Try run:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b4febb2e621
Comment on attachment 8648977 [details] [diff] [review]
strawman

Fixed 2 more issues in initial patch:

>-  PR_INSERT_BEFORE(aCollection, &mElementCollections);
>+  mElementCollections.insertFront(aCollection);

I'd mistakenly read INSERT_BEFORE...mElementCollections as meaning "insert at the front of the list" here. But mElementCollections is actually a pointer to the end of the list, so this is insertion at the end. So, I've changed this locally to insertBack, which fixed some mochitest failures.

> CommonAnimationManager::RemoveAllElementCollections()
> {
>-  while (!PR_CLIST_IS_EMPTY(&mElementCollections)) {
>-    AnimationCollection* head =
>-      static_cast<AnimationCollection*>(PR_LIST_HEAD(&mElementCollections));
>-    head->Destroy();
>-  }
>+ while (AnimationCollection* head = mElementCollections.popFirst()) {
>+   head->Destroy();
>+ }

My "popFirst" usage here slightly tweaked the semantics (it ended up removing the AnimationCollection from mElementCollections, *before* we hit the AnimationCollection destructor, instead of waiting for it to be removed *in* its destructor).

I've changed this locally to use getFirst instead of popFirst, so that I'm not changing the order in which things happen.
Attached patch fix v2Splinter Review
Attachment #8651226 - Flags: review?(bbirtles)
Comment on attachment 8651226 [details] [diff] [review]
fix v2

>diff --git a/layout/style/AnimationCommon.cpp b/layout/style/AnimationCommon.cpp
>--- a/layout/style/AnimationCommon.cpp
>+++ b/layout/style/AnimationCommon.cpp
>@@ -77,27 +76,25 @@ CommonAnimationManager::AddElementCollec
>   if (!mIsObservingRefreshDriver) {
>     NS_ASSERTION(aCollection->mNeedsRefreshes,
>       "Added data which doesn't need refreshing?");
>     // We need to observe the refresh driver.
>     mPresContext->RefreshDriver()->AddRefreshObserver(this, Flush_Style);
>     mIsObservingRefreshDriver = true;
>   }
> 
>-  PR_INSERT_BEFORE(aCollection, &mElementCollections);
>+  mElementCollections.insertBack(aCollection);
> }

If I understand this right, the old code would leave us with:

  aCollection -> mElementCollections

But this new code will leave us with:

  mElementCollections -> aCollection

I guess that will effect things like the order we dispatch events and mutation observer records. Given that the current order isn't really sensible in anyway (it's based on when an element newly transitioned from having zero animations to 1 or more animations, I believe), this is probably ok. I'm currently working on both these areas to make us dispatch these things in a more sensible order.
 
So, looks good!
Attachment #8651226 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles, busy 22~30 Aug) from comment #10)
> If I understand this right, the old code would leave us with:
> 
>   aCollection -> mElementCollections
> 
> But this new code will leave us with:
> 
>   mElementCollections -> aCollection

If you want to change the order, that's probably best done in a separate patch for regression-hunting, though.
(In reply to Brian Birtles (:birtles, busy 22~30 Aug) from comment #10)
> If I understand this right, the old code would leave us with:
> 
>   aCollection -> mElementCollections
> 
> But this new code will leave us with:
> 
>   mElementCollections -> aCollection

I'm not sure I follow.

I think in the old code, mElementCollections is just a sentinel/dummy node at the end of the list -- not a node with useful data. Note that in our iteration, we iterate until " l != &mElementCollections;" -- i.e. we intentionally never visit that node.

So I don't think this patch is actually changing behavior -- it's still appending at the end of the list. In the old code, you do that by inserting before the sentinel node. In the new code, you do that with .insertBack().

Birtles, does this make sense, and can you clarify if you think I'm misunderstanding something? I agree with dbaron's sentiment - I very much do not want to change behavior in this refactoring patch, even in a way that seems correct.
Flags: needinfo?(bbirtles)
(In reply to Daniel Holbert [:dholbert] (vacation 8/27-9/13) from comment #12)
> (In reply to Brian Birtles (:birtles, busy 22~30 Aug) from comment #10)
> > If I understand this right, the old code would leave us with:
> > 
> >   aCollection -> mElementCollections
> > 
> > But this new code will leave us with:
> > 
> >   mElementCollections -> aCollection
> 
> I'm not sure I follow.
> 
> I think in the old code, mElementCollections is just a sentinel/dummy node
> at the end of the list -- not a node with useful data. Note that in our
> iteration, we iterate until " l != &mElementCollections;" -- i.e. we
> intentionally never visit that node.
> 
> So I don't think this patch is actually changing behavior -- it's still
> appending at the end of the list. In the old code, you do that by inserting
> before the sentinel node. In the new code, you do that with .insertBack().

Yes, you're right. I checked this over again and I agree it should be fine as you have it. Sorry for the confusion.
Flags: needinfo?(bbirtles)
No worries - thanks for double-checking & for the review! Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/793be272a336
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/793be272a336
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.