776401 - Copy less arrays when constructing animations for the compositor

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[David Zbarsky (:dzbarsky)]

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."

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Avoid copying animation segments

Comment 2

[David Zbarsky (:dzbarsky)]

The indentation of the closing curly braces looks a little wacky, but it's correct.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's pretty clearly not correct in the diff.  Are you using strange diff options or something?

Comment 4

[David Zbarsky (:dzbarsky)]

I have 
git = 1
showfunc = 1
unified = 8

which is pretty standard as I understand it.

Comment 5

[David Zbarsky (:dzbarsky)]

Ah, I didn't qref after I fixed the indentation.

Comment 6

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Avoid copying animation segments

Comment 7

[David Zbarsky (:dzbarsky)]

Attachment: Part 1: Avoid copying animation segments

Comment 8

[David Zbarsky (:dzbarsky)]

Attachment: Part 2: Add animation to layer upfront

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 10

[David Zbarsky (:dzbarsky)]

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

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f36ab372817e
https://hg.mozilla.org/mozilla-central/rev/bf32bd170798