Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22861008:
Add support for inverse transform animations. (Closed)
|
Created:
7 years, 4 months ago by avallee Modified:
7 years, 3 months ago CC:
ajuma, chromium-reviews, jbauman+watch_chromium.org, piman+watch_chromium.org, danakj+watch_chromium.org, Ian Vollick, sievers+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd support for inverse transform animations.
This allows creation of a transform and its inverse that would be used
to counter-animate some child layer.
BUG=270857
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220124
R=vollick@chromium.org, wittman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220479
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221309
Patch Set 1 #Patch Set 2 : Add support for inverse transform animations. #
Total comments: 12
Patch Set 3 : Moved the logic into a new curve adapter and did miminal work in the counter transform element. #
Total comments: 8
Patch Set 4 : #Patch Set 5 : Create child animation element with pointer to its parent for animation properties. #Patch Set 6 : Remove duration parameters from the child and have the factory method pass it in. #
Total comments: 10
Patch Set 7 : Removed parent/child naming. #
Total comments: 4
Patch Set 8 : Address final nits. #Patch Set 9 : Rebase to head and fix new pure virtual in parent of InverseCurveAdapter. #Patch Set 10 : Fix build breaks, CurveAdapters needed to be exported for unittests in component builds. #Patch Set 11 : Add more detailed error messages in matrix comparison. #
Total comments: 9
Patch Set 12 : Fix the DCHECK production code and removed k prefix to const transform variable. #
Total comments: 7
Patch Set 13 : Fixed << spacing nit. #Patch Set 14 : Fixed loop condition nit. #Messages
Total messages: 38 (0 generated)
This is off to a great start! https://chromiumcodereview.appspot.com/22861008/diff/3001/ui/compositor/layer... File ui/compositor/layer_animation_element.cc (right): https://chromiumcodereview.appspot.com/22861008/diff/3001/ui/compositor/layer... ui/compositor/layer_animation_element.cc:613: new TransformAnimationCurveAdapter(tween_type(), I think we'll to perform matrix inversion in the curve's GetValue method, meaning we'll either need to add an additional argument to TransformAnimationCurveAdapter's constructor, or (more likely) we'll need a new kind of TransformAnimationCurveAdapter. https://chromiumcodereview.appspot.com/22861008/diff/3001/ui/compositor/layer... ui/compositor/layer_animation_element.cc:626: target->transform = ComputeWithParentTransform(start_, parent_target_); If ComputeWithParentTransform turns out to be more complex than, say, a matrix multiplication, we should store the target value to avoid having to recompute it each time this gets called. https://chromiumcodereview.appspot.com/22861008/diff/3001/ui/compositor/layer... ui/compositor/layer_animation_element.cc:642: DLOG(WARNING) << "Parent transform not invertible."; Use a DCHECK instead of a DLOG (so that we crash a debug build instead of providing output that might not be noticed). https://chromiumcodereview.appspot.com/22861008/diff/3001/ui/compositor/layer... ui/compositor/layer_animation_element.cc:645: to_return.ConcatTransform(start); Since we're skipping initialization above, this is concatenating with an uninitialized transform. https://chromiumcodereview.appspot.com/22861008/diff/3001/ui/compositor/layer... ui/compositor/layer_animation_element.cc:646: return to_return; This method isn't using |parent| anywhere. Is something missing? https://chromiumcodereview.appspot.com/22861008/diff/3001/ui/compositor/layer... File ui/compositor/layer_animation_element_unittest.cc (right): https://chromiumcodereview.appspot.com/22861008/diff/3001/ui/compositor/layer... ui/compositor/layer_animation_element_unittest.cc:118: delegate.GetTransformForAnimation() * For threaded animations, the delegate won't actually get updated as the animation progresses in LayerAnimationElementTests (since we don't actually have a compositor). So these are going to be stuck at their initial value. Consider checking the math by directly instantiating the corresponding TransformAnimationCurveAdapters and calling GetValue on them.
I've got a new patch set that should fix these issues. https://codereview.chromium.org/22861008/diff/3001/ui/compositor/layer_animat... File ui/compositor/layer_animation_element.cc (right): https://codereview.chromium.org/22861008/diff/3001/ui/compositor/layer_animat... ui/compositor/layer_animation_element.cc:613: new TransformAnimationCurveAdapter(tween_type(), On 2013/08/20 15:32:41, ajuma (vacation till Sept. 2) wrote: > I think we'll to perform matrix inversion in the curve's GetValue method, > meaning we'll either need to add an additional argument to > TransformAnimationCurveAdapter's constructor, or (more likely) we'll need a new > kind of TransformAnimationCurveAdapter. Done. https://codereview.chromium.org/22861008/diff/3001/ui/compositor/layer_animat... ui/compositor/layer_animation_element.cc:626: target->transform = ComputeWithParentTransform(start_, parent_target_); On 2013/08/20 15:32:41, ajuma (vacation till Sept. 2) wrote: > If ComputeWithParentTransform turns out to be more complex than, say, a matrix > multiplication, we should store the target value to avoid having to recompute it > each time this gets called. I'll leave this here for now. We could compute the target_ value in OnStart once if this turns out to be called frequently. https://codereview.chromium.org/22861008/diff/3001/ui/compositor/layer_animat... ui/compositor/layer_animation_element.cc:642: DLOG(WARNING) << "Parent transform not invertible."; On 2013/08/20 15:32:41, ajuma wrote: > Use a DCHECK instead of a DLOG (so that we crash a debug build instead of > providing output that might not be noticed). Done. https://codereview.chromium.org/22861008/diff/3001/ui/compositor/layer_animat... ui/compositor/layer_animation_element.cc:645: to_return.ConcatTransform(start); On 2013/08/20 15:32:41, ajuma wrote: > Since we're skipping initialization above, this is concatenating with an > uninitialized transform. It's initialized either by successfully getting the inverse of parent or by MakeIdentity in the failure case. https://codereview.chromium.org/22861008/diff/3001/ui/compositor/layer_animat... ui/compositor/layer_animation_element.cc:646: return to_return; On 2013/08/20 15:32:41, ajuma wrote: > This method isn't using |parent| anywhere. Is something missing? parent.GetInverse inside the if statement. https://codereview.chromium.org/22861008/diff/3001/ui/compositor/layer_animat... File ui/compositor/layer_animation_element_unittest.cc (right): https://codereview.chromium.org/22861008/diff/3001/ui/compositor/layer_animat... ui/compositor/layer_animation_element_unittest.cc:118: delegate.GetTransformForAnimation() * On 2013/08/20 15:32:41, ajuma (vacation till Sept. 2) wrote: > For threaded animations, the delegate won't actually get updated as the > animation progresses in LayerAnimationElementTests (since we don't actually have > a compositor). So these are going to be stuck at their initial value. Consider > checking the math by directly instantiating the corresponding > TransformAnimationCurveAdapters and calling GetValue on them. Moved this test to a new test file for the transform curve adapters. Would it still make sense to simulate an animation here?
https://codereview.chromium.org/22861008/diff/10001/ui/compositor/layer_anima... File ui/compositor/layer_animation_element.h (right): https://codereview.chromium.org/22861008/diff/10001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.h:71: // value. I worry a bit about ease of use here. I think this API is mostly reasonable, but in a follow-up patch, I would like to see ScopedLayerAnimationSettings updated to make the following possible: { ScopedLayerAnimationSettings settings(parent); settings.AddCounterAnimatedLayer(child); parent->SetTransform(target); } In this case |child| should not be affected by |parent|'s animation. In order for this to work, not only will |child|'s element have to be created with the right set of transforms and duration, you'll also have to make sure the tween type is right, etc. I think that this would be much easier if you were to take, rather than parent_end, the parent's transform animation element and steal its parameters. This is still error-prone, though. The parent's start transform may go stale by the time we start animating, but there's not much we can do about this until we move away from a per-layer animation controller to a per-tree controller that can allow the parent and child animations elements to interact and be scheduled as a unit. This will happen eventually, though; it's something ajuma@ plans to tackle. Please add a warning about the danger of staleness here and a TODO to get these animations coordinated explicitly. https://codereview.chromium.org/22861008/diff/10001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.h:72: static LayerAnimationElement* CreateInverseTransformElement( nit: Maybe CreateCounterTransformElement to match the verbage elsewhere in this patch? https://codereview.chromium.org/22861008/diff/10001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter.cc (right): https://codereview.chromium.org/22861008/diff/10001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter.cc:62: * initial_value_; nit: weird leading whitespace. https://codereview.chromium.org/22861008/diff/10001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/10001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:16: // applied on top of the parent transform gives the original transform Nice test!
https://codereview.chromium.org/22861008/diff/10001/ui/compositor/layer_anima... File ui/compositor/layer_animation_element.h (right): https://codereview.chromium.org/22861008/diff/10001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.h:71: // value. On 2013/08/23 12:45:19, vollick wrote: > I worry a bit about ease of use here. I think this API is mostly reasonable, but > in a follow-up patch, I would like to see ScopedLayerAnimationSettings updated > to make the following possible: > { > ScopedLayerAnimationSettings settings(parent); > settings.AddCounterAnimatedLayer(child); > parent->SetTransform(target); > } > In this case |child| should not be affected by |parent|'s animation. In order > for this to work, not only will |child|'s element have to be created with the > right set of transforms and duration, you'll also have to make sure the tween > type is right, etc. I think that this would be much easier if you were to take, > rather than parent_end, the parent's transform animation element and steal its > parameters. This is still error-prone, though. The parent's start transform may > go stale by the time we start animating, but there's not much we can do about > this until we move away from a per-layer animation controller to a per-tree > controller that can allow the parent and child animations elements to interact > and be scheduled as a unit. This will happen eventually, though; it's something > ajuma@ plans to tackle. Please add a warning about the danger of staleness here > and a TODO to get these animations coordinated explicitly. I haven't really worked through how a caller would get parent_start. In the forward transform animation, it gets the start transform from the delegate when the animation starts. Would we want a pointer to a parent ThreadedLayerAnimationElement, or something else? https://codereview.chromium.org/22861008/diff/10001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.h:72: static LayerAnimationElement* CreateInverseTransformElement( On 2013/08/23 12:45:19, vollick wrote: > nit: Maybe CreateCounterTransformElement to match the verbage elsewhere in this > patch? Done. https://codereview.chromium.org/22861008/diff/10001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter.cc (right): https://codereview.chromium.org/22861008/diff/10001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter.cc:62: * initial_value_; On 2013/08/23 12:45:19, vollick wrote: > nit: weird leading whitespace. Done. https://codereview.chromium.org/22861008/diff/10001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/10001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:16: // applied on top of the parent transform gives the original transform On 2013/08/23 12:45:19, vollick wrote: > Nice test! Thanks.
https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... File ui/compositor/layer_animation_element.cc (right): https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:566: // ChildCounterTransformTransition --------------------------------------------- I wonder if we could avoid the child/parent verbage here. Although the initial intent of this element is to allow children to counteract a parent's animation, I see no reason why someone couldn't use this in future to create an 'inverse' curve that is unrelated to a parent animation. Perhaps we could rename it InverseTransformTransition and we could rename the parameters parent_start -> base_transform and parent -> uninverted_transition? It would also help if you added a comment that described exactly the invariant that this element maintains (i.e., the child's transform will always be base_transform * uninverted_transition->at(t) * <some inverted stuff> = <some nice and tidy thing> https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:572: base::TimeDelta duration) This duration can be pulled from the 'parent' element, no? https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:613: TransformAnimationCurveAdapter parent_curve(tween_type(), The values in this function are built based on constants. Why not create them in OnStart and get rid of all the fields on this class that exist for the sole purpose of being parameters here? https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:665: gfx::Transform start_; Do we need to store this at all? https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:850: parent->duration()); See above about ditching the 3rd param.
https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... File ui/compositor/layer_animation_element.cc (right): https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:566: // ChildCounterTransformTransition --------------------------------------------- On 2013/08/26 14:19:55, vollick wrote: > I wonder if we could avoid the child/parent verbage here. Although the initial > intent of this element is to allow children to counteract a parent's animation, > I see no reason why someone couldn't use this in future to create an 'inverse' > curve that is unrelated to a parent animation. > > Perhaps we could rename it InverseTransformTransition and we could rename the > parameters parent_start -> base_transform and parent -> uninverted_transition? > It would also help if you added a comment that described exactly the invariant > that this element maintains (i.e., the child's transform will always be > base_transform * uninverted_transition->at(t) * <some inverted stuff> = <some > nice and tidy thing> Done. https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:572: base::TimeDelta duration) On 2013/08/26 14:19:55, vollick wrote: > This duration can be pulled from the 'parent' element, no? Plumbed through duration in a LAE constructor, makes sure not to double scale when usind the slow/fast animations. https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:613: TransformAnimationCurveAdapter parent_curve(tween_type(), On 2013/08/26 14:19:55, vollick wrote: > The values in this function are built based on constants. Why not create them in > OnStart and get rid of all the fields on this class that exist for the sole > purpose of being parameters here? Done. https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:665: gfx::Transform start_; On 2013/08/26 14:19:55, vollick wrote: > Do we need to store this at all? Thought we did, but reworked it so that it's no longer necessary. https://codereview.chromium.org/22861008/diff/23001/ui/compositor/layer_anima... ui/compositor/layer_animation_element.cc:850: parent->duration()); On 2013/08/26 14:19:55, vollick wrote: > See above about ditching the 3rd param. Done.
nice. lgtm with nits. https://codereview.chromium.org/22861008/diff/26001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter.h (right): https://codereview.chromium.org/22861008/diff/26001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter.h:39: class InverseTransformCurveAdapter :public cc::TransformAnimationCurve { nit: space after the colon. https://codereview.chromium.org/22861008/diff/26001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter.h:55: base::TimeDelta duration_; nit: I know this is missing elsewhere in this file, but shouldn't we be disallowing copy and assign here? If that works, please update the other class here, too.
All done. https://codereview.chromium.org/22861008/diff/26001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter.h (right): https://codereview.chromium.org/22861008/diff/26001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter.h:39: class InverseTransformCurveAdapter :public cc::TransformAnimationCurve { On 2013/08/27 19:54:18, vollick wrote: > nit: space after the colon. Done. https://codereview.chromium.org/22861008/diff/26001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter.h:55: base::TimeDelta duration_; On 2013/08/27 19:54:18, vollick wrote: > nit: I know this is missing elsewhere in this file, but shouldn't we be > disallowing copy and assign here? If that works, please update the other class > here, too. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avallee@chromium.org/22861008/31001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avallee@chromium.org/22861008/31001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avallee@chromium.org/22861008/69001
Fixed the exports that made the build unhappy.
Message was sent while issue was closed.
Change committed as 220124
Message was sent while issue was closed.
Reverted in r220141 due to Chromium OS unit test failure.
Could you link to the failure please? Thanks. On Wed, Aug 28, 2013 at 8:43 PM, <wittman@chromium.org> wrote: > Reverted in r220141 due to Chromium OS unit test failure. > > https://chromiumcodereview.**appspot.com/22861008/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Could you PTAL and check off submit.
https://codereview.chromium.org/22861008/diff/93001/ui/compositor/test/test_u... File ui/compositor/test/test_utils.cc (right): https://codereview.chromium.org/22861008/diff/93001/ui/compositor/test/test_u... ui/compositor/test/test_utils.cc:23: if (::testing::Test::HasFailure()) { I'm not sure about this, if another test had a failure adn then later called this function it'd get additional spam for no reason, right? Maybe keep a local bool here to decide to print this or not?
lgtm other than danakj's comment, although my experience with the compositor is minimal to say the least
https://codereview.chromium.org/22861008/diff/93001/ui/compositor/test/test_u... File ui/compositor/test/test_utils.cc (right): https://codereview.chromium.org/22861008/diff/93001/ui/compositor/test/test_u... ui/compositor/test/test_utils.cc:23: if (::testing::Test::HasFailure()) { On 2013/08/29 21:22:34, danakj wrote: > I'm not sure about this, if another test had a failure adn then later called > this function it'd get additional spam for no reason, right? > > Maybe keep a local bool here to decide to print this or not? Yeah, I'm not sure about this either. What I actually want to do is use the float equals code provided to EXPECT_FLOAT_EQ, but that class is inside ::testing::internal, so I can't rely on that.
https://codereview.chromium.org/22861008/diff/93001/ui/compositor/test/test_u... File ui/compositor/test/test_utils.cc (right): https://codereview.chromium.org/22861008/diff/93001/ui/compositor/test/test_u... ui/compositor/test/test_utils.cc:23: if (::testing::Test::HasFailure()) { On 2013/08/30 00:01:46, avallee wrote: > On 2013/08/29 21:22:34, danakj wrote: > > I'm not sure about this, if another test had a failure adn then later called > > this function it'd get additional spam for no reason, right? > > > > Maybe keep a local bool here to decide to print this or not? > > Yeah, I'm not sure about this either. What I actually want to do is use the > float equals code provided to EXPECT_FLOAT_EQ, but that class is inside > ::testing::internal, so I can't rely on that. Oh I see. I guess that's why I used side effects in the << string part, same reason. You could print the number of errors and increment in the << above, and use that here.
Message was sent while issue was closed.
Committed patchset #11 manually as r220479 (presubmit successful).
Message was sent while issue was closed.
I committed your CL and informed the sheriffs about it, so that we can get that data for you to have by tomorrow morning. I'll revert it shortly later on tonight once the bots hit it. I also learnt some stuff about flaky tests and failures today. It may be the test is flaky, and managed to pass on the try bot but not on the waterfall. This is even true if it fails on the try bot, but passes on the 2nd try - but doesn't on the waterfall. This happened to me today!
Or http://goo.gl/4MvYtN in case rietveld mangles that.
https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter.cc (right): https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter.cc:96: DCHECK(base_transform.GetInverse(&to_return)); DCHECKs don't run on release bots. I think you want book ok = base_transform.GetInverse(); DCHECK(ok) << to_return.ToString(); That explains a lot. Can't put production code in DCHECK :)
https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:30: const gfx::Transform kEffective_child_transform = nit: Since this isn't a compile-time constant, no need for the 'k' prefix. (That said, if you do prefix a constant with k, you have to switch the whole thing to camel case and ditch the hacker style underscores).
https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:30: const gfx::Transform kEffective_child_transform = On 2013/08/30 17:20:43, vollick (ooo until 09-02) wrote: > nit: Since this isn't a compile-time constant, no need for the 'k' prefix. (That > said, if you do prefix a constant with k, you have to switch the whole thing to > camel case and ditch the hacker style underscores). http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta... Style guide for reference. I've been applying that to all use of "const" it it really should just be for things that are const at compile time. (thanks ian for pointing this out.)
PTAL. Thanks. https://codereview.chromium.org/22861008/diff/93001/ui/compositor/test/test_u... File ui/compositor/test/test_utils.cc (right): https://codereview.chromium.org/22861008/diff/93001/ui/compositor/test/test_u... ui/compositor/test/test_utils.cc:23: if (::testing::Test::HasFailure()) { On 2013/08/30 00:04:09, danakj wrote: > On 2013/08/30 00:01:46, avallee wrote: > > On 2013/08/29 21:22:34, danakj wrote: > > > I'm not sure about this, if another test had a failure adn then later called > > > this function it'd get additional spam for no reason, right? > > > > > > Maybe keep a local bool here to decide to print this or not? > > > > Yeah, I'm not sure about this either. What I actually want to do is use the > > float equals code provided to EXPECT_FLOAT_EQ, but that class is inside > > ::testing::internal, so I can't rely on that. > > Oh I see. I guess that's why I used side effects in the << string part, same > reason. > > You could print the number of errors and increment in the << above, and use that > here. Done. https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter.cc (right): https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter.cc:96: DCHECK(base_transform.GetInverse(&to_return)); On 2013/08/30 03:03:40, danakj wrote: > DCHECKs don't run on release bots. I think you want book ok = > base_transform.GetInverse(); DCHECK(ok) << to_return.ToString(); > > That explains a lot. Can't put production code in DCHECK :) Done. D'oh! https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/93001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:30: const gfx::Transform kEffective_child_transform = On 2013/08/30 17:21:37, danakj wrote: > On 2013/08/30 17:20:43, vollick (ooo until 09-02) wrote: > > nit: Since this isn't a compile-time constant, no need for the 'k' prefix. > (That > > said, if you do prefix a constant with k, you have to switch the whole thing > to > > camel case and ditch the hacker style underscores). > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta... > > Style guide for reference. I've been applying that to all use of "const" it it > really should just be for things that are const at compile time. (thanks ian for > pointing this out.) Done.
lgtm
LGTM2 https://codereview.chromium.org/22861008/diff/65001/ui/compositor/test/test_u... File ui/compositor/test/test_utils.cc (right): https://codereview.chromium.org/22861008/diff/65001/ui/compositor/test/test_u... ui/compositor/test/test_utils.cc:20: << "(i, j) = (" << i << ", " << j <<"), error count: " << ++errors; nit: spaces around << https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:43: for (int i = 0; i != kSteps + 1; ++i) { nit: i < kSteps + 1 ?
https://codereview.chromium.org/22861008/diff/65001/ui/compositor/test/test_u... File ui/compositor/test/test_utils.cc (right): https://codereview.chromium.org/22861008/diff/65001/ui/compositor/test/test_u... ui/compositor/test/test_utils.cc:20: << "(i, j) = (" << i << ", " << j <<"), error count: " << ++errors; On 2013/09/04 15:29:09, danakj wrote: > nit: spaces around << Done. https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:43: for (int i = 0; i != kSteps + 1; ++i) { On 2013/09/04 15:29:09, danakj wrote: > nit: i < kSteps + 1 ? I want to exit the loop having done 1001 steps in this case, with i in the interval [0-1000]. At the end of the loop i == 1001, the number of steps taken.
https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:43: for (int i = 0; i != kSteps + 1; ++i) { On 2013/09/04 15:36:53, avallee wrote: > On 2013/09/04 15:29:09, danakj wrote: > > nit: i < kSteps + 1 ? > > I want to exit the loop having done 1001 steps in this case, with i in the > interval [0-1000]. At the end of the loop i == 1001, the number of steps taken. I'm not sure how < and != differ in this case. They both exit when i == kSteps + 1, don't they? But < is much more commonly seen and easier to understand.
https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:43: for (int i = 0; i != kSteps + 1; ++i) { On 2013/09/04 15:47:40, danakj wrote: > On 2013/09/04 15:36:53, avallee wrote: > > On 2013/09/04 15:29:09, danakj wrote: > > > nit: i < kSteps + 1 ? > > > > I want to exit the loop having done 1001 steps in this case, with i in the > > interval [0-1000]. At the end of the loop i == 1001, the number of steps > taken. > > I'm not sure how < and != differ in this case. They both exit when i == kSteps + > 1, don't they? But < is much more commonly seen and easier to understand. (i <= kSteps) would also work if you preferred.
https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... File ui/compositor/transform_animation_curve_adapter_unittest.cc (right): https://codereview.chromium.org/22861008/diff/65001/ui/compositor/transform_a... ui/compositor/transform_animation_curve_adapter_unittest.cc:43: for (int i = 0; i != kSteps + 1; ++i) { On 2013/09/04 15:48:26, danakj wrote: > On 2013/09/04 15:47:40, danakj wrote: > > On 2013/09/04 15:36:53, avallee wrote: > > > On 2013/09/04 15:29:09, danakj wrote: > > > > nit: i < kSteps + 1 ? > > > > > > I want to exit the loop having done 1001 steps in this case, with i in the > > > interval [0-1000]. At the end of the loop i == 1001, the number of steps > > taken. > > > > I'm not sure how < and != differ in this case. They both exit when i == kSteps > + > > 1, don't they? But < is much more commonly seen and easier to understand. > > (i <= kSteps) would also work if you preferred. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/avallee@chromium.org/22861008/103001
Message was sent while issue was closed.
Change committed as 221309 |
