Fix Profile* lifetime issues in Chrome's AppListViewDelegate

chrome/browser/ui/app_list/app_list_view_delegate.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_UI_APP_LIST_APP_LIST_VIEW_DELEGATE_H_
 #define CHROME_BROWSER_UI_APP_LIST_APP_LIST_VIEW_DELEGATE_H_
 
 #include <string>
 
 #include "base/basictypes.h"
@@ skipping 34 matching lines @@
 class AppSyncUIStateWatcher;
 #endif
 
 class AppListViewDelegate : public app_list::AppListViewDelegate,
                             public app_list::StartPageObserver,
                             public HotwordClient,
                             public ProfileInfoCacheObserver,
                             public SigninManagerBase::Observer,
                             public SigninManagerFactory::Observer {
  public:
-  AppListViewDelegate(Profile* profile,
-                      AppListControllerDelegate* controller);
+  // Constructor. Does not take ownership of |controller_delegate|.
+  // TODO(tapted): It should.
+  explicit AppListViewDelegate(AppListControllerDelegate* controller_delegate);

[Matt Giuca, 2014/08/25 04:48:46]

I don't see why you had to delete the Profile argument. Since every single
instantiation of ALVD is now followed by a call to SetProfile, it seems better
to just leave this as-is and call SetProfile at the end of the constructor.

This would let you revert a lot of the changes throughout this CL which appear
to just be adding a call to SetProfile (I've noticed you did a lot of "misc"
cleanup nearby those changes, which is good but probably best to leave out of
this already confusing CL).

[tapted, 2014/08/25 06:17:00]

Done.

I think splitting the constructor is still an improvement. It goes with the
http://go/cppguide#Doing_Work_in_Constructors style guide. But it can be done
orthogonally.

[Matt Giuca, 2014/08/26 01:29:45]

Fair enough. Can you put back your comment about taking ownership, and your
TODO?

Yeah I'm happy to split out SetProfile in another CL (to satisfy "doing work in
constructors" rule).

[tapted, 2014/08/26 02:43:57]

Done.

   virtual ~AppListViewDelegate();
 
+  // Configure the AppList for the given |profile|.
+  void SetProfile(Profile* profile);
+
  private:
-  // Updates the app list's current profile and ProfileMenuItems.
-  void OnProfileChanged();
+  // Updates the speech webview and start page for the current |profile_|.
+  void SetUpSearchUI();
+
+  // Updates the app list's ProfileMenuItems for the current |profile_|.
+  void SetUpProfileSwitcher();
+
+  // Updates the app list's custom launcher pages for the current |profile_|.
+  void SetUpCustomLauncherPages();
 
   // Overridden from app_list::AppListViewDelegate:
   virtual bool ForceNativeDesktop() const OVERRIDE;
   virtual void SetProfileByPath(const base::FilePath& profile_path) OVERRIDE;
   virtual app_list::AppListModel* GetModel() OVERRIDE;
   virtual app_list::SpeechUIModel* GetSpeechUI() OVERRIDE;
   virtual void GetShortcutPathForApp(
       const std::string& app_id,
       const base::Callback<void(const base::FilePath&)>& callback) OVERRIDE;
   virtual void StartSearch() OVERRIDE;
@@ skipping 51 matching lines @@
   virtual void GoogleSignedOut(const std::string& username) OVERRIDE;
 
   // Overridden from ProfileInfoCacheObserver:
   virtual void OnProfileAdded(const base::FilePath& profile_path) OVERRIDE;
   virtual void OnProfileWasRemoved(const base::FilePath& profile_path,
                                    const base::string16& profile_name) OVERRIDE;
   virtual void OnProfileNameChanged(
       const base::FilePath& profile_path,
       const base::string16& old_profile_name) OVERRIDE;
 
-  scoped_ptr<app_list::SearchController> search_controller_;
   // Unowned pointer to the controller.
   AppListControllerDelegate* controller_;
   // Unowned pointer to the associated profile. May change if SetProfileByPath
   // is called.
   Profile* profile_;
   // Unowned pointer to the model owned by AppListSyncableService. Will change
   // if |profile_| changes.
   app_list::AppListModel* model_;
 
   scoped_ptr<app_list::SpeechUIModel> speech_ui_;
+  scoped_ptr<app_list::SearchController> search_controller_;
 
   base::TimeDelta auto_launch_timeout_;
 
   Users users_;
 
 #if defined(USE_ASH)
   scoped_ptr<AppSyncUIStateWatcher> app_sync_ui_state_watcher_;
 #endif
 
   ObserverList<app_list::AppListViewDelegateObserver> observers_;
 
   // Used to track the SigninManagers that this instance is observing so that
   // this instance can be removed as an observer on its destruction.
   ScopedObserver<SigninManagerBase, AppListViewDelegate> scoped_observer_;
 
   // Window contents of additional custom launcher pages.
   ScopedVector<apps::CustomLauncherPageContents> custom_page_contents_;
 
   DISALLOW_COPY_AND_ASSIGN(AppListViewDelegate);
 };
 
 #endif  // CHROME_BROWSER_UI_APP_LIST_APP_LIST_VIEW_DELEGATE_H_