App list: Added contents view custom animation framework.

App list: Added contents view custom animation framework.

Adds ContentsAnimator class. A ContentsAnimator subclass can implement
an animation between two states of the ContentsView. This will allow us
to have custom animations between arbitrary pages of the app launcher,
instead of just sliding. The default animation is just a slide.

BUG=425444
Committed: https://crrev.com/a723b068167b82d3757584ad456c2a5df92c344f
Cr-Commit-Position: refs/heads/master@{#303568}

[Matt Giuca, 2014-11-05 03:42:02]

mgiuca@chromium.org changed reviewers:
+ calamity@chromium.org

[calamity, 2014-11-05 06:29:07]

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:481: Close();
It's probably worth putting this in its own contents_view_unittest.cc given that
there are no views things going on here.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_animator.h (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:35: virtual void Update(double progress,
int from_page, int to_page) = 0;
Mention that the animations need to handle this linearly so that reversibility
is maintained.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:36: 
How are you planning to handle object creation/destruction (e.g the temporary
app icon views that will be used in the StartToApps animation)?

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:55: 
Destructor?

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:67: 
Destructor?

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:228: if (reverse)
Mention that easing is handled by pagination_model.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:248: bool* reverse) const {
Having this out var locks us into symmetrical animations. Using a
ReverseAnimation decorator on insertion would be more flexible if we need to
define non-symmetrical animations later. I don't really mind either way.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:165: void AddAnimator(int from_page, int
to_page, ContentsAnimator* animator);
If this takes ownership, shouldn't it take a scoped_ptr?

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:168: // the animator is should be run in
reverse (because it is a |to_page| to
s/is//

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:203: // The animator for a transition not
found in |contents_animators_|.
s/for a transition/for transitions/

[Matt Giuca, 2014-11-06 00:32:59]

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:481: Close();
I just tried doing this. It basically requires the full setup of
app_list_view_unittest: to create a ContentsView requires an AppListMainView, an
AppListModel and an AppListViewDelegate.

Creating a valid AppListMainView requires a properly initialised AppListView
which requires a full call to InitAsBubbleAtFixedLocation which requires a
NativeView as the parent.

I've gotten this far but it's still not working and I've basically ended up
copy+pasting nearly all of the initialization code from app_list_view_unittest
into contents_view_unittest. I think I've found the reason why ContentsView is
tested in app_list_view_unittest instead of having its own separate test file.

Let's just leave this test here please pretty please?

(ContentsView probably should not be so tightly coupled to the rest of the app
list... it is severely untestable right now.)

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_animator.h (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:35: virtual void Update(double progress,
int from_page, int to_page) = 0;
On 2014/11/05 06:29:07, calamity wrote:
> Mention that the animations need to handle this linearly so that reversibility
> is maintained.

Done.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:36: 
I hadn't thought about it. It's probably safe to store those things in the
ContentsAnimator subclass itself (given that there will only be one animation
active at a particular time).

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:55: 
On 2014/11/05 06:29:07, calamity wrote:
> Destructor?

Done.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:67: 
On 2014/11/05 06:29:07, calamity wrote:
> Destructor?

Done.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:228: if (reverse)
On 2014/11/05 06:29:07, calamity wrote:
> Mention that easing is handled by pagination_model.

Done.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:248: bool* reverse) const {
As discussed, I think this is a good idea if we need it. It's pretty easy to
change later (doesn't require changing the ContentsAnimator interface).

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:165: void AddAnimator(int from_page, int
to_page, ContentsAnimator* animator);
s/scoped_ptr/linked_ptr/ yeah... probably should.

But in doing so it kind of defeats most of the point of having this function,
which is to save as much repetition in the animation listing as possible.
Currently:

AddAnimator(GetPageIndexForState(AppListModel::STATE_START),
            GetPageIndexForState(AppListModel::STATE_APPS),
            new StartToAppsAnimator(this));

If I made it take a linked_ptr, it would mean each animation has to be written
as:

AddAnimator(GetPageIndexForState(AppListModel::STATE_START),
            GetPageIndexForState(AppListModel::STATE_APPS),
            linked_ptr<ContentsAnimator>(new StartToAppsAnimator(this)));

The purpose of AddAnimator is to make the animation insertion code as simple and
readable as possible. Up to you.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:168: // the animator is should be run in
reverse (because it is a |to_page| to
On 2014/11/05 06:29:07, calamity wrote:
> s/is//

Done.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:203: // The animator for a transition not
found in |contents_animators_|.
On 2014/11/05 06:29:07, calamity wrote:
> s/for a transition/for transitions/

Done.

[calamity, 2014-11-06 05:34:47]

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:481: Close();
On 2014/11/06 00:32:58, Matt Giuca wrote:
> I just tried doing this. It basically requires the full setup of
> app_list_view_unittest: to create a ContentsView requires an AppListMainView,
an
> AppListModel and an AppListViewDelegate.
> 
> Creating a valid AppListMainView requires a properly initialised AppListView

??? Why? AppListMainViewTest gets by without an AppListView. You can move the
test there instead if you want. I just don't want this test which literally does
nothing on 2/3 of its runs to be spinning up the views unit test stuff.

> which requires a full call to InitAsBubbleAtFixedLocation which requires a
> NativeView as the parent.
> 
> I've gotten this far but it's still not working and I've basically ended up
> copy+pasting nearly all of the initialization code from app_list_view_unittest
> into contents_view_unittest. I think I've found the reason why ContentsView is
> tested in app_list_view_unittest instead of having its own separate test file.
> 
> Let's just leave this test here please pretty please?
> 
> (ContentsView probably should not be so tightly coupled to the rest of the app
> list... it is severely untestable right now.)

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_animator.h (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:36: 
On 2014/11/06 00:32:58, Matt Giuca wrote:
> I hadn't thought about it. It's probably safe to store those things in the
> ContentsAnimator subclass itself (given that there will only be one animation
> active at a particular time).

I was more concerned about _when_ you would perform those tasks. I guess you can
just always clean up on progress == 1/0. Let me know if you think of a better
way.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:248: bool* reverse) const {
On 2014/11/06 00:32:59, Matt Giuca wrote:
> As discussed, I think this is a good idea if we need it. It's pretty easy to
> change later (doesn't require changing the ContentsAnimator interface).

Acknowledged.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:165: void AddAnimator(int from_page, int
to_page, ContentsAnimator* animator);
On 2014/11/06 00:32:59, Matt Giuca wrote:
> s/scoped_ptr/linked_ptr/ yeah... probably should.
> 
> But in doing so it kind of defeats most of the point of having this function,
> which is to save as much repetition in the animation listing as possible.
> Currently:
> 
> AddAnimator(GetPageIndexForState(AppListModel::STATE_START),
>             GetPageIndexForState(AppListModel::STATE_APPS),
>             new StartToAppsAnimator(this));
> 
> If I made it take a linked_ptr, it would mean each animation has to be written
> as:
> 
> AddAnimator(GetPageIndexForState(AppListModel::STATE_START),
>             GetPageIndexForState(AppListModel::STATE_APPS),
>             linked_ptr<ContentsAnimator>(new StartToAppsAnimator(this)));
> 
> The purpose of AddAnimator is to make the animation insertion code as simple
and
> readable as possible. Up to you.

I think it would still be scoped_ptr and then made into a linked_ptr to insert
it into the map. linked_ptr doesn't imply ownership transfer (since it's
copyable).

In fact, regarding readability, can we just make AddAnimator take a State?

I don't think

AddAnimator(AppListModel::STATE_START,
            AppListModel::STATE_APPS,
            scoped_ptr<ContentsAnimation>(new StartToAppsAnimator(this)));

is so bad.

[Matt Giuca, 2014-11-10 04:08:26]

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:481: Close();
On 2014/11/06 05:34:47, calamity wrote:
> On 2014/11/06 00:32:58, Matt Giuca wrote:
> > I just tried doing this. It basically requires the full setup of
> > app_list_view_unittest: to create a ContentsView requires an
AppListMainView,
> an
> > AppListModel and an AppListViewDelegate.
> > 
> > Creating a valid AppListMainView requires a properly initialised AppListView
> 
> ??? Why? AppListMainViewTest gets by without an AppListView. You can move the
> test there instead if you want. I just don't want this test which literally
does
> nothing on 2/3 of its runs to be spinning up the views unit test stuff.
> 
> > which requires a full call to InitAsBubbleAtFixedLocation which requires a
> > NativeView as the parent.
> > 
> > I've gotten this far but it's still not working and I've basically ended up
> > copy+pasting nearly all of the initialization code from
app_list_view_unittest
> > into contents_view_unittest. I think I've found the reason why ContentsView
is
> > tested in app_list_view_unittest instead of having its own separate test
file.
> > 
> > Let's just leave this test here please pretty please?
> > 
> > (ContentsView probably should not be so tightly coupled to the rest of the
app
> > list... it is severely untestable right now.)

Done.

(Thanks for the tip; it was non-obvious how to create a ContentsView for testing
but AppListMainViewTest shows how.)

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_animator.h (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:36: 
Yeah I started implementing it since this discussion. It was a bit messy. I
would initialize it on the first update I receive, and clean up the from_page
when progress == 1, clean up the to_page when progress == 0.

I could provide an init and cleanup method but it would get confusing with the
reversing. How about we add those as we need them.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:165: void AddAnimator(int from_page, int
to_page, ContentsAnimator* animator);
On 2014/11/06 05:34:47, calamity wrote:
> On 2014/11/06 00:32:59, Matt Giuca wrote:
> > s/scoped_ptr/linked_ptr/ yeah... probably should.
> > 
> > But in doing so it kind of defeats most of the point of having this
function,
> > which is to save as much repetition in the animation listing as possible.
> > Currently:
> > 
> > AddAnimator(GetPageIndexForState(AppListModel::STATE_START),
> >             GetPageIndexForState(AppListModel::STATE_APPS),
> >             new StartToAppsAnimator(this));
> > 
> > If I made it take a linked_ptr, it would mean each animation has to be
written
> > as:
> > 
> > AddAnimator(GetPageIndexForState(AppListModel::STATE_START),
> >             GetPageIndexForState(AppListModel::STATE_APPS),
> >             linked_ptr<ContentsAnimator>(new StartToAppsAnimator(this)));
> > 
> > The purpose of AddAnimator is to make the animation insertion code as simple
> and
> > readable as possible. Up to you.
> 
> I think it would still be scoped_ptr and then made into a linked_ptr to insert
> it into the map. linked_ptr doesn't imply ownership transfer (since it's
> copyable).
> 
> In fact, regarding readability, can we just make AddAnimator take a State?

Sort of depends on https://codereview.chromium.org/683703002/ (because before
that, there is no custom page state). Can we change it to a state later?

> I don't think
> 
> AddAnimator(AppListModel::STATE_START,
>             AppListModel::STATE_APPS,
>             scoped_ptr<ContentsAnimation>(new StartToAppsAnimator(this)));
> 
> is so bad.

Ack.

[calamity, 2014-11-10 05:18:56]

lgtm w/ a nit.

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
File ui/app_list/views/contents_animator.h (right):

https://codereview.chromium.org/701773002/diff/60001/ui/app_list/views/conten...
ui/app_list/views/contents_animator.h:36: 
On 2014/11/10 04:08:26, Matt Giuca wrote:
> Yeah I started implementing it since this discussion. It was a bit messy. I
> would initialize it on the first update I receive, and clean up the from_page
> when progress == 1, clean up the to_page when progress == 0.
> 
> I could provide an init and cleanup method but it would get confusing with the
> reversing. How about we add those as we need them.

Ack.

https://codereview.chromium.org/701773002/diff/140001/ui/app_list/views/conte...
File ui/app_list/views/contents_view_unittest.cc (right):

https://codereview.chromium.org/701773002/diff/140001/ui/app_list/views/conte...
ui/app_list/views/contents_view_unittest.cc:42:
scoped_ptr<app_list::SearchBoxView> search_box_view_;
These namespaces shouldn't be necessary.

[Matt Giuca, 2014-11-10 09:37:31]

https://codereview.chromium.org/701773002/diff/140001/ui/app_list/views/conte...
File ui/app_list/views/contents_view_unittest.cc (right):

https://codereview.chromium.org/701773002/diff/140001/ui/app_list/views/conte...
ui/app_list/views/contents_view_unittest.cc:42:
scoped_ptr<app_list::SearchBoxView> search_box_view_;
On 2014/11/10 05:18:55, calamity wrote:
> These namespaces shouldn't be necessary.

Done.

[Matt Giuca, 2014-11-10 09:37:36]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-10 09:37:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701773002/160001

[commit-bot: I haz the power, 2014-11-10 09:40:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-10 09:40:33]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Matt Giuca, 2014-11-10 23:10:54]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-10 23:12:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701773002/180001

[Matt Giuca, 2014-11-10 23:21:44]

The CQ bit was unchecked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-10 23:22:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-10 23:22:45]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2014-11-10 23:27:23]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-10 23:27:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701773002/180001

[Matt Giuca, 2014-11-10 23:37:31]

Take a quick look at PS 12?

https://codereview.chromium.org/701773002/diff/220001/ui/app_list/views/conte...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/701773002/diff/220001/ui/app_list/views/conte...
ui/app_list/views/contents_view.h:175: void AddAnimator(AppListModel::State
from_state,
Now that your CL has gone through, I've updated this to take a State rather than
an int. Happy?

(I didn't update GetAnimatorForTransition since it's hard to see what the
current state is.)

[Matt Giuca, 2014-11-10 23:37:43]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-10 23:38:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701773002/220001

[Matt Giuca, 2014-11-10 23:43:24]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2014-11-11 00:43:11]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-11 00:43:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701773002/220001

[Matt Giuca, 2014-11-11 00:46:37]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2014-11-11 00:50:23]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-11-11 00:52:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/701773002/240001

[calamity, 2014-11-11 01:26:52]

slgtm

[commit-bot: I haz the power, 2014-11-11 01:39:39]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2014-11-11 01:40:15]

Patchset 13 (id:??) landed as
https://crrev.com/a723b068167b82d3757584ad456c2a5df92c344f
Cr-Commit-Position: refs/heads/master@{#303568}