Add VR Shell animation classes and unit test.

Add VR Shell animation classes and unit test.

This change includes the animation and easing (intermediate state
calculation) capabilities developed by mthiesse@, and adds initial unit
tests for animation.

BUG=

Committed: https://crrev.com/1ce4ff3d8c73bfd77e7f8d0135134f2524b4d480
Cr-Commit-Position: refs/heads/master@{#419449}

[cjgrant, 2016-09-12 13:28:57]

Description was changed from

==========
Add VR Shell animation classes and unit test.

This change includes the animation and easing (intermediate state
calculation) capabilities developed by mthiesse@, and adds initial unit
tests for animation.

BUG=
==========

to

==========
Add VR Shell animation classes and unit test.

This change includes the animation and easing (intermediate state
calculation) capabilities developed by mthiesse@, and adds initial unit
tests for animation.

BUG=
==========

[cjgrant, 2016-09-12 13:28:57]

cjgrant@chromium.org changed reviewers:
+ bshe@chromium.org, klausw@chromium.org, mthiesse@chromium.org

[cjgrant, 2016-09-12 13:30:42]

Folks, note that this change will require a rebase after Michael's CL 2301633002
lands.  This is posted for early review of the unit test code.

[mthiesse, 2016-09-12 14:18:27]

Just some nits, otherwise looks good.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/animation.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/animation.h:21: UNUSED, // Available for a
future type.
Add a comment for why this strangeness is here?

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_gl.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl.h:15: const gvr::Rectf& texture_rect);:w
Now now, no need to brag about your vim usage.

;)

[bshe, 2016-09-12 15:38:27]

