New AppListView Scroll Behavior.

New AppListView Scroll Behavior.

The old AppListView did not respond to scroll events from PEEKING mode.
Now the AppListView transitions from PEEKING to FULLSCREEN_ALL_APPS when
a scroll event (mousewheel or touchpad gesture) is received via
AppListView::HandleScroll.

I had to make a fix to ensure that events are properly propogating, and
I had to explicitly pass MouseWheel events from the SearchBoxView
widget.

New Tests:
MouseWheel Scroll events from the SearchBoxView from PEEKING.
MouseWheel scroll events from the AppListView shield from PEEKING.
Ditto, from other states.

BUG=739408
Review-Url: https://codereview.chromium.org/2982453002
Cr-Commit-Position: refs/heads/master@{#487222}
Committed: https://chromium.googlesource.com/chromium/src/+/b8a3badb43f071f13b420aeacccf818cbf5e9bef

[newcomer, 2017-07-11 17:42:15]

Description was changed from

==========
New AppListView Scroll Behavior.

The old AppListView did not respond to scroll events from PEEKING mode.
Now the AppListView transitions from PEEKING to FULLSCREEN_ALL_APPS when
a scroll event (mousewheel or touchpad gesture) is received via
AppListView::HandleScroll.

I had to make a fix to ensure that events are properly propogating, and
I had to explicitly pass MouseWheel events from the SearchBoxView
widget.

New Tests:
MouseWheel Scroll events from the SearchBoxView from PEEKING.
MouseWheel scroll events from the AppListView shield from PEEKING.
Ditto, from other states.
==========

to

==========
New AppListView Scroll Behavior.

The old AppListView did not respond to scroll events from PEEKING mode.
Now the AppListView transitions from PEEKING to FULLSCREEN_ALL_APPS when
a scroll event (mousewheel or touchpad gesture) is received via
AppListView::HandleScroll.

I had to make a fix to ensure that events are properly propogating, and
I had to explicitly pass MouseWheel events from the SearchBoxView
widget.

New Tests:
MouseWheel Scroll events from the SearchBoxView from PEEKING.
MouseWheel scroll events from the AppListView shield from PEEKING.
Ditto, from other states.

BUG=739408
==========

[newcomer, 2017-07-11 17:42:16]

newcomer@chromium.org changed reviewers:
+ vadimt@chromium.org

[newcomer, 2017-07-11 17:42:34]

Vadim, please take a look.

-Alex

[vadimt, 2017-07-11 23:32:57]

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_page.h (right):

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_page.h:71: ContentsView* contents_view_;
Please replace making protected with an accessor, as was discussed.

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_view.cc:680: break;
No need to break after unconditional return.

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/search_bo...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/search_bo...
ui/app_list/views/search_box_view.cc:464: return false;
Empty line after.

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/search_bo...
ui/app_list/views/search_box_view.cc:466: return
app_list_view_->HandleScroll(event);
HandleScroll(&mouse_event) ?

https://codereview.chromium.org/2982453002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2982453002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:639: return (controller_ &&
controller_->HandleMouseEvent(this, event));
No braces.

[newcomer, 2017-07-12 16:56:00]

Responded to comments!

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_page.h (right):

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_page.h:71: ContentsView* contents_view_;
On 2017/07/11 23:32:56, vadimt wrote:
> Please replace making protected with an accessor, as was discussed.

Done.

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/app_list_...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/app_list_...
ui/app_list/views/app_list_view.cc:680: break;
On 2017/07/11 23:32:56, vadimt wrote:
> No need to break after unconditional return.

Done.

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/search_bo...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/search_bo...
ui/app_list/views/search_box_view.cc:464: return false;
On 2017/07/11 23:32:56, vadimt wrote:
> Empty line after.

Done.

https://codereview.chromium.org/2982453002/diff/1/ui/app_list/views/search_bo...
ui/app_list/views/search_box_view.cc:466: return
app_list_view_->HandleScroll(event);
On 2017/07/11 23:32:56, vadimt wrote:
> HandleScroll(&mouse_event) ?

Done.

