Refactor app list so AppsGridView owns the PaginationModel.

ui/app_list/views/app_list_main_view.cc

 // Copyright 2013 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/app_list_main_view.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_util.h"
 #include "ui/app_list/app_list_constants.h"
 #include "ui/app_list/app_list_folder_item.h"
 #include "ui/app_list/app_list_item.h"
 #include "ui/app_list/app_list_model.h"
 #include "ui/app_list/app_list_switches.h"
 #include "ui/app_list/app_list_view_delegate.h"
 #include "ui/app_list/pagination_model.h"
 #include "ui/app_list/search_box_model.h"
 #include "ui/app_list/views/app_list_item_view.h"
 #include "ui/app_list/views/apps_container_view.h"
+#include "ui/app_list/views/apps_grid_view.h"
 #include "ui/app_list/views/contents_switcher_view.h"
 #include "ui/app_list/views/contents_view.h"
 #include "ui/app_list/views/search_box_view.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/widget/widget.h"
 
 namespace app_list {
 
 namespace {
@@ skipping 42 matching lines @@
   AppListMainView* owner_;
   AppListItem* item_;
 
   DISALLOW_COPY_AND_ASSIGN(IconLoader);
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 // AppListMainView:
 
 AppListMainView::AppListMainView(AppListViewDelegate* delegate,
-                                 PaginationModel* pagination_model,
                                  gfx::NativeView parent)
     : delegate_(delegate),
-      pagination_model_(pagination_model),
+      pagination_model_(NULL),
       model_(delegate->GetModel()),
       search_box_view_(NULL),
       contents_view_(NULL),
       weak_ptr_factory_(this) {
-  // Starts icon loading early.
-  PreloadIcons(parent);
-
   SetLayoutManager(new views::BoxLayout(views::BoxLayout::kVertical,
                                         kInnerPadding,
                                         kInnerPadding,
                                         kInnerPadding));
 
   search_box_view_ = new SearchBoxView(this, delegate);
   AddChildView(search_box_view_);
   AddContentsView();
   if (app_list::switches::IsExperimentalAppListEnabled())
     AddChildView(new ContentsSwitcherView(contents_view_));
+  pagination_model_ = contents_view_->apps_container_view()
+                          ->apps_grid_view()
+                          ->GetPaginationModel();

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

Can we get rid of |pagination_model_| as a member and replace it with an
accessor?

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

hehe - that was going to be first comment too :). I tend to prefer accessors
into children rather than weak pointers to their members.  (wow that sentence
sounds so wrong).  It, uh, makes the ownership very clear rather than having to
rely on comments.

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

Done.

+
+  // Starts icon loading early.
+  PreloadIcons(parent);
 }
 
 void AppListMainView::AddContentsView() {
-  contents_view_ = new ContentsView(
-      this, pagination_model_, model_, delegate_);
+  contents_view_ = new ContentsView(this, model_, delegate_);
   AddChildViewAt(contents_view_, kContentsViewIndex);
 
   search_box_view_->set_contents_view(contents_view_);
 
+  pagination_model_ = contents_view_->apps_container_view()
+                          ->apps_grid_view()
+                          ->GetPaginationModel();
+
 #if defined(USE_AURA)
   contents_view_->SetPaintToLayer(true);
   contents_view_->SetFillsBoundsOpaquely(false);
   contents_view_->layer()->SetMasksToBounds(true);
 #endif
 }
 
 AppListMainView::~AppListMainView() {
   pending_icon_loaders_.clear();
 }
@@ skipping 27 matching lines @@
 }
 
 void AppListMainView::Prerender() {
   contents_view_->Prerender();
 }
 
 void AppListMainView::ModelChanged() {
   pending_icon_loaders_.clear();
   model_ = delegate_->GetModel();
   search_box_view_->ModelChanged();
+  pagination_model_->SelectPage(0, false /* animate */);

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

this is perhaps not needed any more? Or, it needs a comment like ~"communicate
to observers to reset back to foo", since the model will get deleted on the next
line, so it feels weird that you need to set some state on it first.

Also (depending on the reasons), it might be better to do this in
~AppsGridView() or similar anyway

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

Ah I see. The reason this was there before was because switching profiles would
otherwise take you to the app list page for the previous profile (because the
PaginationModel was global).

I'm actually fixing this hack because now a fresh PaginationModel is created for
the new profile -- as it should be!

   delete contents_view_;
   contents_view_ = NULL;
-  pagination_model_->SelectPage(0, false /* animate */);
+  pagination_model_ = NULL;
   AddContentsView();
   Layout();
 }
 
 void AppListMainView::OnContentsViewShowStateChanged() {
   search_box_view_->SetVisible(contents_view_->show_state() !=
                                ContentsView::SHOW_START_PAGE);
 }
 
 void AppListMainView::OnStartPageSearchButtonPressed() {
@@ skipping 11 matching lines @@
   return delegate_->ShouldCenterWindow();
 }
 
 void AppListMainView::PreloadIcons(gfx::NativeView parent) {
   float scale_factor = 1.0f;
   if (parent)
     scale_factor = ui::GetScaleFactorForNativeView(parent);
 
   // |pagination_model| could have -1 as the initial selected page and
   // assumes first page (i.e. index 0) will be used in this case.
   const int selected_page = std::max(0, pagination_model_->selected_page());

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

Oh no... I just realised that this will be broken now. Previously, it would
preload the icons for the page that was pre-set by AppListController. Now, this
code will run before AppListController sets the page, which means we could be
preloading icons for the wrong page.

I will have to think about this. Perhaps the solution could involve removing
this dependency on PaginationModel.

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

This function might even be missing an update for folders/app list sync. Likely
it's currently scheduling the first 16 items according to extension_sorting.cc,
before sync actually kicks in (i.e. the first 16 items according to
chrome://apps which is no longer kept in step with the app list).

Since CrOS can't call this until the app list is shown, it's probably better for
this to be done in the model builder in any case. But for that to work, you'd
need all items to be synced first to ensure they're at the correct position. And
AFAIK, there's no way of telling when the "last" item has been synced to
determine if it should be on the first page or not..

If my hunch (i.e. loading the first 16 chrome://apps rather than the first 16
app list apps) is correct, I'd suggest just removing this function.

I think to fix this, we need to go all the way, and "pickle" some state preload
state (separate fron sync), since we can't rely on sync to be ready the the app
list is first built.

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

I would object to removing the code. It is important for ChromeOS where the view
is created when showing it. Removing the code would bring back the icon flashing
bug (http://crbug.com/161025).

Since the code is probably only needed for ChromeOS, we can probably move the
code to ChromeOS's AppListController, close to where the page gets set.

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

Had a chat with tapted@ about this. We've decided that the easiest way forward
is to keep this code here, but remove the use of pagination_model and always
assume the first page.

Since the preload only matters if the app list is opened in the first few
seconds after starting ChromeOS, the first page is all we ever need to preload.
On subsequent opens, all the icons will already be preloaded, so this will have
no effect. The only problem is folders -- if folders sync down in the background
while the app list is closed, then you may see blank icons flash if you open the
app list to a non-first page. We agreed that this case is too obscure to be very
worried about.

To fix it properly will be really complicated -- for one thing, the
tiles_per_page is already wrong for the centered version of the app list
(--enable-centered-app-list), and moving this code to AppListController, before
constructing the AppListView, means it will be difficult to figure out the
correct tiles_per_page. By only preloading the first page, the tiles-per_page
isn't needed for calculating the start index, only the end index (and we can
just assume the centered app list amount of tiles per page).

I will do this in a separate (precursor) CL.

[Matt Giuca, 2014/06/03 03:58:26]

Actually, it isn't *that* hard/messy to pass the page number down into
AppListMainView. In doing this, we get the correct page preloaded without having
to change the architecture or justify a sub-optimal behaviour in obscure cases.
(Done)

[xiyuan, 2014/06/03 04:10:35]

This not true for ChromeOS. The preload is needed because the ImageSkia icon we
get from extensions::IconImage is just a placeholder. The real image loading
does not happen until its GetRepresentation is called. On ChromeOS, AppListView
is re-created every time user invokes it, which means it is always initialized
with placeholders. Without the preloading, user would see an icon flashing
(blank -> real icon) almost always. And we would have to preload the correct
page (or just preload everything.)

On other platforms, since AppListView is kept around, this preload is probably
not needed. 
...
And done while I am typing this comment. Oh well, I'll just still keep it since
I think it might be useful to you guys too. :)

[Matt Giuca, 2014/06/03 06:51:43]

Hi, good that I did it properly so this discussion doesn't really matter. But
just for future reference:

On 2014/06/03 04:10:35, xiyuan wrote:
I don't think this is really right. The icons are stored in the model, not the
view. So once they are all loaded up (which seems to happen in the background),
it doesn't matter whether you tear down the AppListView and recreate it -- you
don't need to preload anything.

This is my understanding. There are several edge cases where new icons are added
while the app list is closed, but in the common cases I don't think you'll
really need to preload anything other than the first page.

But, whatever, I'm preloading the correct page now.

[xiyuan, 2014/06/03 16:10:35]

You are right. I forgot that AppListView no longer owns the model now.

 
   const int tiles_per_page = kPreferredCols * kPreferredRows;
   const int start_model_index = selected_page * tiles_per_page;
   const int end_model_index =
       std::min(static_cast<int>(model_->top_level_item_list()->item_count()),
                start_model_index + tiles_per_page);
 
   pending_icon_loaders_.clear();
   for (int i = start_model_index; i < end_model_index; ++i) {
     AppListItem* item = model_->top_level_item_list()->item_at(i);
@@ skipping 63 matching lines @@
   // Resubmit the query via a posted task so that all observers for the
   // uninstall notification are notified.
   base::MessageLoop::current()->PostTask(
       FROM_HERE,
       base::Bind(&AppListMainView::QueryChanged,
                  weak_ptr_factory_.GetWeakPtr(),
                  search_box_view_));
 }
 
 }  // namespace app_list