ChromeOS: The centered app list's top is now forced to be >= 10.

ChromeOS: The centered app list's top is now forced to be >= 10.

Previously, in small space environments (e.g., low resolution or big
virtual keyboard), the app list's top would disappear off screen. Now,
it is forced below the screen top (with a 10px margin), even if it means
forcing the bottom further off screen.

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

[Matt Giuca, 2014-05-12 06:34:23]

See discussion on Issue 371681 (from #12 to #14) for the rationale.

We will want to merge this into M36.

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
File ash/wm/app_list_controller_unittest.cc (right):

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
ash/wm/app_list_controller_unittest.cc:149: TEST_P(AppListControllerTest,
TinyDisplay) {
Note: This test fails without CL 283443003 because even when GetParam() is true,
it runs the non-centered app list (which doesn't behave the way this test
expects). So this CL has a hard dependency on that one.

[Matt Giuca, 2014-05-13 02:44:37]

Attached before and after screenshots (with a hypothetically huge keyboard) on
the bug:
https://code.google.com/p/chromium/issues/detail?id=371681#c23

[varkha, 2014-05-13 03:25:30]

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

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:219: applist_button,
GetMinimumBoundsHeightForAppList(view)));
Would it be clearer if s/applist_button/view?

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
File ash/wm/app_list_controller_unittest.cc (right):

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
ash/wm/app_list_controller_unittest.cc:149: TEST_P(AppListControllerTest,
TinyDisplay) {
(Should this test and maybe other tests be disabled when the screen is not
resizable?
  if (!SupportsHostWindowResize())
    return;

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
ash/wm/app_list_controller_unittest.cc:154: if (!GetParam())
Can we wrap this in a self-descriptive method (bool
AppListControllerTest::use_centered_app_list()) rather than use GetParam().

[Matt Giuca, 2014-05-13 04:02:12]

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

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:219: applist_button,
GetMinimumBoundsHeightForAppList(view)));
On 2014/05/13 03:25:31, varkha wrote:
> Would it be clearer if s/applist_button/view?

Yes, but it *has* to be the button, or we regress http://crbug.com/368974. The
reason we use the button is because we want the app launcher to open on the same
display that the button that the user clicked is on.

If we use view here (which would nicely simplify the code), we would create the
view at (0, 0) and then move it to the center of the display with (0, 0) on it.

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
File ash/wm/app_list_controller_unittest.cc (right):

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
ash/wm/app_list_controller_unittest.cc:149: TEST_P(AppListControllerTest,
TinyDisplay) {
Why? This test doesn't use window resizing, it just sets up an initial display
size for testing.

Note: That other CL is in now.

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
ash/wm/app_list_controller_unittest.cc:154: if (!GetParam())
On 2014/05/13 03:25:31, varkha wrote:
> Can we wrap this in a self-descriptive method (bool
> AppListControllerTest::use_centered_app_list()) rather than use GetParam().

Done.

[varkha, 2014-05-13 04:40:11]

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

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller.c...
ash/wm/app_list_controller.cc:219: applist_button,
GetMinimumBoundsHeightForAppList(view)));
On 2014/05/13 04:02:12, Matt Giuca wrote:
> On 2014/05/13 03:25:31, varkha wrote:
> > Would it be clearer if s/applist_button/view?
> 
> Yes, but it *has* to be the button, or we regress http://crbug.com/368974. The
> reason we use the button is because we want the app launcher to open on the
same
> display that the button that the user clicked is on.
> 
> If we use view here (which would nicely simplify the code), we would create
the
> view at (0, 0) and then move it to the center of the display with (0, 0) on
it.

I see. I guess UpdateBounds never moves to another display so that's OK.

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
File ash/wm/app_list_controller_unittest.cc (right):

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
ash/wm/app_list_controller_unittest.cc:149: TEST_P(AppListControllerTest,
TinyDisplay) {
On 2014/05/13 04:02:12, Matt Giuca wrote:
> Why? This test doesn't use window resizing, it just sets up an initial display
> size for testing.
> 
> Note: That other CL is in now.

Unfortunately this call is a NOP on platforms that don't support display
resizing such as Windows Ash - as far as I remember trying. So it is safer to
bail out or code assertions that are clear about it.
This is true even if you set display bounds in SetUp method.

[Matt Giuca, 2014-05-13 05:31:58]

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
File ash/wm/app_list_controller_unittest.cc (right):

https://codereview.chromium.org/281523002/diff/1/ash/wm/app_list_controller_u...
ash/wm/app_list_controller_unittest.cc:149: TEST_P(AppListControllerTest,
TinyDisplay) {
On 2014/05/13 04:40:11, varkha wrote:
> On 2014/05/13 04:02:12, Matt Giuca wrote:
> > Why? This test doesn't use window resizing, it just sets up an initial
display
> > size for testing.
> > 
> > Note: That other CL is in now.
> 
> Unfortunately this call is a NOP on platforms that don't support display
> resizing such as Windows Ash - as far as I remember trying. So it is safer to
> bail out or code assertions that are clear about it.
> This is true even if you set display bounds in SetUp method.

Fair enough. Done.

[varkha, 2014-05-13 05:36:57]

lgtm.

[Matt Giuca, 2014-05-13 05:38:11]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-05-13 05:39:23]

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

[commit-bot: I haz the power, 2014-05-13 11:00:38]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-13 13:45:24]

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

[commit-bot: I haz the power, 2014-05-13 13:45:24]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Matt Giuca, 2014-05-13 14:32:47]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-05-13 14:33:17]

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

[commit-bot: I haz the power, 2014-05-13 16:13:08]

Change committed as 270115