Refactor app list scrolling: introduce the PaginationController class.

Refactor app list scrolling: introduce the PaginationController class.

PaginationController abstracts over mouse wheel, touch screen and
touchpad event handling for updating the PaginationModel. This allows
any client of PaginationModel (ie, ContentsView) to properly handle
scrolling events. It also removes the code duplication in OnMouseWheel
and OnScrollEvent.

BUG=406222
Committed: https://crrev.com/13cb53dbf92741bd9f432a734bb20bcb16e4ec51
Cr-Commit-Position: refs/heads/master@{#292843}

[Matt Giuca, 2014-08-29 06:21:33]

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

[calamity, 2014-09-01 01:49:30]

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
File ui/app_list/pagination_controller.cc (right):

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.cc:34: if (abs(offset.x()) > abs(offset.y()))
Ternary operator?

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
File ui/app_list/pagination_controller.h (right):

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.h:17: class ScrollEvent;
Unnecessary forward declaration.

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.h:39: bool OnScroll(const gfx::Point& offset);
This API seems kinda weird. The fact that OnGestureEvent takes an Event and
OnScroll doesn't means that the responsibility of marking the event as handled
is inconsistent across the class.

It would be better to use the same API style across these two methods.

https://codereview.chromium.org/524503003/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/524503003/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:998: static_cast<int>(event->y_offset()))))
{
Maybe gfx::ToFlooredPoint(gfx::PointF(event->x_offset(), event->y_offset()));

[Matt Giuca, 2014-09-01 03:02:26]

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
File ui/app_list/pagination_controller.cc (right):

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.cc:34: if (abs(offset.x()) > abs(offset.y()))
Mmkay (even though it would have been better to do this in a previous CL rather
than this, where it's just moving a bunch of code from somewhere else).

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
File ui/app_list/pagination_controller.h (right):

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.h:17: class ScrollEvent;
On 2014/09/01 01:49:30, calamity wrote:
> Unnecessary forward declaration.

Done.

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.h:39: bool OnScroll(const gfx::Point& offset);
I agree that this should return a bool instead of calling SetHandled(), for
consistency. But I think OnGestureEvent should still take an Event rather than
taking the pieces -- because it's fundamentally a more specialized and complex
thing (it's specifically for handling this set of event types).

https://codereview.chromium.org/524503003/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/524503003/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:998: static_cast<int>(event->y_offset()))))
{
On 2014/09/01 01:49:30, calamity wrote:
> Maybe gfx::ToFlooredPoint(gfx::PointF(event->x_offset(), event->y_offset()));

Done.

[Matt Giuca, 2014-09-01 04:23:27]

And decided to switch from Point to Vector2d for proper semantics.

[calamity, 2014-09-01 05:50:32]

lgtm

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
File ui/app_list/pagination_controller.h (right):

https://codereview.chromium.org/524503003/diff/20001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.h:39: bool OnScroll(const gfx::Point& offset);
On 2014/09/01 03:02:26, Matt Giuca wrote:
> I agree that this should return a bool instead of calling SetHandled(), for
> consistency. But I think OnGestureEvent should still take an Event rather than
> taking the pieces -- because it's fundamentally a more specialized and complex
> thing (it's specifically for handling this set of event types).

Ack.

https://codereview.chromium.org/524503003/diff/80001/ui/app_list/pagination_c...
File ui/app_list/pagination_controller.h (right):

https://codereview.chromium.org/524503003/diff/80001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.h:42: // PaginationModel. Returns true if the
event was handled.
'event was captured'? I'm nervous about using the word handled since
event->SetHandled() is a thing.

https://codereview.chromium.org/524503003/diff/80001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.h:43: bool OnGestureEvent(ui::GestureEvent*
event, const gfx::Rect& bounds);
event can be a const ref now.

[Matt Giuca, 2014-09-01 06:03:07]

https://codereview.chromium.org/524503003/diff/80001/ui/app_list/pagination_c...
File ui/app_list/pagination_controller.h (right):

https://codereview.chromium.org/524503003/diff/80001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.h:42: // PaginationModel. Returns true if the
event was handled.
On 2014/09/01 05:50:32, calamity wrote:
> 'event was captured'? I'm nervous about using the word handled since
> event->SetHandled() is a thing.

Done.

https://codereview.chromium.org/524503003/diff/80001/ui/app_list/pagination_c...
ui/app_list/pagination_controller.h:43: bool OnGestureEvent(ui::GestureEvent*
event, const gfx::Rect& bounds);
On 2014/09/01 05:50:32, calamity wrote:
> event can be a const ref now.

Done.

[Matt Giuca, 2014-09-01 06:05:34]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-09-01 06:06:30]

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

[commit-bot: I haz the power, 2014-09-01 07:02:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Matt Giuca, 2014-09-01 07:07:45]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-09-01 07:08:46]

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

[commit-bot: I haz the power, 2014-09-01 08:05:47]

Committed patchset #7 (id:120001) as e2fd050124aa12982e7febc9e8069e5b2d7f8eaa

[commit-bot: I haz the power, 2014-09-10 03:15:52]

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