Refactor app list so AppsGridView owns the PaginationModel.

Refactor app list so AppsGridView owns the PaginationModel. 

Previously, PaginationModel was created by platform-specific code
(either AppListController or AppListShower) and passed down through 6
layers to AppsGridView. Now, AppsGridView creates its own
PaginationModel, and many of the layers don't need to know about it.

Some classes do need to access the PaginationModel (most of these 
dependencies can probably be broken later). These now pull the 
PaginationModel from the AppsGridView instead of caching it on the way 
down. 

On ChromeOS, the lifetime of the PaginationModel is now tied to the 
visibility of the app list (was previously the lifetime of the process). 
To get around this, AppListController must now manually remember the 
current page, passing it to new PaginationModels as they are created, as 
well as adding itself as an observer. This is still a net win, as the 
ownership is in the right place, and several classes no longer depend on 
PaginationModel, with more dependency cuts possible in the future. 

BUG=377626
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274785

[Matt Giuca, 2014-05-30 09:08:28]

Hi, this is a reasonably annoying refactor because it introduces a bit of extra
mess (especially in AppListController -- having to explicitly track the page
number as a separate int). But it cleans up quite a few dependencies on
PaginationModel, and more to come (we should be able to clean up the use of
PaginationModel in ContentsView, AppListMainView, and some of
AppListController).

tapted: Please review ui/app_list and chrome/browser/ui/app_list.
pkotwicz: Please review ash/wm/app_list_controller.*

benwells, xiyuan, calamity: FYI.

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

Good change to move PaginationModel into views.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:106: ->GetPaginationModel();
Can we get rid of |pagination_model_| as a member and replace it with an
accessor?

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.h:103: PaginationModel* pagination_model_; 
// Owned by AppListGridView.
nit: AppListGridView -> AppsGridView

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:103: PaginationModel*
PaginationModel() const;
nit: accessor should be named in lower case, like pagination_model. And no
"const" because it does not return a const object.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:390: // should not be called during
destruction.)
RemoveAllChildViews could be called from a view's dtor. What problem did you
see?

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:135: PaginationModel* GetPaginationModel()
const {
GetPaginationModel -> pagination_model and get rid of "const"

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:91: PaginationModel* pagination_model_;  //
Owned by AppListGridView.
AppListGridView -> AppsGridView

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/page_s...
File ui/app_list/views/page_switcher.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/page_s...
ui/app_list/views/page_switcher.h:57: PaginationModel* model_;  // Owned by
AppListController; may be NULL.
Update comments as |model_| is ownded by AppsGridView now.

[tapted, 2014-06-02 04:02:20]

https://codereview.chromium.org/302803002/diff/40001/ash/wm/app_list_controll...
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ash/wm/app_list_controll...
ash/wm/app_list_controller.cc:288: pagination_model_ =
view_->app_list_main_view()
I think this justifies a GetPaginationModel() accessor on AppListView :) - you
can get rid of a lot of #includes that way. And even remove the
pagination_model_ member on AppListController; relying instead on
view_->GetPaginationModel()

https://codereview.chromium.org/302803002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_shower_views.h (right):

https://codereview.chromium.org/302803002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_shower_views.h:9: #include
"ui/app_list/pagination_model.h"
nit: can this go?

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:106: ->GetPaginationModel();
On 2014/05/30 15:47:47, xiyuan wrote:
> Can we get rid of |pagination_model_| as a member and replace it with an
> accessor?

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.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:169: pagination_model_->SelectPage(0,
false /* animate */);
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

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.h:30: class PaginationModel;
Depending what else uses it.. this could possibly be removed from here.

That is, I think (critically) only AppListView, AppsGridView and PageSwitcher
now need knowledge about PaginationView. If it can be purged from all other
files in ui/app_list/ then that would be a win IMO

(note AppListView* doesn't _really_ need to know, but I think something at the
top level in /ui/app_list needs to expose it to avoid an explosion of
dependencies)

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:390: // should not be called during
destruction.)
On 2014/05/30 15:47:47, xiyuan wrote:
> RemoveAllChildViews could be called from a view's dtor. What problem did you
> see?