https://codereview.chromium.org/2982453002/diff/1/ui/views/controls/textfield...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/2982453002/diff/1/ui/views/controls/textfield...
ui/views/controls/textfield/textfield.cc:639: return (controller_ &&
controller_->HandleMouseEvent(this, event));
On 2017/07/11 23:32:56, vadimt wrote:
> No braces.

Done.

[vadimt, 2017-07-12 18:28:02]

lgtm

https://codereview.chromium.org/2982453002/diff/20001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_page.h (right):

https://codereview.chromium.org/2982453002/diff/20001/ui/app_list/views/app_l...
ui/app_list/views/app_list_page.h:71: ContentsView* contents_view() const {
return contents_view_; }
My suggestion was to have a method returning contents_view()->IsStateActive(
         AppListModel::STATE_CUSTOM_LAUNCHER_PAGE);	      
AppListModel::STATE_CUSTOM_LAUNCHER_PAGE);

And not exposing contents_view directly...

Does this make sense?

[newcomer, 2017-07-13 23:37:16]

newcomer@chromium.org changed reviewers:
+ oshima@chromium.org

[newcomer, 2017-07-13 23:37:17]

Oshima,

Please take a look.

-Alex

https://codereview.chromium.org/2982453002/diff/20001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_page.h (right):

https://codereview.chromium.org/2982453002/diff/20001/ui/app_list/views/app_l...
ui/app_list/views/app_list_page.h:71: ContentsView* contents_view() const {
return contents_view_; }
On 2017/07/12 18:28:02, vadimt wrote:
> My suggestion was to have a method returning contents_view()->IsStateActive(
>          AppListModel::STATE_CUSTOM_LAUNCHER_PAGE);	      
> AppListModel::STATE_CUSTOM_LAUNCHER_PAGE);
> 
> And not exposing contents_view directly...
> 
> Does this make sense?

Done.

[newcomer, 2017-07-13 23:37:52]

newcomer@chromium.org changed reviewers:
+ khmel@chromium.org

[newcomer, 2017-07-13 23:37:52]

Adding Khmel@

Please take a look.

-Alex

[oshima, 2017-07-14 02:06:47]

https://codereview.chromium.org/2982453002/diff/40001/ash/app_list/app_list_p...
File ash/app_list/app_list_presenter_delegate_unittest.cc (right):

https://codereview.chromium.org/2982453002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:598: // swipe down, the app
list should return to peeking mode.
Swipe

https://codereview.chromium.org/2982453002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:599: gfx::Point start =
gfx::Point(0, 0);
start(0, 0)

https://codereview.chromium.org/2982453002/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_page.cc (right):

