Ensure that the AppListController class which lives in app_list_controller_win.cc plays nice with A…

Ensure that the AppListController class which lives in app_list_controller_win.cc plays nice with AURA and ASH.

For ASH on Windows 8 we defer calls to open and hide the apps list to the ash Shell via the ToggleAppList member
function in the Shell class.

For AURA the fixes were mostly around retrieving the native window for the view appropriately, i.e via RootWindow::GetAcceleratedWidget

This also fixes the AppInstallConfirmation and AppInstallConfirmation_Incognito browser tests for AURA.

I added a class AppListControllerAsh which inherits from the AppListController class and
overrides the ShowAppList/DismissAppList functions to display the ASH specific app lists.

This also fixes the startup crash in desktop chrome observed when displaying the app list
as we were deferencing a NULL ash shell pointer.

The app_list_controller_ash.cc functionality is disabled for Windows and is provided by the newly
added AppListControllerAsh class which lives in the app_list_controller_win.cc file.

BUG=174940, 174399, 174504, 175389
TEST=covered by existing Browser tests for desktop chrome AURA.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182051

[koz (OOO until 15th September), 2013-02-10 22:22:10]

[+benwells, +tapted]

[koz (OOO until 15th September), 2013-02-10 22:41:39]

https://codereview.chromium.org/12210102/diff/13002/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12210102/diff/13002/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:474: #if
defined(USE_AURA)
Rather than adding these #ifdefs into app_list_controller_win, it would be nicer
if the shared interface was lifted into an abstract base class, then callers
could acquire the right implementation and call it themselves.

So rather than callers looking like they do now, ie:

  chrome::ShowAppList(profile);

They would look like

  chrome::GetCurrentAppList()->ShowAppList(profile);

Where GetCurrentAppList() returns different things depending on '#if
defined(USE_AURA)' and chrome::GetActiveDesktop().

Is there any way to achieve this?

[ananta, 2013-02-11 23:27:00]

https://codereview.chromium.org/12210102/diff/13002/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12210102/diff/13002/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:474: #if
defined(USE_AURA)
On 2013/02/10 22:41:39, koz wrote:
> Rather than adding these #ifdefs into app_list_controller_win, it would be
nicer
> if the shared interface was lifted into an abstract base class, then callers
> could acquire the right implementation and call it themselves.
> 
> So rather than callers looking like they do now, ie:
> 
>   chrome::ShowAppList(profile);
> 
> They would look like
> 
>   chrome::GetCurrentAppList()->ShowAppList(profile);
> 
> Where GetCurrentAppList() returns different things depending on '#if
> defined(USE_AURA)' and chrome::GetActiveDesktop().
> 
> Is there any way to achieve this?


I wanted to avoid adding a framework around USE_AURA because  this is going to
be the default in the near future.

To reduce the number of ifdefs I did the following.

1. Added a member function GetAppListHWND to the 
   AppListController class. This function returns the HWND
   appropriately for the AURA and non AURA case.

2. Added a class AppListControllerAsh which inherits from 
   AppListController. It overrides the following functions
   1. ShowAppList
   2. DismissAppList.
   3. OnBeginExtensionInstall
   4. OnDownloadProgress.
   The last 2 are unimplemented for ASH as they are not 
   needed there.

   5. I added a new virtual bool function IsAppListVisible 
      to the AppListController class which returns whether
      the app list is visible for desktop and ASH.

Disabled the chrome:: implementations for ShowAppList et all 
in app_list_controller_ash.cc for Windows as they conflict with the
implementation in app_list_controller_win.cc which is needed for desktop chrome.

This also fixes the crash in desktop chrome reported here.
https://code.google.com/p/chromium/issues/detail?id=175389

[koz (OOO until 15th September), 2013-02-12 00:33:48]

LGTM

This is much nicer, thanks!

https://codereview.chromium.org/12210102/diff/11006/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12210102/diff/11006/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:785: #if
!defined(USE_AURA)
nit: rearrange this to be #if defined(USE_AURA)

https://codereview.chromium.org/12210102/diff/11006/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:864: virtual void
OnBeginExtensionInstall(Profile* profile,
You mentioned in your top level comment that this method and
OnDownloadProgress() aren't necessary for Ash, could you add a comment here
explaining why?

https://codereview.chromium.org/12210102/diff/11006/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:883: void
AppListControllerAsh::ShowAppList(Profile* profile) {
I think this will have problems when |profile| is not the same profile that
ash::Shell is using. Could you add a TODO to handle |profile| correctly?

[benwells, 2013-02-12 00:41:50]

rubber stamp lgtm

[ananta, 2013-02-12 04:57:53]

https://codereview.chromium.org/12210102/diff/11006/chrome/browser/ui/views/a...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12210102/diff/11006/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:785: #if
!defined(USE_AURA)
On 2013/02/12 00:33:48, koz wrote:
> nit: rearrange this to be #if defined(USE_AURA)

Done.

https://codereview.chromium.org/12210102/diff/11006/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:864: virtual void
OnBeginExtensionInstall(Profile* profile,
On 2013/02/12 00:33:48, koz wrote:
> You mentioned in your top level comment that this method and
> OnDownloadProgress() aren't necessary for Ash, could you add a comment here
> explaining why?

Done.

https://codereview.chromium.org/12210102/diff/11006/chrome/browser/ui/views/a...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:883: void
AppListControllerAsh::ShowAppList(Profile* profile) {
On 2013/02/12 00:33:48, koz wrote:
> I think this will have problems when |profile| is not the same profile that
> ash::Shell is using. Could you add a TODO to handle |profile| correctly?

Done.

[xiyuan, 2013-02-12 18:54:39]

lgtm

https://codereview.chromium.org/12210102/diff/9020/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12210102/diff/9020/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:53: #include
"ash/shell.h"
nit: This probably should be wrapped under defined(USE_ASH)

[ananta, 2013-02-12 19:00:11]

https://codereview.chromium.org/12210102/diff/9020/chrome/browser/ui/views/ap...
File chrome/browser/ui/views/app_list/app_list_controller_win.cc (right):

https://codereview.chromium.org/12210102/diff/9020/chrome/browser/ui/views/ap...
chrome/browser/ui/views/app_list/app_list_controller_win.cc:53: #include
"ash/shell.h"
On 2013/02/12 18:54:39, xiyuan wrote:
> nit: This probably should be wrapped under defined(USE_ASH)

Done.