Last Comment Bug 776401 - Copy less arrays when constructing animations for the compositor
: Copy less arrays when constructing animations for the compositor
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: David Zbarsky (:dzbarsky)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-22 14:13 PDT by David Zbarsky (:dzbarsky)
Modified: 2012-08-21 06:32 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Avoid copying animation segments (6.14 KB, patch)
2012-07-23 15:04 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Part 1: Avoid copying animation segments (5.89 KB, patch)
2012-07-23 15:52 PDT, David Zbarsky (:dzbarsky)
no flags Details | Diff | Splinter Review
Part 1: Avoid copying animation segments (3.10 KB, patch)
2012-08-19 09:29 PDT, David Zbarsky (:dzbarsky)
roc: review+
Details | Diff | Splinter Review
Part 2: Add animation to layer upfront (6.09 KB, patch)
2012-08-19 09:30 PDT, David Zbarsky (:dzbarsky)
roc: review+
Details | Diff | Splinter Review

Description David Zbarsky (:dzbarsky) 2012-07-22 14:13:44 PDT
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) 2012-07-23 15:04:28 PDT
Created attachment 645086 [details] [diff] [review]
Part 1: Avoid copying animation segments
Comment 2 David Zbarsky (:dzbarsky) 2012-07-23 15:07:49 PDT
The indentation of the closing curly braces looks a little wacky, but it's correct.
Comment 3 David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) 2012-07-23 15:23:42 PDT
It's pretty clearly not correct in the diff.  Are you using strange diff options or something?
Comment 4 David Zbarsky (:dzbarsky) 2012-07-23 15:50:42 PDT
I have 
git = 1
showfunc = 1
unified = 8

which is pretty standard as I understand it.
Comment 5 David Zbarsky (:dzbarsky) 2012-07-23 15:51:55 PDT
Ah, I didn't qref after I fixed the indentation.
Comment 6 David Zbarsky (:dzbarsky) 2012-07-23 15:52:26 PDT
Created attachment 645104 [details] [diff] [review]
Part 1: Avoid copying animation segments
Comment 7 David Zbarsky (:dzbarsky) 2012-08-19 09:29:30 PDT
Created attachment 653191 [details] [diff] [review]
Part 1: Avoid copying animation segments
Comment 8 David Zbarsky (:dzbarsky) 2012-08-19 09:30:02 PDT
Created attachment 653192 [details] [diff] [review]
Part 2: Add animation to layer upfront
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-19 17:48:37 PDT
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) 2012-08-20 06:38:39 PDT
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

Note You need to log in before you can comment on or make changes to this bug.