https://codereview.chromium.org/2982453002/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_page.cc:67: bool
AppListPage::is_custom_launcher_page_active() {
IsCustomLauncherPageActive()

shouldn't this be const?

https://codereview.chromium.org/2982453002/diff/40001/ui/app_list/views/searc...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2982453002/diff/40001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:32: #include
"ui/gfx/geometry/vector2d_conversions.h"
do you need this?

[newcomer, 2017-07-14 17:49:33]

Responded to Oshima@'s comments!

-Alex

https://codereview.chromium.org/2982453002/diff/40001/ash/app_list/app_list_p...
File ash/app_list/app_list_presenter_delegate_unittest.cc (right):

https://codereview.chromium.org/2982453002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:598: // swipe down, the app
list should return to peeking mode.
On 2017/07/14 02:06:47, oshima wrote:
> Swipe

Done.

https://codereview.chromium.org/2982453002/diff/40001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:599: gfx::Point start =
gfx::Point(0, 0);
On 2017/07/14 02:06:47, oshima wrote:
> start(0, 0)

Done.

https://codereview.chromium.org/2982453002/diff/40001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_page.cc (right):

https://codereview.chromium.org/2982453002/diff/40001/ui/app_list/views/app_l...
ui/app_list/views/app_list_page.cc:67: bool
AppListPage::is_custom_launcher_page_active() {
On 2017/07/14 02:06:47, oshima wrote:
> IsCustomLauncherPageActive()
> 
> shouldn't this be const?

I did it in the header and left it out of the cc file. Thanks!

https://codereview.chromium.org/2982453002/diff/40001/ui/app_list/views/searc...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2982453002/diff/40001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:32: #include
"ui/gfx/geometry/vector2d_conversions.h"
On 2017/07/14 02:06:47, oshima wrote:
> do you need this?

Not any more. Thanks!

[oshima, 2017-07-14 17:58:14]

lgtm

[khmel, 2017-07-14 18:10:01]

lgtm after nits are adressed.

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
File ash/app_list/app_list_presenter_delegate_unittest.cc (right):

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:574: if (!GetParam())
nit: can you use test_with_fullscreen_ instead?

If not please do something:
const bool full_screen = !GetParam() and reuse it at L 578

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:578: bool test_mouse_event
= GetParam();
nit: const bool ... or discard it in regards to prev comment

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:600:
generator.GestureScrollSequence(start, gfx::Point(0, 720),
nit: please inline (gfx::Point(0, 0), gfx::Point(0, 720))

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:605: gfx::Point
outside_search_box = view->GetBoundsInScreen().origin();
nit: const gfx::...

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/pagination_...
File ui/app_list/pagination_controller.cc (right):

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/pagination_...
ui/app_list/pagination_controller.cc:44: if (abs(offset_magnitude) >
kMinScrollToSwitchPage &&
nit: I would not do just only style fix and prefer to leave file un-touch to
simplify the review. Have no strong pref here. You may also leave it.

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:682: int offset =
event->AsScrollEvent()->y_offset();
 nit const int or even better inline

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:735: if (HandleScroll(event)) {
nit: 
we prefer following style:
if (!HandleScroll(event))
  return;
....

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/searc...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:364: if
(app_list_view_->HandleScroll(event))
nit:
  if (!app_list_view_->HandleScroll(event))
     return;

[newcomer, 2017-07-14 22:38:12]

The CQ bit was checked by newcomer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-14 22:38:19]

Dry run: 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, 2017-07-14 22:41:31]

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

[commit-bot: I haz the power, 2017-07-14 22:41:32]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[newcomer, 2017-07-17 15:41:23]

The CQ bit was checked by newcomer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-17 15:41:36]

Dry run: 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, 2017-07-17 16:59:41]

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

[commit-bot: I haz the power, 2017-07-17 16:59:42]

Dry run: This issue passed the CQ dry run.

[newcomer, 2017-07-17 18:07:23]

The CQ bit was checked by newcomer@chromium.org

[newcomer, 2017-07-17 18:07:24]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimt@chromium.org,
oshima@chromium.org, khmel@chromium.org
Link to the patchset: https://codereview.chromium.org/2982453002/#ps140001
(title: "New AppListView Scroll Behavior.")

[commit-bot: I haz the power, 2017-07-17 18:07:35]

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, 2017-07-17 18:16:39]

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

[commit-bot: I haz the power, 2017-07-17 18:16:41]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[newcomer, 2017-07-17 18:44:41]

Patchset #9 (id:160001) has been deleted

[newcomer, 2017-07-17 18:44:49]

Patchset #8 (id:140001) has been deleted

[newcomer, 2017-07-17 18:45:03]

The CQ bit was checked by newcomer@chromium.org

[newcomer, 2017-07-17 18:45:05]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimt@chromium.org,
oshima@chromium.org, khmel@chromium.org
Link to the patchset: https://codereview.chromium.org/2982453002/#ps180001
(title: "Removed unintended edit.")

[commit-bot: I haz the power, 2017-07-17 18:45:20]

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, 2017-07-17 20:22:40]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1500317103739590,
"parent_rev": "9e40c598d313158558cdf73275cfeb7a7ae03a83", "commit_rev":
"b8a3badb43f071f13b420aeacccf818cbf5e9bef"}

[commit-bot: I haz the power, 2017-07-17 20:22:56]

Description was changed from

==========
New AppListView Scroll Behavior.

The old AppListView did not respond to scroll events from PEEKING mode.
Now the AppListView transitions from PEEKING to FULLSCREEN_ALL_APPS when
a scroll event (mousewheel or touchpad gesture) is received via
AppListView::HandleScroll.

