Merge ElementAnimations with CommonElementAnimationData

RESOLVED FIXED in mozilla33

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: birtles, Assigned: birtles)

Tracking

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

4.79 KB, patch
dbaron
: review+
birtles
: checkin+
Details | Diff | Splinter Review
5.29 KB, patch
dbaron
: review+
birtles
: checkin+
Details | Diff | Splinter Review
7.26 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
12.39 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.85 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
This corresponds to phase 4 outlined in bug 999922.
(Assignee)

Comment 1

4 years ago
Created attachment 8441077 [details] [diff] [review]
part 1 - Move IsForElement and PseudoElement from ElementAnimations to CommonElementAnimationData

IsForElement and PseudoElement are currently only defined on ElementAnimations
but could be used for transitions. This patch moves these methods to the common
base class CommonElementAnimationData and also makes use of PseudoElement within
nsTransitionManager.
Attachment #8441077 - Flags: review?(dbaron)
(Assignee)

Comment 2

4 years ago
Created attachment 8441078 [details] [diff] [review]
part 2 - Move PostRestyleForAnimation to CommonElementAnimationData

This patch moves PostRestyleForAnimation from ElementAnimations to the base
class CommonElementAnimationData and makes use of it within nsTransitionManager.
Attachment #8441078 - Flags: review?(dbaron)
(Assignee)

Comment 3

4 years ago
Created attachment 8441080 [details] [diff] [review]
part 3 - Move GetEventsAt to nsAnimationManager

In order to unify ElementAnimations with CommonElementAnimationData we need to
find another home for GetEventsAt which is specific to queueing CSS Animation
events. For now nsAnimationManager seems an appropriate place and corresponds
more closely to the arrangement for transitions (where nsTransitionManager is
responsible for queueing the events by iterating over the list of animations).

In future we may reintroduce a subclass of animation specific to CSS Animations
that does this event queueing but for now this nsAnimationManager seems to be
a suitable place.

This patch simply moves the code and replaces references to "mAnimation" with
"eEA->mAnimation". There are no functional changes.
Attachment #8441080 - Flags: review?(dbaron)
(Assignee)

Comment 4

4 years ago
Created attachment 8441081 [details] [diff] [review]
part 4 - Remove ElementAnimations

This patch removes ElementAnimations and replaces all references to
ElementAnimations with references to CommonElementAnimationData.

We don't bother to rename variables like 'ea' or methods like
GetElementAnimations to correspond with the data type
(CommonElementAnimationData) since CommonElementAnimationData will soon be
renamed in bug 1010067 and we'll rename these things then.

The ElementAnimationsPropertyDtor function is renamed and merged in a subsequent
patch in this series.
Attachment #8441081 - Flags: review?(dbaron)
(Assignee)

Comment 5

4 years ago
Created attachment 8441082 [details] [diff] [review]
part 5 - Make a common property dtor for CommonElementAnimationData

This patch takes the two static methods ElementAnimationsPropertyDtor and
ElementTransitionsPropertyDtor and replaces them with a class static on
CommonElementAnimationData.
Attachment #8441082 - Flags: review?(dbaron)
(Assignee)

Comment 6

4 years ago
Created attachment 8443285 [details] [diff] [review]
part 1 - Move IsForElement and PseudoElement from ElementAnimations to CommonElementAnimationData

Fixing bitrot in a few patches, and fixing layering in parts 3 and 4
Attachment #8443285 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8441077 - Attachment is obsolete: true
Attachment #8441077 - Flags: review?(dbaron)
(Assignee)

Comment 7

4 years ago
Created attachment 8443286 [details] [diff] [review]
part 2 - Move PostRestyleForAnimation to CommonElementAnimationData
Attachment #8443286 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8441078 - Attachment is obsolete: true
Attachment #8441078 - Flags: review?(dbaron)
(Assignee)

Comment 8

4 years ago
Created attachment 8443287 [details] [diff] [review]
part 3 - Move GetEventsAt to nsAnimationManager
Attachment #8443287 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8441080 - Attachment is obsolete: true
Attachment #8441080 - Flags: review?(dbaron)
(Assignee)

Comment 9

