Observe chrome::NOTIFICATION_APP_TERMINATING in Chrome's AppListViewDelegate

This allows custom launcher pages to be torn down in response to this
notification in the same way that regular packaged app windows are.
Currently these WebContents are torn down later, by
Widget::CloseAllSecondaryWidgets(), but this relies on the WebContents
lifetime being tied to a Widget.

Observing NOTIFICATION_APP_TERMINATING also allows us to explore
decoupling the AppListViewDelegate from the AppListView lifetime, and
remove some special code on Mac to deal with updating the profile
switcher (since the AppListViewDelegate is owned by a leaky singleton
there).

BUG=403647

Review URL: https://codereview.chromium.org/559823002

Cr-Commit-Position: refs/heads/master@{#295194}

chrome/browser/signin/signin_manager_factory.cc

@@ -24,20 +24,6 @@ SigninManagerFactory::SigninManagerFactory()
 }
 
 SigninManagerFactory::~SigninManagerFactory() {
-#if defined(OS_MACOSX)
-  // Check that the number of remaining observers is as expected. Mac has a
-  // known issue wherein there might be a remaining observer
-  // (UIAppListViewDelegate).
-  int num_observers = 0;
-  if (observer_list_.might_have_observers()) {
-    ObserverListBase<SigninManagerFactory::Observer>::Iterator it(
-        observer_list_);
-    while (it.GetNext()) {
-      num_observers++;
-    }
-  }
-  DCHECK_LE(num_observers, 1);
-#endif  // defined(OS_MACOSX)
 }
 
 #if defined(OS_CHROMEOS)

chrome/browser/signin/signin_manager_factory.h

@@ -76,14 +76,8 @@ class SigninManagerFactory : public BrowserContextKeyedServiceFactory {
   SigninManagerFactory();
   virtual ~SigninManagerFactory();
 
-#if defined(OS_MACOSX)
-  // List of observers. Does not check that list is empty on destruction, as
-  // there are some leaky singletons that observe the SigninManagerFactory.
-  mutable ObserverList<Observer> observer_list_;
-#else
   // List of observers. Checks that list is empty on destruction.
   mutable ObserverList<Observer, true> observer_list_;
-#endif
 
   // BrowserContextKeyedServiceFactory:
   virtual KeyedService* BuildServiceInstanceFor(

chrome/browser/ui/app_list/app_list_view_delegate.cc

@@ -35,6 +35,7 @@
 #include "chrome/common/url_constants.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_contents.h"
@@ -181,9 +182,16 @@ AppListViewDelegate::AppListViewDelegate(Profile* profile,
           IDR_APP_LIST_GOOGLE_LOGO_VOICE_SEARCH));
 #endif
   SetProfile(profile);
+
+  registrar_.Add(this,
+                 chrome::NOTIFICATION_APP_TERMINATING,
+                 content::NotificationService::AllSources());
 }
 
 AppListViewDelegate::~AppListViewDelegate() {
+  // Note that the destructor is not always called. E.g. on Mac, this is owned
+  // by a leaky singleton. Essential shutdown work must be done by observing
+  // chrome::NOTIFICATION_APP_TERMINATING.
   SetProfile(NULL);
   g_browser_process->profile_manager()->GetProfileInfoCache().RemoveObserver(
       this);
@@ -194,6 +202,9 @@ AppListViewDelegate::~AppListViewDelegate() {
 }
 
 void AppListViewDelegate::SetProfile(Profile* new_profile) {
+  if (profile_ == new_profile)
+    return;
+
   if (profile_) {
     // Note: |search_controller_| has a reference to |speech_ui_| so must be
     // destroyed first.
@@ -248,6 +259,11 @@ void AppListViewDelegate::SetUpSearchUI() {
 }
 
 void AppListViewDelegate::SetUpProfileSwitcher() {
+  // If a profile change is observed when there is no app list, there is nothing
+  // to update until SetProfile() calls this function again.
+  if (!profile_)
+    return;
+
   // Don't populate the app list users if we are on the ash desktop.
   chrome::HostDesktopType desktop = chrome::GetHostDesktopTypeForNativeWindow(
       controller_->GetAppListWindow());
@@ -439,6 +455,9 @@ void AppListViewDelegate::Dismiss()  {
 void AppListViewDelegate::ViewClosing() {
   controller_->ViewClosing();
 
+  if (!profile_)
+    return;
+
   app_list::StartPageService* service =
       app_list::StartPageService::Get(profile_);
   if (service) {
@@ -623,3 +642,20 @@ void AppListViewDelegate::RemoveObserver(
     app_list::AppListViewDelegateObserver* observer) {
   observers_.RemoveObserver(observer);
 }
+
+void AppListViewDelegate::Observe(int type,
+                                  const content::NotificationSource& source,
+                                  const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_APP_TERMINATING:
+      SetProfile(NULL);  // Ensures launcher page web contents are torn down.
+
+      // SigninManagerFactory is not a leaky singleton (unlike this class), and
+      // its destructor will check that it has no remaining observers.
+      scoped_observer_.RemoveAll();
+      SigninManagerFactory::GetInstance()->RemoveObserver(this);
+      break;
+    default:
+      NOTREACHED();
+  }
+}

chrome/browser/ui/app_list/app_list_view_delegate.h

@@ -19,6 +19,8 @@
 #include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/ui/app_list/start_page_observer.h"
 #include "components/signin/core/browser/signin_manager_base.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
 #include "ui/app_list/app_list_view_delegate.h"
 
 class AppListControllerDelegate;
@@ -50,7 +52,8 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
                             public HotwordClient,
                             public ProfileInfoCacheObserver,
                             public SigninManagerBase::Observer,
-                            public SigninManagerFactory::Observer {
+                            public SigninManagerFactory::Observer,
+                            public content::NotificationObserver {
  public:
   // Constructs Chrome's AppListViewDelegate, initially for |profile|.
   // Does not take ownership of |controller|. TODO(tapted): It should.
@@ -143,6 +146,11 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
       const base::FilePath& profile_path,
       const base::string16& old_profile_name) OVERRIDE;
 
+  // Overridden from content::NotificationObserver:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
   // Unowned pointer to the controller.
   AppListControllerDelegate* controller_;
   // Unowned pointer to the associated profile. May change if SetProfileByPath
@@ -173,6 +181,9 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
   // Window contents of additional custom launcher pages.
   ScopedVector<apps::CustomLauncherPageContents> custom_page_contents_;
 
+  // Registers for NOTIFICATION_APP_TERMINATING to unload custom launcher pages.
+  content::NotificationRegistrar registrar_;
+
   DISALLOW_COPY_AND_ASSIGN(AppListViewDelegate);
 };