I had to make a fix to ensure that events are properly propogating, and
I had to explicitly pass MouseWheel events from the SearchBoxView
widget.

New Tests:
MouseWheel Scroll events from the SearchBoxView from PEEKING.
MouseWheel scroll events from the AppListView shield from PEEKING.
Ditto, from other states.

BUG=739408
==========

to

==========
New AppListView Scroll Behavior.

The old AppListView did not respond to scroll events from PEEKING mode.
Now the AppListView transitions from PEEKING to FULLSCREEN_ALL_APPS when
a scroll event (mousewheel or touchpad gesture) is received via
AppListView::HandleScroll.

I had to make a fix to ensure that events are properly propogating, and
I had to explicitly pass MouseWheel events from the SearchBoxView
widget.

New Tests:
MouseWheel Scroll events from the SearchBoxView from PEEKING.
MouseWheel scroll events from the AppListView shield from PEEKING.
Ditto, from other states.

BUG=739408

Review-Url: https://codereview.chromium.org/2982453002
Cr-Commit-Position: refs/heads/master@{#487222}
Committed:
https://chromium.googlesource.com/chromium/src/+/b8a3badb43f071f13b420aeacccf...
==========

[commit-bot: I haz the power, 2017-07-17 20:22:58]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/b8a3badb43f071f13b420aeacccf...

[newcomer, 2017-07-17 21:38:11]

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
File ash/app_list/app_list_presenter_delegate_unittest.cc (right):

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:574: if (!GetParam())
On 2017/07/14 18:10:00, khmel wrote:
> nit: can you use test_with_fullscreen_ instead?
> 
> If not please do something:
> const bool full_screen = !GetParam() and reuse it at L 578

Fixed by putting this test into FullscreenAppListPresenterDelegateTest

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:578: bool test_mouse_event
= GetParam();
On 2017/07/14 18:10:00, khmel wrote:
> nit: const bool ... or discard it in regards to prev comment

Done.

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:600:
generator.GestureScrollSequence(start, gfx::Point(0, 720),
On 2017/07/14 18:10:00, khmel wrote:
> nit: please inline (gfx::Point(0, 0), gfx::Point(0, 720))

Done.

https://codereview.chromium.org/2982453002/diff/60001/ash/app_list/app_list_p...
ash/app_list/app_list_presenter_delegate_unittest.cc:605: gfx::Point
outside_search_box = view->GetBoundsInScreen().origin();
On 2017/07/14 18:10:01, khmel wrote:
> nit: const gfx::...

Refactored test after rebase to use GetPointOutside/InsideSearchBox()

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/pagination_...
File ui/app_list/pagination_controller.cc (right):

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/pagination_...
ui/app_list/pagination_controller.cc:44: if (abs(offset_magnitude) >
kMinScrollToSwitchPage &&
On 2017/07/14 18:10:01, khmel wrote:
> nit: I would not do just only style fix and prefer to leave file un-touch to
> simplify the review. Have no strong pref here. You may also leave it.


I actually am fixing a bug here (returning true inside the if statement, else
returning false). This conditional was preventing scroll events from propogating
to AppListView.

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/app_l...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:682: int offset =
event->AsScrollEvent()->y_offset();
On 2017/07/14 18:10:01, khmel wrote:
>  nit const int or even better inline

Done.

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/app_l...
ui/app_list/views/app_list_view.cc:735: if (HandleScroll(event)) {
On 2017/07/14 18:10:01, khmel wrote:
> nit: 
> we prefer following style:
> if (!HandleScroll(event))
>   return;
> ....

Done.

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/searc...
File ui/app_list/views/search_box_view.cc (right):

https://codereview.chromium.org/2982453002/diff/60001/ui/app_list/views/searc...
ui/app_list/views/search_box_view.cc:364: if
(app_list_view_->HandleScroll(event))
On 2017/07/14 18:10:01, khmel wrote:
> nit:
>   if (!app_list_view_->HandleScroll(event))
>      return;

Done.