Move ownership of the AppListViewDelegate into the AppListService

Move ownership of the AppListViewDelegate into the AppListService

It's currently hard for the AppListService to access the model. The
model is now profile keyed. However, to get the "right" profile, the
AppListViewDelegate is the current source of truth. And to get this,
AppListService needs to dig into its platform-specific parts to get it
via the platform-specific view.

However, the AppListView can be destroyed, so the delegate doesn't
always exist. Therefore the AppListViewDelegate doesn't always exist --
it is created each time the AppListView is created. On ChromeOS, this is
each time the app list is shown.

Before the model was profile-keyed, some state was tricky to store.
E.g., some is stored in ExtensionAppModelBuilder, and updated via
notifications (e.g. NOTIFICATION_APP_INSTALLED_TO_APPLIST). Also, since
the model must now outlive the AppListView for sync, it makes less sense
for the AppListViewDelegate to be owned by the view. Currently showing
the app list can create a new ALVD, which must be re-attached to the
model each time.

This change moves ownership of the AppListViewDelegate into the
AppListServiceIpml or test/shell controllers, which will create it
lazily and then cache the result.

BUG=403647, 404535
Committed: https://crrev.com/537b88b13b9aa406744db1f2681ec979d593d2e8
Cr-Commit-Position: refs/heads/master@{#295646}

[tapted, 2014-09-18 04:10:58]

Patchset #7 (id:120001) has been deleted

[tapted, 2014-09-18 04:11:06]

Patchset #6 (id:100001) has been deleted

[tapted, 2014-09-18 04:11:07]

Patchset #4 (id:60001) has been deleted

[tapted, 2014-09-18 04:11:11]

Patchset #3 (id:40001) has been deleted

[tapted, 2014-09-18 04:11:14]

Patchset #2 (id:20001) has been deleted

[tapted, 2014-09-18 04:11:29]

Patchset #4 (id:160001) has been deleted

[tapted, 2014-09-18 04:11:39]

Patchset #4 (id:180001) has been deleted

[tapted, 2014-09-18 06:16:32]

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

[tapted, 2014-09-18 06:29:03]

Hi xiyuan - I think you might be the best reviewer for this, (just in case what
I'm trying to do is crazy :). Please take a look

[xiyuan, 2014-09-18 19:48:40]

lgtm

[tapted, 2014-09-18 23:10:25]

tapted@chromium.org changed reviewers:
+ jamescook@chromium.org

[tapted, 2014-09-18 23:10:25]

+ jamescook could you please review for OWNERS in src/ash and c/b/ui/ash

[James Cook, 2014-09-18 23:29:56]

ash LGTM with nits

https://codereview.chromium.org/508813002/diff/220001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/508813002/diff/220001/ash/shell_delegate.h#ne...
ash/shell_delegate.h:114: // Invoked to create an AppListViewDelegate. Shell
takes the ownership of
nit: Is "Shell takes ownership" still correct?

https://codereview.chromium.org/508813002/diff/220001/ash/wm/app_list_control...
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/508813002/diff/220001/ash/wm/app_list_control...
ash/wm/app_list_controller.cc:190: // AppListModel and AppListViewDelegate are
owned by AppListView. They
nit: Is this comment still correct?

https://codereview.chromium.org/508813002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl_browsertest.cc (right):

https://codereview.chromium.org/508813002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl_browsertest.cc:128: Profile*
second_profile = test::CreateSecondProfileAsync();
Not for this CL: CreateSecondProfileAsync() is kind of an odd name -- when I
read the test it looks like it is doing something async and the test isn't
waiting for it.  But it is actually a synchronous call.  It just reads oddly.

[tapted, 2014-09-19 00:49:01]

Thanks all!

https://codereview.chromium.org/508813002/diff/220001/ash/shell_delegate.h
File ash/shell_delegate.h (right):

https://codereview.chromium.org/508813002/diff/220001/ash/shell_delegate.h#ne...
ash/shell_delegate.h:114: // Invoked to create an AppListViewDelegate. Shell
takes the ownership of
On 2014/09/18 23:29:55, James Cook wrote:
> nit: Is "Shell takes ownership" still correct?

good catch - thought I got all of these :). Changed to

  // Get the AppListViewDelegate, creating one if it does not yet exist.
  // Ownership stays with Chrome's AppListService, or the ShellDelegate.

https://codereview.chromium.org/508813002/diff/220001/ash/wm/app_list_control...
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/508813002/diff/220001/ash/wm/app_list_control...
ash/wm/app_list_controller.cc:190: // AppListModel and AppListViewDelegate are
owned by AppListView. They
On 2014/09/18 23:29:55, James Cook wrote:
> nit: Is this comment still correct?

The model comment hasn't been right for a while (it's owned by
AppListServiceSyncable in Chrome builds), but it's out of place here anyway.
Here, I've put

    // Note the AppListViewDelegate outlives the AppListView. For Ash, the view
    // is destroyed when dismissed.

https://codereview.chromium.org/508813002/diff/220001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_service_impl_browsertest.cc (right):

https://codereview.chromium.org/508813002/diff/220001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_service_impl_browsertest.cc:128: Profile*
second_profile = test::CreateSecondProfileAsync();
On 2014/09/18 23:29:56, James Cook wrote:
> Not for this CL: CreateSecondProfileAsync() is kind of an odd name -- when I
> read the test it looks like it is doing something async and the test isn't
> waiting for it.  But it is actually a synchronous call.  It just reads oddly.

good point - I agree it's confusing. It's named this way because it calls
ProfileManager::CreateProfileAsync() (which is asynchronous). The asynchrony is
encapsulated here, but it's important for app list tests to know a run loop is
being spun up since if that happens while the app list is showing, it can cause
the app list to be dismissed and make flaky tests. I'll fix this in a follow-up.

[tapted, 2014-09-19 01:50:58]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-09-19 01:52:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/508813002/240001

[commit-bot: I haz the power, 2014-09-19 02:49:20]

Committed patchset #6 (id:240001) as 5080ffa167d3a4fb76273abeea84d73eb831d90d

[commit-bot: I haz the power, 2014-09-19 02:50:19]

Patchset 6 (id:??) landed as
https://crrev.com/537b88b13b9aa406744db1f2681ec979d593d2e8
Cr-Commit-Position: refs/heads/master@{#295646}