AppListController now responsible for ensuring app list stays centered.

AppListController now responsible for ensuring app list stays centered.

Just a refactor; effectively reverting SVN 266176 and rewriting it so
that AppListController re-centers the app list when the screen bounds
change (AppListView no longer needs to do this itself).

Rationale: This architecture is simpler. On subsequent CLs I was having
trouble plumbing more state (specifically, the virtual keyboard state)
down into AppListView; this will mean we can do it from
AppListController which has easy access to the virtual keyboard.

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

[Matt Giuca, 2014-04-30 07:44:56]

Hey guys, sorry to do this -- you just reviewed the previous CL and now I'm
reverting it and doing it in a totally different way.

Trent convinced me to do it this way; it makes subsequent CLs relating to app
list and Ash virtual keyboard MUCH simpler (no longer having to plumb
Ash-specific keyboard stuff through the main app list views).

jamescook: ash/wm/app_list_controller.*
xiyuan: ui/app_list/views/app_list_view.* (just reverting)

[xiyuan, 2014-04-30 16:26:47]

I like it.

LGTM++

[varkha, 2014-04-30 16:37:37]

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:174: if
(app_list::switches::IsExperimentalAppListPositionEnabled()) {
nit: maybe:
is_centered_ = app_list::switches::IsExperimentalAppListPositionEnabled();
if (is_centered_) {

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:180:
Shell::GetScreen()->GetPrimaryDisplay().bounds().CenterPoint(),
Could use GetScreenCenter().

[James Cook, 2014-04-30 16:50:05]

LGTM assuming varkha's comments addressed

[tapted, 2014-05-01 00:51:10]

lgtm

[Matt Giuca, 2014-05-01 03:54:32]

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:174: if
(app_list::switches::IsExperimentalAppListPositionEnabled()) {
On 2014/04/30 16:37:38, varkha wrote:
> nit: maybe:
> is_centered_ = app_list::switches::IsExperimentalAppListPositionEnabled();
> if (is_centered_) {

Done. (Nice)

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:180:
Shell::GetScreen()->GetPrimaryDisplay().bounds().CenterPoint(),
I can't find anything called GetScreenCenter or ScreenCenter in the Chrome
codebase...

[Matt Giuca, 2014-05-01 03:54:55]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-05-01 03:55:28]

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

[varkha, 2014-05-01 03:57:21]

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:180:
Shell::GetScreen()->GetPrimaryDisplay().bounds().CenterPoint(),
On 2014/05/01 03:54:32, Matt Giuca wrote:
> I can't find anything called GetScreenCenter or ScreenCenter in the Chrome
> codebase...

It's a new method in this file's anonymous namespace that you added - line 116
(?)

[Matt Giuca, 2014-05-01 03:59:41]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2014-05-01 03:59:58]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2014-05-01 04:02:42]

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:180:
Shell::GetScreen()->GetPrimaryDisplay().bounds().CenterPoint(),
/me hits self over head with hammer.

Sorry, I must have been searching in the wrong branch. I completely forgot about
this, but in fact the entire *point* of abstracting out that method was to use
it here (and in the other place). Thanks for pointing this out.

[Matt Giuca, 2014-05-01 04:03:09]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-05-01 04:03:53]

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

[varkha, 2014-05-01 04:04:49]

lgtm.

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.cc
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/260663002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:180:
Shell::GetScreen()->GetPrimaryDisplay().bounds().CenterPoint(),
On 2014/05/01 04:02:43, Matt Giuca wrote:
> /me hits self over head with hammer.
> 
> Sorry, I must have been searching in the wrong branch. I completely forgot
about
> this, but in fact the entire *point* of abstracting out that method was to use
> it here (and in the other place). Thanks for pointing this out.

Great, you got it in time.

[commit-bot: I haz the power, 2014-05-01 05:02:31]

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

[commit-bot: I haz the power, 2014-05-01 05:02:32]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Matt Giuca, 2014-05-01 08:24:44]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-05-01 08:25:02]

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

[commit-bot: I haz the power, 2014-05-01 10:20:10]

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

[commit-bot: I haz the power, 2014-05-01 10:20:11]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium

[Matt Giuca, 2014-05-02 00:30:27]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-05-02 00:31:32]

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

[commit-bot: I haz the power, 2014-05-02 01:36:42]

Change committed as 267720