Use centered app list position whenever virtual keyboard is enabled.

Use centered app list position whenever virtual keyboard is enabled.

The old app list position doesn't fit on the screen when the keyboard is
open. Therefore, whenever the virtual keyboard is enabled, use the new
app list position (centered and wide).

BUG=369382
TEST=In ChromeOS settings, under Accessibility, enable on-screen
keyboard. Open App Launcher. App Launcher should now be centered and
landscape.
TBR=jamescook@chromium.org

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

[Matt Giuca, 2014-04-30 06:26:49]

benwells: Please review
chrome/browser/ui/views/app_list/*
ui/app_list/*
Let me know if you want to delegate the review to someone else.

The reason for most of the complexity is that I can't directly talk to
ui/keyboard from ui/app_list; I have to go through AppListViewDelegate.

varkha: Just a quick change to ash/wm/app_list_controller.cc.

[benwells, 2014-04-30 06:40:29]

https://codereview.chromium.org/253983002/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/linux/app_list_linux_unittest.cc (right):

https://codereview.chromium.org/253983002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/linux/app_list_linux_unittest.cc:112: false);
This thing is no longer experimental: it is being enabled by default for certain
situations. So we should add some testing of it here. Luckily it should be a lot
easier to test than the more general positioning that happens.

https://codereview.chromium.org/253983002/diff/50001/ui/app_list/app_list_swi...
File ui/app_list/app_list_switches.h (right):

https://codereview.chromium.org/253983002/diff/50001/ui/app_list/app_list_swi...
ui/app_list/app_list_switches.h:36: bool APP_LIST_EXPORT
IsExperimentalAppListPositionEnabled();
Let's remove all traces of 'experimental' from the code for this - it is no
longer experimental with your change. E.g. this could be
IsCenteredAppListEnabled.

[Matt Giuca, 2014-04-30 06:53:38]

https://codereview.chromium.org/253983002/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/linux/app_list_linux_unittest.cc (right):

https://codereview.chromium.org/253983002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/linux/app_list_linux_unittest.cc:112: false);
Sounds reasonable.

https://codereview.chromium.org/253983002/diff/50001/ui/app_list/app_list_swi...
File ui/app_list/app_list_switches.h (right):

https://codereview.chromium.org/253983002/diff/50001/ui/app_list/app_list_swi...
ui/app_list/app_list_switches.h:36: bool APP_LIST_EXPORT
IsExperimentalAppListPositionEnabled();
Should we also change the flag name, or is that frowned upon?

[varkha, 2014-04-30 16:20:15]

I've looked at the bug and left a comment there. I think it may make more sense
to adjust the size but not necessarily the origin of the app list when virtual
keyboard is enabled.

Also is "new devices" considered to be same as "vk enabled"? I thought the bug
was asking to enable it on specific devices - just want to clarify.

cc kevers@.

https://codereview.chromium.org/253983002/diff/50001/ash/wm/app_list_controll...
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/253983002/diff/50001/ash/wm/app_list_controll...
ash/wm/app_list_controller.cc:168: if (view->UseExperimentalAppListPosition()) {
I agree, let's remove any mention of "experimental".

https://codereview.chromium.org/253983002/diff/50001/ui/app_list/app_list_swi...
File ui/app_list/app_list_switches.h (right):

https://codereview.chromium.org/253983002/diff/50001/ui/app_list/app_list_swi...
ui/app_list/app_list_switches.h:30: bool APP_LIST_EXPORT
IsExperimentalAppListEnabled();
This should also not be experimental anymore.

[varkha, 2014-04-30 16:39:34]

A question: should this be looked at in tie with
https://codereview.chromium.org/250423004/ and
https://codereview.chromium.org/260663002/ or is it independent?

[Matt Giuca, 2014-04-30 22:32:28]

Yes. It isn't really related to those other two CLs, it's just going in after
them and I've pre-resolved the merge conflicts.

Also see my comments on the bug.

[Matt Giuca, 2014-05-05 06:18:05]

OK, all updated and the renames and tests are done. Please take another look.

Please note that most of the renaming took place in another CL
(https://codereview.chromium.org/263883002/), so that will show up in the Rebase
patch set, not the subsequent ones.

https://codereview.chromium.org/253983002/diff/50001/ash/wm/app_list_controll...
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/253983002/diff/50001/ash/wm/app_list_controll...
ash/wm/app_list_controller.cc:168: if (view->UseExperimentalAppListPosition()) {
On 2014/04/30 16:20:15, varkha wrote:
> I agree, let's remove any mention of "experimental".

Done.

https://codereview.chromium.org/253983002/diff/50001/chrome/browser/ui/views/...
File chrome/browser/ui/views/app_list/linux/app_list_linux_unittest.cc (right):

https://codereview.chromium.org/253983002/diff/50001/chrome/browser/ui/views/...
chrome/browser/ui/views/app_list/linux/app_list_linux_unittest.cc:112: false);
On 2014/04/30 06:53:38, Matt Giuca wrote:
> Sounds reasonable.

Done.

https://codereview.chromium.org/253983002/diff/50001/ui/app_list/app_list_swi...
File ui/app_list/app_list_switches.h (right):

https://codereview.chromium.org/253983002/diff/50001/ui/app_list/app_list_swi...
ui/app_list/app_list_switches.h:30: bool APP_LIST_EXPORT
IsExperimentalAppListEnabled();
This one is still experimental (it is different to
IsExperimentalAppListPositionEnabled; now IsCenteredAppListEnabled -- this one
also introduces a bunch of new experiments including switchable top-level
views).

https://codereview.chromium.org/253983002/diff/50001/ui/app_list/app_list_swi...
ui/app_list/app_list_switches.h:36: bool APP_LIST_EXPORT
IsExperimentalAppListPositionEnabled();
On 2014/04/30 06:53:38, Matt Giuca wrote:
> Should we also change the flag name, or is that frowned upon?

Done.

[varkha, 2014-05-05 06:28:53]

LGTM for ash/wm.

[benwells, 2014-05-06 04:41:22]

thanks! lgtm!

[Matt Giuca, 2014-05-06 05:50:35]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-05-06 05:51:01]

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

[commit-bot: I haz the power, 2014-05-06 05:57:09]

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:
  ios_rel_device_ninja on tryserver.chromium

[commit-bot: I haz the power, 2014-05-06 06:03:30]

Change committed as 268443

[Mike West, 2014-05-06 06:57:38]

A revert of this CL has been created in
https://codereview.chromium.org/266193009/ by mkwst@chromium.org.

The reason for reverting is: Broke compilation on for CrOS[1] and Win64[2].

[1]:
http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20C...
[2]:
http://build.chromium.org/p/chromium.win/buildstatus?builder=Win%20x64%20Buil....

[Matt Giuca, 2014-05-06 08:09:45]

jamescook: Had to add you as a last-minute reviewer to fix the broken build for
ash_shell. (Just going to commit with TBR.)

[Matt Giuca, 2014-05-06 08:11:30]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-05-06 08:11:53]

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

[commit-bot: I haz the power, 2014-05-06 10:24:00]

Change committed as 268479