mostly nits

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/BUILD.gn (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/BUILD.gn:44: test("vr_shell_unittests") {
I assume you have a follow up CL that config bot to build and run these tests?
It would be nice to file a bug and refer to it here.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/BUILD.gn:46: "animation.cc",
no unittest related file should be moved to vr_shell? And then you can probably
depend on vr_shell to pull the dependency.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/easing.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:10: #include "base/logging.h"
incorrect include? You probably just want base/macros.h

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:11: #include
"ui/gfx/geometry/cubic_bezier.h"
move this include to .cc?

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:28: DISALLOW_COPY_AND_ASSIGN(Easing);
DISALLOW_COPY_AND_ASSIGN should be private

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:38: gfx::CubicBezier bezier;
nit: s/bezier/bezier_

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:46: double mPower;
nit: s/mPower/m_power_

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:54: double mPower;
nit: s/mPower/m_power_

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.cc:98: // If no "from" was
specified, use the current value.
s/"from"/|from|

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.cc:173: for (auto it =
animations.begin(); it != animations.end();) {
perhaps use "for (auto& it : animations)" to be consistent with the above.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.h:12: #include
"chrome/browser/android/vr_shell/vr_math.h"
Is this necessary here? Looks like you include vr_math both here and in cc file.
One of them is probably not necessary.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements_unittest.cc (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:34: TEST(Animation,
copy_rect) {
nit: s/copy_rect/CopyRect

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:34: TEST(Animation,
copy_rect) {
Rename "Animation" to "UiElements" since this is ui_elements_unittests?

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:39:
std::unique_ptr<Easing>(new easing::Linear()),
perhaps add test for other type of easing too?

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_gl.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl.h:3: // found in the LICENSE file.
perhaps use vr_gl_util as file name?

[cjgrant, 2016-09-13 17:32:16]

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/BUILD.gn (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/BUILD.gn:44: test("vr_shell_unittests") {
On 2016/09/12 15:38:26, bshe wrote:
> I assume you have a follow up CL that config bot to build and run these tests?
> It would be nice to file a bug and refer to it here.

Will do.  I'll read up on this.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/BUILD.gn:46: "animation.cc",
On 2016/09/12 15:38:26, bshe wrote:
> no unittest related file should be moved to vr_shell? And then you can
probably
> depend on vr_shell to pull the dependency. 

Good call.  I initially liked that the unit test target could compile without
vr_shell's dependencies, but this would get uglier as we add more tests.  And
this is the convention too.  Thanks.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/animation.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/animation.h:21: UNUSED, // Available for a
future type.
On 2016/09/12 14:18:27, mthiesse wrote:
> Add a comment for why this strangeness is here?

Done.  It'll be SCALE in a follow-on change.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/easing.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:10: #include "base/logging.h"
On 2016/09/12 15:38:26, bshe wrote:
> incorrect include? You probably just want base/macros.h

Done.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:11: #include
"ui/gfx/geometry/cubic_bezier.h"
On 2016/09/12 15:38:26, bshe wrote:
> move this include to .cc?

This is needed for the CubicBezier type.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:28: DISALLOW_COPY_AND_ASSIGN(Easing);
On 2016/09/12 15:38:26, bshe wrote:
> DISALLOW_COPY_AND_ASSIGN should be private

Done.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:38: gfx::CubicBezier bezier;
On 2016/09/12 15:38:26, bshe wrote:
> nit: s/bezier/bezier_

Done.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:46: double mPower;
On 2016/09/12 15:38:26, bshe wrote:
> nit: s/mPower/m_power_

Done (power_).

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/easing.h:54: double mPower;
On 2016/09/12 15:38:26, bshe wrote:
> nit: s/mPower/m_power_

Done.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.cc:173: for (auto it =
animations.begin(); it != animations.end();) {
On 2016/09/12 15:38:26, bshe wrote:
> perhaps use "for (auto& it : animations)" to be consistent with the above.

Here, we're managing the increment explicitly since elements are being removed. 
I don't know that we can use the shorthand version in this case.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.h:12: #include
"chrome/browser/android/vr_shell/vr_math.h"
On 2016/09/12 15:38:26, bshe wrote:
> Is this necessary here? Looks like you include vr_math both here and in cc
file.
> One of them is probably not necessary.

Done (in the next patch set).

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements_unittest.cc (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:34: TEST(Animation,
copy_rect) {
On 2016/09/12 15:38:26, bshe wrote:
> nit: s/copy_rect/CopyRect

Done.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:34: TEST(Animation,
copy_rect) {
On 2016/09/12 15:38:27, bshe wrote:
> Rename "Animation" to "UiElements" since this is ui_elements_unittests?

Done.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:39:
std::unique_ptr<Easing>(new easing::Linear()),
On 2016/09/12 15:38:26, bshe wrote:
> perhaps add test for other type of easing too?

To exercise easing more extensively, I'd prefer a dedicated easing unit tests,
but I didn't think were justified at this stage.  I check easing in a few cases
here to make sure its actually having an effect.  I can add easing tests to this
change, but would prefer to add them in a separate change.  Preference?

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_gl.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl.h:3: // found in the LICENSE file.
On 2016/09/12 15:38:27, bshe wrote:
> perhaps use vr_gl_util as file name?

I've been encouraged not to name files "util", as it implies a dumping ground. 
Personally I like vr_gl_util.  :)

[cjgrant, 2016-09-13 17:47:58]

cjgrant@chromium.org changed reviewers:
+ dtrainor@chromium.org

[cjgrant, 2016-09-13 17:49:49]

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements_unittest.cc (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:13: #define
EXPECT_VEC3F_EQ(a, b) \
David, I'm curious about your take on these macros.  I use them in place of
helper functions so that unit test failures are attributed to a meaningful line
number, rather than a line in a helper function.

[bshe, 2016-09-13 19:37:22]

lgtm

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements.cc:173: for (auto it =
animations.begin(); it != animations.end();) {
On 2016/09/13 17:32:16, cjgrant wrote:
> On 2016/09/12 15:38:26, bshe wrote:
> > perhaps use "for (auto& it : animations)" to be consistent with the above.
> 
> Here, we're managing the increment explicitly since elements are being
removed. 
> I don't know that we can use the shorthand version in this case.

hah. I see. Missed the erase here.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/ui_elements_unittest.cc (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:39:
std::unique_ptr<Easing>(new easing::Linear()),
On 2016/09/13 17:32:16, cjgrant wrote:
> On 2016/09/12 15:38:26, bshe wrote:
> > perhaps add test for other type of easing too?
> 
> To exercise easing more extensively, I'd prefer a dedicated easing unit tests,
> but I didn't think were justified at this stage.  I check easing in a few
cases
> here to make sure its actually having an effect.  I can add easing tests to
this
> change, but would prefer to add them in a separate change.  Preference?

Separate change sounds good.

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
File chrome/browser/android/vr_shell/vr_gl.h (right):

https://codereview.chromium.org/2335643002/diff/1/chrome/browser/android/vr_s...
chrome/browser/android/vr_shell/vr_gl.h:3: // found in the LICENSE file.
On 2016/09/13 17:32:16, cjgrant wrote:
> On 2016/09/12 15:38:27, bshe wrote:
> > perhaps use vr_gl_util as file name?
> 
> I've been encouraged not to name files "util", as it implies a dumping ground.

> Personally I like vr_gl_util.  :)

ack.

[David Trainor- moved to gerrit, 2016-09-14 21:43:30]

mostly nits with a question on whether or not Animation should be smarter :).

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/animation.h (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/animation.h:15: class Animation {
Can we add a class comment describing what this is and how it should be used?

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/easing.cc (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.cc:16: return bezier_.Solve(state);
Do we want to DCHECK or clamp state for all of these?

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/easing.h (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:12: enum EasingType {
Any reason to keep these outside the easing namespace?

Also do these types warrant a comment?

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:19: class Easing {
Should this just be called Interpolator?  At least that's what the the Android
version is called.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:21: virtual double
CalculateValue(double state) = 0;
Should we add a comment specifying the expected input range?  0 - 1?

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:23: protected:
new line before protected and private

Same for all other classes?

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:35: private:
I forget if you still need DISALLOW_COPY_AND_ASSIGN on the subclasses, but I
think generally it's added?

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:92: Animation& animation = *it;
Does it make sense to push this logic into Animation?

Random thoughts on this:
Could you build an Animation that knew how to modify a gvr::Vec3 or a Rectf? 
Then you could just call Animation::Animate(time) and let it do the magic
internally?

There are a lot of ways to flesh that out.  Just a thought.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.h:71: int content_texture_handle;
If we're specifying these in the header, should we default this one to something
as well?

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements_unittest.cc (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:13: #define
EXPECT_VEC3F_EQ(a, b) \
On 2016/09/13 17:49:49, cjgrant wrote:
> David, I'm curious about your take on these macros.  I use them in place of
> helper functions so that unit test failures are attributed to a meaningful
line
> number, rather than a line in a helper function.

I'm ok with them.  I don't know chrome's general stance but this seems clean
enough to me if you're getting useful log data out of it.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/vr_gl_util.h (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/vr_gl_util.h:1: // Copyright 2016 The Chromium
Authors. All rights reserved.
I'm assuming vr_gl_util is all copied from other places? :)

[mthiesse, 2016-09-14 22:15:21]

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:92: Animation& animation = *it;
On 2016/09/14 21:43:30, David Trainor wrote:
> Does it make sense to push this logic into Animation?
> 
> Random thoughts on this:
> Could you build an Animation that knew how to modify a gvr::Vec3 or a Rectf? 
> Then you could just call Animation::Animate(time) and let it do the magic
> internally?
> 
> There are a lot of ways to flesh that out.  Just a thought.
(Full disclosure, I wrote this code originally :P)

There are various problems with various ways of doing this. We want to avoid
circular dependencies between Animation and ContentRectangle, and we want
ContentRectangle to manage the lifetime of the Animation.

Animation::Animate(time) suggests that we have either many Animation
constructors, many Animation subclasses, or Animation depends on
ContentRectangle, and that we also maintain pointers to the things we're
animating in Animation, which seems like a major pain.

Animation::Animate(time, gvr::Vec3&/Rectf&/etc.) with a few overloads could
work, with Animation being constructed with from/to vectors, though I'm not sure
how much better the code would look as we'd still need a switch statement here
to figure out which property is being animated.

[cjgrant, 2016-09-16 17:47:08]

Thanks for the comments David!  PTAL when you have time...

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/animation.h (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/animation.h:15: class Animation {
On 2016/09/14 21:43:29, David Trainor wrote:
> Can we add a class comment describing what this is and how it should be used?

Done.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/easing.cc (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.cc:16: return bezier_.Solve(state);
On 2016/09/14 21:43:29, David Trainor wrote:
> Do we want to DCHECK or clamp state for all of these?

I refactored this to let the check live in the base class.  Let me know if you
like this better.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/easing.h (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:12: enum EasingType {
On 2016/09/14 21:43:29, David Trainor wrote:
> Any reason to keep these outside the easing namespace?
> 
> Also do these types warrant a comment?

Done.  Also, I removed the EasingType, as Easing itself doesn't need this (will
add it later when needed).

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:19: class Easing {
On 2016/09/14 21:43:30, David Trainor wrote:
> Should this just be called Interpolator?  At least that's what the the Android
> version is called.

It could be, but as per
http://www.w3schools.com/cssref/css3_pr_transition-timing-function.asp, "easing"
seems appropriate too (and maybe better, as it isn't Android specific).  If you
feel strongly about this, I'll certainly change it.  :)

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:21: virtual double
CalculateValue(double state) = 0;
On 2016/09/14 21:43:30, David Trainor wrote:
> Should we add a comment specifying the expected input range?  0 - 1?

Done.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:23: protected:
On 2016/09/14 21:43:30, David Trainor wrote:
> new line before protected and private
> 
> Same for all other classes?

Done.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:23: protected:
On 2016/09/14 21:43:30, David Trainor wrote:
> new line before protected and private
> 
> Same for all other classes?

Done.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:35: private:
On 2016/09/14 21:43:30, David Trainor wrote:
> I forget if you still need DISALLOW_COPY_AND_ASSIGN on the subclasses, but I
> think generally it's added?

Done.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.cc (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.cc:92: Animation& animation = *it;
On 2016/09/14 22:15:21, mthiesse wrote:
> On 2016/09/14 21:43:30, David Trainor wrote:
> > Does it make sense to push this logic into Animation?
> > 
> > Random thoughts on this:
> > Could you build an Animation that knew how to modify a gvr::Vec3 or a Rectf?

> > Then you could just call Animation::Animate(time) and let it do the magic
> > internally?
> > 
> > There are a lot of ways to flesh that out.  Just a thought.
> (Full disclosure, I wrote this code originally :P)
> 
> There are various problems with various ways of doing this. We want to avoid
> circular dependencies between Animation and ContentRectangle, and we want
> ContentRectangle to manage the lifetime of the Animation.
> 
> Animation::Animate(time) suggests that we have either many Animation
> constructors, many Animation subclasses, or Animation depends on
> ContentRectangle, and that we also maintain pointers to the things we're
> animating in Animation, which seems like a major pain.
> 
> Animation::Animate(time, gvr::Vec3&/Rectf&/etc.) with a few overloads could
> work, with Animation being constructed with from/to vectors, though I'm not
sure
> how much better the code would look as we'd still need a switch statement here
> to figure out which property is being animated.

Personally, I'd like to keep Animation as basically descriptor, leaving the
application to clients.  Later on, if we start duplicating application code,
we'll have a better sense of how to best generalize it here.  Is that okay?

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements.h (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements.h:71: int content_texture_handle;
On 2016/09/14 21:43:30, David Trainor wrote:
> If we're specifying these in the header, should we default this one to
something
> as well?

Done.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/ui_elements_unittest.cc (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/ui_elements_unittest.cc:13: #define
EXPECT_VEC3F_EQ(a, b) \
On 2016/09/14 21:43:30, David Trainor wrote:
> On 2016/09/13 17:49:49, cjgrant wrote:
> > David, I'm curious about your take on these macros.  I use them in place of
> > helper functions so that unit test failures are attributed to a meaningful
> line
> > number, rather than a line in a helper function.
> 
> I'm ok with them.  I don't know chrome's general stance but this seems clean
> enough to me if you're getting useful log data out of it.

Acknowledged.

[David Trainor- moved to gerrit, 2016-09-16 21:25:44]

lgtm.  Yeah I don't have a much cleaner idea for the Animation class yet.  If I
think of one I'll let you know.  Sorry for the delay I've been out sick these
past few days.

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
File chrome/browser/android/vr_shell/easing.h (right):

https://codereview.chromium.org/2335643002/diff/40001/chrome/browser/android/...
chrome/browser/android/vr_shell/easing.h:19: class Easing {
On 2016/09/16 17:47:08, cjgrant wrote:
> On 2016/09/14 21:43:30, David Trainor wrote:
> > Should this just be called Interpolator?  At least that's what the the
Android
> > version is called.
> 
> It could be, but as per
> http://www.w3schools.com/cssref/css3_pr_transition-timing-function.asp,
"easing"
> seems appropriate too (and maybe better, as it isn't Android specific).  If
you
> feel strongly about this, I'll certainly change it.  :)

Not particularly :).  Just a suggestion.

[cjgrant, 2016-09-19 13:55:29]

The CQ bit was checked by cjgrant@chromium.org

[cjgrant, 2016-09-19 13:55:30]

The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org
Link to the patchset: https://codereview.chromium.org/2335643002/#ps80001
(title: "Add VR Shell animation classes and unit test.")

[commit-bot: I haz the power, 2016-09-19 13:55:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-19 14:04:00]

Description was changed from

==========
Add VR Shell animation classes and unit test.

This change includes the animation and easing (intermediate state
calculation) capabilities developed by mthiesse@, and adds initial unit
tests for animation.

BUG=
==========

to

==========
Add VR Shell animation classes and unit test.

This change includes the animation and easing (intermediate state
calculation) capabilities developed by mthiesse@, and adds initial unit
tests for animation.

BUG=
==========

[commit-bot: I haz the power, 2016-09-19 14:04:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-09-19 14:06:26]

Description was changed from

==========
Add VR Shell animation classes and unit test.

This change includes the animation and easing (intermediate state
calculation) capabilities developed by mthiesse@, and adds initial unit
tests for animation.

BUG=
==========

to

==========
Add VR Shell animation classes and unit test.

This change includes the animation and easing (intermediate state
calculation) capabilities developed by mthiesse@, and adds initial unit
tests for animation.

BUG=

Committed: https://crrev.com/1ce4ff3d8c73bfd77e7f8d0135134f2524b4d480
Cr-Commit-Position: refs/heads/master@{#419449}
==========

[commit-bot: I haz the power, 2016-09-19 14:06:27]

Patchset 5 (id:??) landed as
https://crrev.com/1ce4ff3d8c73bfd77e7f8d0135134f2524b4d480
Cr-Commit-Position: refs/heads/master@{#419449}