Refactor app list so AppsGridView owns the PaginationModel.

ui/app_list/views/apps_grid_view.cc

Index: ui/app_list/views/apps_grid_view.cc
diff --git a/ui/app_list/views/apps_grid_view.cc b/ui/app_list/views/apps_grid_view.cc
index 52034d8c024b4de49027bdab98b83935e43982c5..5c6193ca52a4871da517621b6d23c66305bf0d49 100644
--- a/ui/app_list/views/apps_grid_view.cc
+++ b/ui/app_list/views/apps_grid_view.cc
@@ -335,14 +335,13 @@ class SynchronousDrag : public ui::DragSourceWin {
 };
 #endif  // defined(OS_WIN)
 
-AppsGridView::AppsGridView(AppsGridViewDelegate* delegate,
-                           PaginationModel* pagination_model)
+AppsGridView::AppsGridView(AppsGridViewDelegate* delegate)
     : model_(NULL),
       item_list_(NULL),
       delegate_(delegate),
       folder_delegate_(NULL),
-      pagination_model_(pagination_model),
-      page_switcher_view_(new PageSwitcher(pagination_model)),
+      pagination_model_(new PaginationModel),
+      page_switcher_view_(new PageSwitcher(pagination_model_.get())),
       cols_(0),
       rows_per_page_(0),
       selected_view_(NULL),
@@ -384,6 +383,12 @@ AppsGridView::~AppsGridView() {
 
   if (item_list_)
     item_list_->RemoveObserver(this);
+
+  // Since |pagination_model_| will be destroyed before the views,
+  // |page_switcher_view_| must stop observing. (We cannot simply call
+  // RemoveAllChildViews, since that will trigger a ViewHierarchyChanged which
+  // should not be called during destruction.)

[xiyuan, 2014/05/30 15:47:47]

RemoveAllChildViews could be called from a view's dtor. What problem did you
see?

[tapted, 2014/06/02 04:02:21]

(#guessing) Perhaps because PageSwitcher is a "sibling" view the dependency
between it and the grid view can be lost by the time it gets to ~PageSwitcher().
Perhaps this is reason enough for a WeakPtr? That has downsides too, but is a
bit more conventional, and would make some DCHECKs redundant as well as remove
the need for ReleasePaginationModel (I think).

[Matt Giuca, 2014/06/02 07:11:23]

I can't call RemoveAllChildViews from this view's dtor because
ViewHierarchyChanged gets called (as I said in the comment).
ViewHierarchyChanged is not equipped to deal with the case where the view is
partially torn down (its CHECK_EQ fails for one thing).

I don't think a weak ref will help here. The problem is not about
use-after-free, but observing. (Unless you mean to use the weak ref to decide
not to call RemoveObserver if the PaginationModel has already been destroyed.)

[tapted, 2014/06/02 08:36:49]

yep - that ;)

True, it's a lazy/not-ideal way to handle observing, since it means the observer
list still has one observer when it is destroyed. It might have a certain
elegance here, but I'm not too fussed.

[xiyuan, 2014/06/02 17:02:08]

CHECK_EQ is added to catch a crash where view_model_ is holding an invalid
reference. We can clear view_model_ here to fix the CHECK_EQ (or just "delete
page_switcher_view_"). The rest of ViewHierarchyChanged should be okay.

[Matt Giuca, 2014/06/03 01:52:36]

Ah OK, I didn't realise it was as simple as clearing view_model_. I've done that
and it works, so I'll go with that much simpler solution.

+  page_switcher_view_->ReleasePaginationModel();
 }
 
 void AppsGridView::SetLayout(int icon_size, int cols, int rows_per_page) {