4 years ago
Created attachment 8443288 [details] [diff] [review]
part 4 - Remove ElementAnimations
Attachment #8443288 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8441081 - Attachment is obsolete: true
Attachment #8441081 - Flags: review?(dbaron)
(Assignee)

Comment 10

4 years ago
Created attachment 8443289 [details] [diff] [review]
part 5 - Make a common property dtor for CommonElementAnimationData
Attachment #8443289 - Flags: review?(dbaron)
(Assignee)

Updated

4 years ago
Attachment #8441082 - Attachment is obsolete: true
Attachment #8441082 - Flags: review?(dbaron)
Comment on attachment 8443285 [details] [diff] [review]
part 1 - Move IsForElement and PseudoElement from ElementAnimations to CommonElementAnimationData

(I don't mind else-after-return, although some other people do.)

r=dbaron
Attachment #8443285 - Flags: review?(dbaron) → review+
Comment on attachment 8443286 [details] [diff] [review]
part 2 - Move PostRestyleForAnimation to CommonElementAnimationData

I have some similar refactoring in my tree for bug 960465, but I'll merge with this.

r=dbaron
Attachment #8443286 - Flags: review?(dbaron) → review+
(Assignee)

Comment 13

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e719e2cc1bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a2b642bf77a
Keywords: leave-open
(Assignee)

Updated

4 years ago
Attachment #8443285 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8443286 - Flags: checkin+
(Assignee)

Comment 14

4 years ago
Backed out while I fixed build bustage (unreferenced variables):
https://hg.mozilla.org/integration/mozilla-inbound/rev/faee3492f067
(Assignee)

Comment 15

4 years ago
Pushed again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a35c6a47a929
https://hg.mozilla.org/integration/mozilla-inbound/rev/019fef132f2b
https://hg.mozilla.org/mozilla-central/rev/a35c6a47a929
https://hg.mozilla.org/mozilla-central/rev/019fef132f2b
Comment on attachment 8443287 [details] [diff] [review]
part 3 - Move GetEventsAt to nsAnimationManager

r=dbaron

(I considered suggesting making the method static, but it's probably better as it is.)
Attachment #8443287 - Flags: review?(dbaron) → review+
Comment on attachment 8443288 [details] [diff] [review]
part 4 - Remove ElementAnimations

>+  mozilla::css::CommonElementAnimationData* GetElementAnimations(
>+    mozilla::dom::Element *aElement,
>+    nsCSSPseudoElements::Type aPseudoType,
>+    bool aCreateIfNeeded);

I probably prefer formatting this as either:

  mozilla::css::CommonElementAnimationData*
  GetElementAnimations(mozilla::dom::Element *aElement,
                       nsCSSPseudoElements::Type aPseudoType,
                       bool aCreateIfNeeded);

or:

  mozilla::css::CommonElementAnimationData*
    GetElementAnimations(mozilla::dom::Element *aElement,
                         nsCSSPseudoElements::Type aPseudoType,
                         bool aCreateIfNeeded);


We probably want a followup patch to rename CommonElementAnimationData to something else -- it was previously named as it was because it was the common base class for ElementAnimations and ElementTransitions, but I think the name doesn't really make sense at this point.

r=dbaron on this patch, though
Attachment #8443288 - Flags: review?(dbaron) → review+
Comment on attachment 8443289 [details] [diff] [review]
part 5 - Make a common property dtor for CommonElementAnimationData

r=dbaron
Attachment #8443289 - Flags: review?(dbaron) → review+
Keywords: leave-open
(Assignee)

Comment 20

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a01902ab089
https://hg.mozilla.org/integration/mozilla-inbound/rev/e69467cd9d52
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c82c80954dd


(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #18)
> We probably want a followup patch to rename CommonElementAnimationData to
> something else -- it was previously named as it was because it was the
> common base class for ElementAnimations and ElementTransitions, but I think
> the name doesn't really make sense at this point.

That would be bug 1010067 comment 1 (I plan to add more patches to that bug but I was waiting for this to land first).
https://hg.mozilla.org/mozilla-central/rev/3a01902ab089
https://hg.mozilla.org/mozilla-central/rev/e69467cd9d52
https://hg.mozilla.org/mozilla-central/rev/4c82c80954dd
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33

Updated

4 years ago
Depends on: 1029927
You need to log in before you can comment on or make changes to this bug.