Refactor app list so AppsGridView owns the PaginationModel.

ui/app_list/views/apps_grid_view.cc

 // 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.
 
 #include "ui/app_list/views/apps_grid_view.h"
 
 #include <algorithm>
 #include <set>
 #include <string>
 
@@ skipping 317 matching lines @@
   gfx::Point drag_view_offset_;
   bool has_shortcut_path_;
   base::FilePath shortcut_path_;
   bool running_;
   bool canceled_;
 
   DISALLOW_COPY_AND_ASSIGN(SynchronousDrag);
 };
 #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),
       drag_view_(NULL),
       drag_start_page_(-1),
 #if defined(OS_WIN)
       use_synchronous_drag_(true),
 #endif
       drag_pointer_(NONE),
       drop_attempt_(DROP_FOR_NONE),
@@ skipping 21 matching lines @@
   DCHECK(!drag_view_);
   if (drag_view_)
     EndDrag(true);
 
   if (model_)
     model_->RemoveObserver(this);
   pagination_model_->RemoveObserver(this);
 
   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) {
   icon_size_.SetSize(icon_size, icon_size);
   cols_ = cols;
   rows_per_page_ = rows_per_page;
 
   SetBorder(views::Border::CreateEmptyBorder(
       kTopPadding, kLeftRightPadding, 0, kLeftRightPadding));
 }
@@ skipping 1769 matching lines @@
 void AppsGridView::SetAsFolderDroppingTarget(const Index& target_index,
                                              bool is_target_folder) {
   AppListItemView* target_view =
       static_cast<AppListItemView*>(
           GetViewAtSlotOnCurrentPage(target_index.slot));
   if (target_view)
     target_view->SetAsAttemptedFolderTarget(is_target_folder);
 }
 
 }  // namespace app_list