Copy less arrays when constructing animations for the compositor

RESOLVED FIXED in mozilla17

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Dbaron: "It would be nice if AddTransformAnimations, etc., did less array
copying.  In particular, it would help if the AnimationSegment
constructor didn't take the arrays as arguments, but instead let you
fill in the arrays after constructing it.  Doesn't need to be done now,
though.  Also constructing the Animation at the top and adding the
segments directly to it."
(Assignee)

Comment 1

5 years ago
Created attachment 645086 [details] [diff] [review]
Part 1: Avoid copying animation segments
Attachment #645086 - Flags: review?(dbaron)
(Assignee)

Comment 2

5 years ago
The indentation of the closing curly braces looks a little wacky, but it's correct.
It's pretty clearly not correct in the diff.  Are you using strange diff options or something?
(Assignee)

Comment 4

5 years ago
I have 
git = 1
showfunc = 1
unified = 8

which is pretty standard as I understand it.
(Assignee)

Comment 5

5 years ago
Ah, I didn't qref after I fixed the indentation.
(Assignee)

Comment 6

5 years ago
Created attachment 645104 [details] [diff] [review]
Part 1: Avoid copying animation segments
Attachment #645086 - Attachment is obsolete: true
Attachment #645086 - Flags: review?(dbaron)
Attachment #645104 - Flags: review?(dbaron)
(Assignee)

Comment 7

5 years ago
Created attachment 653191 [details] [diff] [review]
Part 1: Avoid copying animation segments
Attachment #645104 - Attachment is obsolete: true
Attachment #645104 - Flags: review?(dbaron)
Attachment #653191 - Flags: review?(roc)
(Assignee)

Comment 8

5 years ago
Created attachment 653192 [details] [diff] [review]
Part 2: Add animation to layer upfront
Attachment #653192 - Flags: review?(roc)
Attachment #653191 - Flags: review?(roc) → review+
Comment on attachment 653192 [details] [diff] [review]
Part 2: Add animation to layer upfront

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

::: gfx/layers/Layers.cpp
@@ +242,5 @@
>  {}
>  
> +Animation*
> +Layer::AddAnimation(TimeStamp aStart, TimeDuration aDuration, float aIterations,
> +                    int aDirection, nsCSSProperty aProperty, AnimationData aData)

Take AnimationData as a const reference
Attachment #653192 - Flags: review?(roc) → review+
(Assignee)

Comment 10

5 years ago
I added the following to nsDisplayList::AddAnimationsForProperty to prevent crashes when performing visibility animations without opacity:
+  // If this is a visibility animation, we should not actually add it.
+  // This will be fixed in bug 783893
+  Animation* animation = nullptr;
+  for (PRUint32 propIdx = 0; propIdx < ea->mProperties.Length(); propIdx++) {
+    if (aProperty == ea->mProperties[propIdx].mProperty) {
+      animation = aLayer->AddAnimation(startTime, duration,
+                                       iterations, direction,
+                                       aProperty, aData);
+      break;
+    }
+  }

https://hg.mozilla.org/integration/mozilla-inbound/rev/f36ab372817e
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf32bd170798
https://hg.mozilla.org/mozilla-central/rev/f36ab372817e
https://hg.mozilla.org/mozilla-central/rev/bf32bd170798
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.