Fix Profile* lifetime issues in Chrome's AppListViewDelegate

chrome/browser/ui/app_list/app_list_view_delegate.h

Index: chrome/browser/ui/app_list/app_list_view_delegate.h
diff --git a/chrome/browser/ui/app_list/app_list_view_delegate.h b/chrome/browser/ui/app_list/app_list_view_delegate.h
index ec67e7489a809580c637413c31ac2702db49a107..e2ae69edf0d70d7d541aa6dcd9d9d8a455057472 100644
--- a/chrome/browser/ui/app_list/app_list_view_delegate.h
+++ b/chrome/browser/ui/app_list/app_list_view_delegate.h
@@ -52,13 +52,23 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
                             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;
@@ -130,7 +140,6 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
       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
@@ -141,6 +150,7 @@ class AppListViewDelegate : public app_list::AppListViewDelegate,
   app_list::AppListModel* model_;
 
   scoped_ptr<app_list::SpeechUIModel> speech_ui_;
+  scoped_ptr<app_list::SearchController> search_controller_;
 
   base::TimeDelta auto_launch_timeout_;