(#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).

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:13: #include "base/memory/ref_counted.h"
nit: unused?

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:469: scoped_ptr<PaginationModel>
pagination_model_;
I'd consider trying this out as a non-pointer data member. Sure, it will expose
more stuff in the header, but that alone isn't typically a reason to use a
scoped_ptr member.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:72:
pagination_model_->SetTransitionDurations(
This would be nicer in AppsGridView IMO (or even encapsulated entirely in
PaginationModel, but I think it might be used for some tests). some constants
might need to be moved, but I don't seem them at a quick glance - they might
already be in app_list_constants.h

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

https://codereview.chromium.org/302803002/diff/40001/ash/wm/app_list_controll...
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ash/wm/app_list_controll...
ash/wm/app_list_controller.cc:288: pagination_model_ =
view_->app_list_main_view()
Done & Done.

https://codereview.chromium.org/302803002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_shower_views.h (right):

https://codereview.chromium.org/302803002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_shower_views.h:9: #include
"ui/app_list/pagination_model.h"
On 2014/06/02 04:02:21, tapted wrote:
> nit: can this go?

Done.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:106: ->GetPaginationModel();
On 2014/06/02 04:02:21, tapted wrote:
> On 2014/05/30 15:47:47, xiyuan wrote:
> > Can we get rid of |pagination_model_| as a member and replace it with an
> > accessor?
> 
> 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.

Done.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:169: pagination_model_->SelectPage(0,
false /* animate */);
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!

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:204: const int selected_page =
std::max(0, pagination_model_->selected_page());
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.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.h:30: class PaginationModel;
Yeah, I was planning to clean up the use of PaginationModel in AppListMainView
and ContentsView in a follow-up CL. Their use of PaginationModel is non-trivial,
but I think it could be moved.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.h:103: PaginationModel* pagination_model_; 
// Owned by AppListGridView.
On 2014/05/30 15:47:47, xiyuan wrote:
> nit: AppListGridView -> AppsGridView

Done.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view_unittest.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view_unittest.cc:103: PaginationModel*
PaginationModel() const;
const: done.
lowercase: My reading of the style guide is that this only applies to methods
that simply return an attribute. This is a more complex getter. I think the
correct thing to do is to keep CamelCase but add "Get" to the front.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:390: // should not be called during
destruction.)
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.)

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:13: #include "base/memory/ref_counted.h"
On 2014/06/02 04:02:21, tapted wrote:
> nit: unused?

Done.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:135: PaginationModel* GetPaginationModel()
const {
On 2014/05/30 15:47:47, xiyuan wrote:
> GetPaginationModel -> pagination_model and get rid of "const"

Done.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:469: scoped_ptr<PaginationModel>
pagination_model_;
On 2014/06/02 04:02:21, tapted wrote:
> I'd consider trying this out as a non-pointer data member. Sure, it will
expose
> more stuff in the header, but that alone isn't typically a reason to use a
> scoped_ptr member.

Done.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/conten...
ui/app_list/views/contents_view.cc:72:
pagination_model_->SetTransitionDurations(
On 2014/06/02 04:02:21, tapted wrote:
> This would be nicer in AppsGridView IMO (or even encapsulated entirely in
> PaginationModel, but I think it might be used for some tests). some constants
> might need to be moved, but I don't seem them at a quick glance - they might
> already be in app_list_constants.h

Done.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/conten...
File ui/app_list/views/contents_view.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/conten...
ui/app_list/views/contents_view.h:91: PaginationModel* pagination_model_;  //
Owned by AppListGridView.
On 2014/05/30 15:47:47, xiyuan wrote:
> AppListGridView -> AppsGridView

Done.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/page_s...
File ui/app_list/views/page_switcher.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/page_s...
ui/app_list/views/page_switcher.h:57: PaginationModel* model_;  // Owned by
AppListController; may be NULL.
On 2014/05/30 15:47:47, xiyuan wrote:
> Update comments as |model_| is ownded by AppsGridView now.

Done.

[tapted, 2014-06-02 08:36:48]

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:204: const int selected_page =
std::max(0, pagination_model_->selected_page());
On 2014/06/02 07:11:23, Matt Giuca wrote:
> 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.

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.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:390: // should not be called during
destruction.)
On 2014/06/02 07:11:23, Matt Giuca wrote:
> (Unless you mean to use the weak ref to decide
> not to call RemoveObserver if the PaginationModel has already been destroyed.)

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.

https://codereview.chromium.org/302803002/diff/70001/ash/wm/app_list_controll...
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/302803002/diff/70001/ash/wm/app_list_controll...
ash/wm/app_list_controller.cc:21: #include
"ui/app_list/views/apps_container_view.h"
nit: these 3 includes, and app_list_main_view.h above might be unnecessary now

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

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:204: const int selected_page =
std::max(0, pagination_model_->selected_page());
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.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:390: // should not be called during
destruction.)
On 2014/06/02 07:11:23, Matt Giuca wrote:
> 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).
> 

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.

