Vertically centered app list page switcher

Vertically centered app list page switcher

Change the app list page switcher from horizontally center to vertically
center. All changes are behind a flag. This is one part of moving to
full screen app list.

BUG=732067
TEST="Add --enable-features=EnableFullscreenAppList to see the changes"

Review-Url: https://codereview.chromium.org/2938563002
Cr-Commit-Position: refs/heads/master@{#479206}
Committed: https://chromium.googlesource.com/chromium/src/+/e081605169d6f3c605d52050a3a13522a473daf6

[weidongg, 2017-06-13 00:41:59]

weidongg@chromium.org changed reviewers:
+ xiyuan@chromium.org

[weidongg, 2017-06-13 00:41:59]

xiyuan@, please review the changes.

[xiyuan, 2017-06-13 18:29:55]

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/apps_grid...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/apps_grid...
ui/app_list/views/apps_grid_view.cc:7: #include
<ui/app_list/views/page_switcher_vertical.h>
Use " instead of < for chromium headers and move the two new ones with other
chromium headers.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
File ui/app_list/views/page_switcher_horizontal.cc (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_horizontal.cc:5: #include
<ui/app_list/views/page_switcher_horizontal.h>
Use " for chromium headers and use angle bracket < for standard C/C++/system
headers.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
File ui/app_list/views/page_switcher_horizontal.h (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_horizontal.h:22: public PageSwitcher {
nit: Put PageSwitcher in the first of the inheritance list since it is the
concrete base class and other ones are interfaces implemented.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
File ui/app_list/views/page_switcher_vertical.cc (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.cc:5: #include
<ui/app_list/views/page_switcher_vertical.h>
Use " for chromium headers

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.cc:78:
rect.ClampToCenteredSize(gfx::Size(kButtonWidth, button_height_));
This would not look the same as the UX mock. You will have follow CLs to change
this?

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.cc:144: int
PageSwitcherVertical::GetPageForPoint(const gfx::Point& point) const {
If we are using the same code for both switchers, this code (, other common
parts) and |buttons_| member can probably stay in the base class.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
File ui/app_list/views/page_switcher_vertical.h (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.h:22: public PageSwitcher {
nit: Make PageSwitcher the first one

[weidongg, 2017-06-13 21:25:00]

Thanks for the review.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/apps_grid...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/apps_grid...
ui/app_list/views/apps_grid_view.cc:7: #include
<ui/app_list/views/page_switcher_vertical.h>
On 2017/06/13 18:29:55, xiyuan wrote:
> Use " instead of < for chromium headers and move the two new ones with other
> chromium headers.

Done.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
File ui/app_list/views/page_switcher_horizontal.cc (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_horizontal.cc:5: #include
<ui/app_list/views/page_switcher_horizontal.h>
On 2017/06/13 18:29:55, xiyuan wrote:
> Use " for chromium headers and use angle bracket < for standard C/C++/system
> headers.

Done.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
File ui/app_list/views/page_switcher_horizontal.h (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_horizontal.h:22: public PageSwitcher {
On 2017/06/13 18:29:55, xiyuan wrote:
> nit: Put PageSwitcher in the first of the inheritance list since it is the
> concrete base class and other ones are interfaces implemented.

Done.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
File ui/app_list/views/page_switcher_vertical.cc (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.cc:5: #include
<ui/app_list/views/page_switcher_vertical.h>
On 2017/06/13 18:29:55, xiyuan wrote:
> Use " for chromium headers

Done.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.cc:78:
rect.ClampToCenteredSize(gfx::Size(kButtonWidth, button_height_));
On 2017/06/13 18:29:55, xiyuan wrote:
> This would not look the same as the UX mock. You will have follow CLs to
change
> this?

Yes, I will upload additional CLs to make changes based on the specs. This CL is
building the basic framework and separate new code from old code for convenience
of removing the old code in the future.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.cc:144: int
PageSwitcherVertical::GetPageForPoint(const gfx::Point& point) const {
On 2017/06/13 18:29:55, xiyuan wrote:
> If we are using the same code for both switchers, this code (, other common
> parts) and |buttons_| member can probably stay in the base class.

Ok, I see. But the both GetPageForPoint() and UpdateUIForDragPoint() use
PageSwitcherButton which is defined in anonymous namespace and will be different
for old and new code.
In the future, the old(horizontal) code will be completely removed and base code
may be not longer needed. I think it may be easier to do the refactor if we make
the base class lighter.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
File ui/app_list/views/page_switcher_vertical.h (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.h:22: public PageSwitcher {
On 2017/06/13 18:29:55, xiyuan wrote:
> nit: Make PageSwitcher the first one

Done.

[weidongg, 2017-06-13 21:25:01]

Thanks for the review.

[xiyuan, 2017-06-13 21:58:26]

lgtm

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
File ui/app_list/views/page_switcher_vertical.cc (right):

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.cc:78:
rect.ClampToCenteredSize(gfx::Size(kButtonWidth, button_height_));
On 2017/06/13 21:25:00, weidongg wrote:
> On 2017/06/13 18:29:55, xiyuan wrote:
> > This would not look the same as the UX mock. You will have follow CLs to
> change
> > this?
> 
> Yes, I will upload additional CLs to make changes based on the specs. This CL
is
> building the basic framework and separate new code from old code for
convenience
> of removing the old code in the future.

Acknowledged.

https://codereview.chromium.org/2938563002/diff/1/ui/app_list/views/page_swit...
ui/app_list/views/page_switcher_vertical.cc:144: int
PageSwitcherVertical::GetPageForPoint(const gfx::Point& point) const {
On 2017/06/13 21:25:00, weidongg wrote:
> On 2017/06/13 18:29:55, xiyuan wrote:
> > If we are using the same code for both switchers, this code (, other common
> > parts) and |buttons_| member can probably stay in the base class.
> 
> Ok, I see. But the both GetPageForPoint() and UpdateUIForDragPoint() use
> PageSwitcherButton which is defined in anonymous namespace and will be
different
> for old and new code.
> In the future, the old(horizontal) code will be completely removed and base
code
> may be not longer needed. I think it may be easier to do the refactor if we
make
> the base class lighter.

Acknowledged.

[weidongg, 2017-06-13 22:10:55]

The CQ bit was checked by weidongg@chromium.org

[commit-bot: I haz the power, 2017-06-13 22:11:42]

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-06-13 23:42:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497391855441730,
"parent_rev": "12a2797868d3c6eb531bf9321cc96b289530ec28", "commit_rev":
"e081605169d6f3c605d52050a3a13522a473daf6"}

[commit-bot: I haz the power, 2017-06-13 23:43:00]

Description was changed from

==========
Vertically centered app list page switcher

Change the app list page switcher from horizontally center to vertically
center. All changes are behind a flag. This is one part of moving to
full screen app list.

BUG=732067
TEST="Add --enable-features=EnableFullscreenAppList to see the changes"
==========

to

==========
Vertically centered app list page switcher

Change the app list page switcher from horizontally center to vertically
center. All changes are behind a flag. This is one part of moving to
full screen app list.

BUG=732067
TEST="Add --enable-features=EnableFullscreenAppList to see the changes"

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

[commit-bot: I haz the power, 2017-06-13 23:43:03]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e081605169d6f3c605d52050a3a1...

[newcomer, 2017-06-15 15:07:01]

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

[newcomer, 2017-06-15 15:07:02]

Weidong,

Please address these and include me in further CLs.

Thanks,
Alex

https://codereview.chromium.org/2938563002/diff/20001/ui/app_list/views/apps_...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/2938563002/diff/20001/ui/app_list/views/apps_...
ui/app_list/views/apps_grid_view.cc:235: if
(features::IsFullscreenAppListEnabled()) {
Call this once and cache it please.

https://codereview.chromium.org/2938563002/diff/20001/ui/app_list/views/apps_...
ui/app_list/views/apps_grid_view.cc:643: if
(features::IsFullscreenAppListEnabled()) {
Same as above.

https://codereview.chromium.org/2938563002/diff/20001/ui/app_list/views/apps_...
ui/app_list/views/apps_grid_view.cc:686: if
(features::IsFullscreenAppListEnabled()) {
Same as above.

[weidongg, 2017-06-15 19:37:02]

On 2017/06/15 15:07:02, newcomer wrote:
> Weidong,
> 
> Please address these and include me in further CLs.
> 
> Thanks,
> Alex
> 
>
https://codereview.chromium.org/2938563002/diff/20001/ui/app_list/views/apps_...
> File ui/app_list/views/apps_grid_view.cc (right):
> 
>
https://codereview.chromium.org/2938563002/diff/20001/ui/app_list/views/apps_...
> ui/app_list/views/apps_grid_view.cc:235: if
> (features::IsFullscreenAppListEnabled()) {
> Call this once and cache it please.
> 
>
https://codereview.chromium.org/2938563002/diff/20001/ui/app_list/views/apps_...
> ui/app_list/views/apps_grid_view.cc:643: if
> (features::IsFullscreenAppListEnabled()) {
> Same as above.
> 
>
https://codereview.chromium.org/2938563002/diff/20001/ui/app_list/views/apps_...
> ui/app_list/views/apps_grid_view.cc:686: if
> (features::IsFullscreenAppListEnabled()) {
> Same as above.

Thanks for the review, will make the change in another CL.