Closed Bug 776401 Opened 8 years ago Closed 8 years ago

Copy less arrays when constructing animations for the compositor

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Details

Attachments

(2 files, 2 obsolete files)

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."
Attachment #645086 - Flags: review?(dbaron)
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?
I have 
git = 1
showfunc = 1
unified = 8

which is pretty standard as I understand it.
Ah, I didn't qref after I fixed the indentation.
Attachment #645086 - Attachment is obsolete: true
Attachment #645086 - Flags: review?(dbaron)
Attachment #645104 - Flags: review?(dbaron)
Attachment #645104 - Attachment is obsolete: true
Attachment #645104 - Flags: review?(dbaron)
Attachment #653191 - Flags: review?(roc)
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+
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.