https://codereview.chromium.org/302803002/diff/70001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/302803002/diff/70001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:295: ->pagination_model();
This repeats many times. Why not moving relevant GetAppsPaginationModel in
AppListMainView/ContentsView to public and call them?

https://codereview.chromium.org/302803002/diff/70001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/302803002/diff/70001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:75: AppsGridView(AppsGridViewDelegate*
delegate);
nit: explicit

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

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:204: const int selected_page =
std::max(0, pagination_model_->selected_page());
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.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:390: // should not be called during
destruction.)
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.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:13: #include "base/memory/ref_counted.h"
Actually, ref_counted.h is used (not by my code, but by some existing code), so
I'm adding this as include-what-you-use for existing code.

Don't need scoped_ptr.h any more, though.

https://codereview.chromium.org/302803002/diff/70001/ash/wm/app_list_controll...
File ash/wm/app_list_controller.cc (right):

https://codereview.chromium.org/302803002/diff/70001/ash/wm/app_list_controll...
ash/wm/app_list_controller.cc:21: #include
"ui/app_list/views/apps_container_view.h"
On 2014/06/02 08:36:49, tapted wrote:
> nit: these 3 includes, and app_list_main_view.h above might be unnecessary now

Done.

https://codereview.chromium.org/302803002/diff/70001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_view.cc (right):

https://codereview.chromium.org/302803002/diff/70001/ui/app_list/views/app_li...
ui/app_list/views/app_list_view.cc:295: ->pagination_model();
I really don't want to expose GetAppsPaginationModel at all of the intermediate
layers. I am planning to remove the dependency of AppListMainView and
ContentsView on PaginationModel -- the main point of this CL is to have as few
classes depend on PaginationModel as possible (which is why I was reluctant to
add this getter in AppListView in the first place).

I think it's cleaner to use the existing getters to reach down and pull out the
PaginationModel than it is to expose a PaginationModel at each level.

https://codereview.chromium.org/302803002/diff/70001/ui/app_list/views/apps_g...
File ui/app_list/views/apps_grid_view.h (right):

https://codereview.chromium.org/302803002/diff/70001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.h:75: AppsGridView(AppsGridViewDelegate*
delegate);
On 2014/06/02 17:02:08, xiyuan wrote:
> nit: explicit

Done.

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

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:204: const int selected_page =
std::max(0, pagination_model_->selected_page());
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:34]

LGTM!

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:204: const int selected_page =
std::max(0, pagination_model_->selected_page());
On 2014/06/03 01:52:36, Matt Giuca wrote:
> 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.
> 

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. 

> 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.

...
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. :)

[tapted, 2014-06-03 06:33:46]

nice! lgtm

[Matt Giuca, 2014-06-03 06:51:42]

pkotwicz: PTAL at the app_list_controller. Should be fairly stable now. Thanks.

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:204: const int selected_page =
std::max(0, pagination_model_->selected_page());
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:
> 
> 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.)

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.

[pkotwicz, 2014-06-03 15:55:56]

LGTM

[xiyuan, 2014-06-03 16:10:34]

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
File ui/app_list/views/app_list_main_view.cc (right):

https://codereview.chromium.org/302803002/diff/40001/ui/app_list/views/app_li...
ui/app_list/views/app_list_main_view.cc:204: const int selected_page =
std::max(0, pagination_model_->selected_page());
On 2014/06/03 06:51:43, Matt Giuca 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.

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

[Matt Giuca, 2014-06-04 01:36:07]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-06-04 01:36:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/302803002/200001

[Matt Giuca, 2014-06-04 01:36:47]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2014-06-04 01:38:16]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-06-04 01:38:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/302803002/200001

[Matt Giuca, 2014-06-04 01:54:06]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-06-04 01:55:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/302803002/220001

[Matt Giuca, 2014-06-04 03:52:57]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-06-04 03:53:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mgiuca@chromium.org/302803002/240001

[commit-bot: I haz the power, 2014-06-04 07:05:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-04 12:23:48]

Change committed as 274785