App list uses PaginationModel to transition between pages.

App list uses PaginationModel to transition between pages.

Should not affect behaviour of the normal app list. The experimental app
list now transitions pages in from the correct side of the screen,
instead of simply sliding pages out to the left.

BUG=377381
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278361

[Matt Giuca, 2014-06-06 04:36:25]

Slideys!

calamity: For full review please.
benwells: For OWNERS.

Note that we're using PaginationModel for normal as well as experimental app
list. It isn't necessary to do so, but it drastically simplifies the code --
otherwise we'd have to use BoundsAnimator for normal, and PaginationModel for
experimental.

One down-side of this is that BoundsAnimator appears to ease out the animation
(it slows down towards the end) whereas PaginationModel is linear. It's barely
noticeable. If we want to keep the easing, I think the best thing to do would be
to implement easing in PaginationModel rather than trying to use BoundsAnimator.

[Matt Giuca, 2014-06-06 05:56:46]

Actually, please don't review this. There are numerous issues (notably, the
performance is quite bad compared to BoundsAnimator) so it might have to change
a lot before being reviewed, or be unacceptable.

I will let you know when it should be reviewed.

[Matt Giuca, 2014-06-10 08:50:17]

OK, ready for review now!

The performance issue is fixed in https://codereview.chromium.org/326023002/ (in
the CQ now).

[calamity, 2014-06-12 03:05:08]

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

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:136: int current_page = std::max(0,
pagination_model_.selected_page());
When can this be negative?

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:174:
view_model_->set_ideal_bounds(target_page, incoming_target);
It looks like ideal bounds aren't really used anywhere else in this class but
semantically they denote where the view should end up on animation completion.

If that's the case, they should always be set by this method.

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:215: return;
nit: blank line after early return.

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

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:111: void CalculateAndSetBounds(bool ideal);
Doesn't the ideal=true case go against the method name since no bounds are
actually set?

Can we just move the SetViewBoundsToIdealBounds call into CalculateAndSetBounds
and change the argument to |animate| instead?

[calamity, 2014-06-12 03:26:54]

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

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:295:
contents_view->FinishCurrentAnimationForTests();
If Layout() sets everything to "where it ends up", shouldn't all animations be
cancelled on Layout()?

[Matt Giuca, 2014-06-16 00:59:05]

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

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:136: int current_page = std::max(0,
pagination_model_.selected_page());
pagination_model_.selected_page() is -1 by default, which means "the default
value" which as far as I can tell always means 0. Not sure why it is this way,
and doesn't just default to 0, but there is a common idiom to use std::max(0,
pagination_model_.selected_page()) to get the "real" selected page.

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:174:
view_model_->set_ideal_bounds(target_page, incoming_target);
On 2014/06/12 03:05:08, calamity wrote:
> It looks like ideal bounds aren't really used anywhere else in this class but
> semantically they denote where the view should end up on animation completion.

I guess if something isn't used at all, but semantically represents something,
it probably should be removed entirely.

It actually is used, though, just at initialization time, Layout() is called
which sets everything to its ideal position, so I do need this (or more
refactoring is required).

> If that's the case, they should always be set by this method.

I don't see why the ideal bounds should always be set. Conceptually, this method
is just two separate functions (one sets the ideal bounds, the other sets the
actual bounds), but since they share almost all the code, they are merged into
one.

Having this always set the ideal bounds would break the code reuse because I'd
have to do the main computation twice.

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:215: return;
On 2014/06/12 03:05:08, calamity wrote:
> nit: blank line after early return.

Done.

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

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:111: void CalculateAndSetBounds(bool ideal);
On 2014/06/12 03:05:08, calamity wrote:
> Doesn't the ideal=true case go against the method name since no bounds are
> actually set?

The implication was that "SetBounds" was non-specific about whether the bounds
being set were ideal or actual, and the Boolean flag clears that up.
> 
> Can we just move the SetViewBoundsToIdealBounds call into
CalculateAndSetBounds
> and change the argument to |animate| instead?

I don't see how that would work. Doesn't that mean that if animate=true, we
would be setting the ideal bounds to the partial position during the animation?

[Matt Giuca, 2014-06-16 07:20:28]

OK after a lot of nitty fixes (rewriting Layout mostly) and testing, ready for
another look. Thanks!

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

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:295:
contents_view->FinishCurrentAnimationForTests();
Good point. They previously weren't. Now Layout does end all animations, so we
don't need FinishCurrentAnimationForTests.

Note that I had to rewrite Layout to use completely different code to
UpdatePageBounds, since we need to move ALL the pages out of the way, not just
the current begin and end page (because there could be a three-way animation, it
isn't enough to just move the begin and end pages).

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

https://codereview.chromium.org/316393002/diff/80001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:111: void CalculateAndSetBounds(bool ideal);
OK as discussed, no more ideal bounds. This method always sets the bounds
directly; the argument determines whether it ignores the animation or not. (Also
renamed the method.)

[calamity, 2014-06-17 05:43:45]

I believe view_model_ is just copying the child view order in ContentsView now.
It should be possible to remove it completely in a follow-up patch.

lgtm w/ nits.

https://codereview.chromium.org/316393002/diff/250001/ui/app_list/views/conte...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/316393002/diff/250001/ui/app_list/views/conte...
ui/app_list/views/contents_view.cc:162: // the state of the PaginationModel).
This comment is a bit weird now since this is only called on a transition
change. It may even be worth moving this all into the TransitionChanged()
handler below.

https://codereview.chromium.org/316393002/diff/250001/ui/app_list/views/conte...
ui/app_list/views/contents_view.cc:268: view->SetBoundsRect(offscreen_target);
Consider ternary operator.

[Matt Giuca, 2014-06-17 06:04:07]

https://codereview.chromium.org/316393002/diff/250001/ui/app_list/views/conte...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/316393002/diff/250001/ui/app_list/views/conte...
ui/app_list/views/contents_view.cc:162: // the state of the PaginationModel).
To the second point: thought about it, but I think this method is generally
useful whenever you want to say "put everything in the right place". Let's not
constrain it to just be when the transition updates.

To the first point: since this could potentially be called not in the context of
an animation (and in fact, the final frame will be when there is no transition),
I think the comment is still valid.

https://codereview.chromium.org/316393002/diff/250001/ui/app_list/views/conte...
ui/app_list/views/contents_view.cc:268: view->SetBoundsRect(offscreen_target);
On 2014/06/17 05:43:45, calamity wrote:
> Consider ternary operator.

Done.

[benwells, 2014-06-19 00:31:44]

had a quick glance. lgtm.

[Matt Giuca, 2014-06-19 00:32:29]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-06-19 00:33:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/316393002/270001

[Matt Giuca, 2014-06-19 01:10:16]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-06-19 01:11:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/316393002/290001

[commit-bot: I haz the power, 2014-06-19 14:31:57]

Change committed as 278361