Store AppItems as pages in AppListModel.

Store AppItems as pages in AppListModel.

* Adds a StringOrdinal to AppListItemModel to track item order
* Makes app_items_ private and adds model accessors to add/delete/move
* Adds observers to AppListModelObserver in lieu of ListModelObservers
* Makes app_items_ a vector, one entry per page of items

BUG=305024

[stevenjb, 2013-10-18 01:04:07]

This still needs some more unit tests, but I wanted to get an early review by
the Sydney folks before the weekend.

The intention here is to change AppListModel to support pages that might have <
16 items, without changing the views (which will currently break, or at least
behave strangely, if any of the pages actually have < 16 items).

The next step would be to sync the models, ignoring the page initially so that
we don't have to worry about items not existing on some platforms. This should
get us to a stable state for 32 where app list order is synced again, but the
behavior is roughly the same as in 31.

(I did the model changes first because I wanted to have a working list model
before developing the sync model).

At that point we can implement folders, disabled items, and incomplete pages
behind a flag as a new feature, without having to maintain two different
AppListModel implementations.

I hope that all makes sense. Feedback welcome!

[benwells, 2013-10-18 04:19:47]

On 2013/10/18 01:04:07, stevenjb wrote:
> This still needs some more unit tests, but I wanted to get an early review by
> the Sydney folks before the weekend.
> 
> The intention here is to change AppListModel to support pages that might have
<
> 16 items, without changing the views (which will currently break, or at least
> behave strangely, if any of the pages actually have < 16 items).
> 
> The next step would be to sync the models, ignoring the page initially so that
> we don't have to worry about items not existing on some platforms. This should
> get us to a stable state for 32 where app list order is synced again, but the
> behavior is roughly the same as in 31.

Could you outline what the differences would be?

> 
> (I did the model changes first because I wanted to have a working list model
> before developing the sync model).
> 
> At that point we can implement folders, disabled items, and incomplete pages
> behind a flag as a new feature, without having to maintain two different
> AppListModel implementations.

Is this alternative possible?
- keep the current AppListModel implementation hooked up to the previous sync
and ordering system
- have the new AppListModel implementation, sync and ordering logic, folders and
other behavior changes there but all behind a flag.

As the previous implementation isn't really changing maintaining it isn't an
issue.

> 
> I hope that all makes sense. Feedback welcome!

[koz (OOO until 15th September), 2013-10-18 05:17:17]

It's a bit hard to see the forest for the trees here. What does having support
in the model for pages with < 16 items allow us to do, exactly? If the view
doesn't support pages with < 16 items, but the model does, won't that cause
problems? (ie: what happens now if we delete an item on the first page if there
are pages after it?) How does ignoring the page ordinal allow us to not worry
about items that don't exist on all platforms?

A design doc would be helpful for understanding the high level direction you are
taking.

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (right):

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/extension_app_model_builder.cc:168:
model_->DeleteItemAt(0, i);
Should this be DeleteItemAt(p, i) ?

Also, why do we delete only items with type kAppType. Won't all the items need
to be removed in a profile switch?

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc (right):

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc:291:
EXPECT_EQ(kDefaultAppCount, model_->GetAppItems(0).item_count());
GetAppItems() -> GetPage(), or similar?

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc (right):

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc:82: void
CopyAppListModel(const AppListModel* src, AppListModel* dest) {
These two methods aren't just used for testing. FastShowPickler is dead code
currently but the intent of it is to unpickle a model and then copy that over an
existing model. It's true one could change AppListView to not create its own
model, then you wouldn't need to copy over it, but until then this should live
with the real code.

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list.gyp
File ui/app_list/app_list.gyp (right):

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list.gyp...
ui/app_list/app_list.gyp:161: # TODO: Remove this dependency. See comment for
views_unitiests.
typo - views_unittests

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:127: // Note: ui::ListModel is not a sorted list.
We seem to assume that the items are ordered below (the "sort_order <
apps->GetItemAt(item_idx)->GetSortOrder()" bit). Is this comment out of date?

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:130: bool done = item_pages_.size() == 0;
nit: item_pages_.empty()

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:83: // <= the number of pages, i.e. at most one new
page will be added.
What does it return when the page isn't full? -1?
When the page is full, it returns the index of the next page, I assume it
doesn't add the item then? Or does it keep moving along until it finds a page
that has capacity, adds it there and returns that page's index? Could you
clarify in the comment?

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/test/app_lis...
File ui/app_list/test/app_list_model_unittest.cc (right):

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/test/app_lis...
ui/app_list/test/app_list_model_unittest.cc:94: base::RunLoop run_loop;
Why do we need to have a message loop in this test? I thought model changes
propagated synchronously.

[xiyuan, 2013-10-18 17:29:35]

I have a proposal:
- Keep the items still as a ListModel in AppListModel
- Add page ordinal and position ordinal to AppListItemModel
- Let the UI code (AppsGridView) to create/maintain the paging structure from
the two ordinals in the item model

I think this way would be easy to hook with the old ordering system and switch
to the new system. The work would then be mostly in AppsGridView. For M32, we
could minimize the changes there if we could create model with the same page
ordinal (or simply ignore it for now).

What do you guys think?

[stevenjb, 2013-10-18 19:30:55]

The intention behind putting the concept of pages into the model is to keep
that behavior consistent across platforms. Otherwise I do not think we will
be able to reliably sync pages.

I'll try to put together a document to help share my thoughts here. We can
use that as a platform for further discussion.




On Fri, Oct 18, 2013 at 10:29 AM, <xiyuan@chromium.org> wrote:

> I have a proposal:
> - Keep the items still as a ListModel in AppListModel
> - Add page ordinal and position ordinal to AppListItemModel
> - Let the UI code (AppsGridView) to create/maintain the paging structure
> from
> the two ordinals in the item model
>
> I think this way would be easy to hook with the old ordering system and
> switch
> to the new system. The work would then be mostly in AppsGridView. For M32,
> we
> could minimize the changes there if we could create model with the same
> page
> ordinal (or simply ignore it for now).
>
> What do you guys think?
>
>
https://codereview.chromium.**org/27438002/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2013-10-18 19:39:23]

On Thu, Oct 17, 2013 at 9:19 PM, <benwells@chromium.org> wrote:

> On 2013/10/18 01:04:07, stevenjb wrote:
>
>> This still needs some more unit tests, but I wanted to get an early
>> review by
>> the Sydney folks before the weekend.
>>
>
>  The intention here is to change AppListModel to support pages that might
>> have
>>
> <
>
>> 16 items, without changing the views (which will currently break, or at
>> least
>> behave strangely, if any of the pages actually have < 16 items).
>>
>
>  The next step would be to sync the models, ignoring the page initially so
>> that
>> we don't have to worry about items not existing on some platforms. This
>> should
>> get us to a stable state for 32 where app list order is synced again, but
>> the
>> behavior is roughly the same as in 31.
>>
>
> Could you outline what the differences would be?
>
> The only differences should be any intentional changes to how we order
local items.


>
>
>  (I did the model changes first because I wanted to have a working list
>> model
>> before developing the sync model).
>>
>
>  At that point we can implement folders, disabled items, and incomplete
>> pages
>> behind a flag as a new feature, without having to maintain two different
>> AppListModel implementations.
>>
>
> Is this alternative possible?
> - keep the current AppListModel implementation hooked up to the previous
> sync
> and ordering system
> - have the new AppListModel implementation, sync and ordering logic,
> folders and
> other behavior changes there but all behind a flag.
>
> As the previous implementation isn't really changing maintaining it isn't
> an
> issue.
>
> I can certainly hook back up the existing existing extensions ordering
sync. Given that there are questions about the right next step I will go
ahead and do that today so that ToT behavior does not remain broken for
very long, and so that there is not so much time pressure on getting the
sync changes to support folders in.

I'm not sure about supporting two parallel models, I think it will be
challenging to do that without separate View implementations also. My
intention is to create a model that can support both the current behavior
and the new behavior.



>
>
>  I hope that all makes sense. Feedback welcome!
>>
>
>
>
https://codereview.chromium.**org/27438002/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2013-10-18 19:58:29]

On Thu, Oct 17, 2013 at 10:04 PM,  <stevenjb@chromium.org> wrote:
> Reviewers: jennyz, xiyuan, benwells, koz, calamity,
>
> Message:
> This still needs some more unit tests, but I wanted to get an early review
> by
> the Sydney folks before the weekend.
>
> The intention here is to change AppListModel to support pages that might
> have <
> 16 items, without changing the views (which will currently break, or at
> least
> behave strangely, if any of the pages actually have < 16 items).
>
> The next step would be to sync the models, ignoring the page initially so
> that
> we don't have to worry about items not existing on some platforms. This
> should
> get us to a stable state for 32 where app list order is synced again, but
> the
> behavior is roughly the same as in 31.
>
> (I did the model changes first because I wanted to have a working list model
> before developing the sync model).
>
> At that point we can implement folders, disabled items, and incomplete pages
> behind a flag as a new feature, without having to maintain two different
> AppListModel implementations.
>
> I hope that all makes sense. Feedback welcome!
>
>
> Description:
> Store AppItems as pages in AppListModel.
>
> * Adds a StringOrdinal to AppListItemModel to track item order
> * Makes app_items_ private and adds model accessors to add/delete/move
> * Adds observers to AppListModelObserver in lieu of ListModelObservers
> * Makes app_items_ a vector, one entry per page of items
>
> BUG=305024
>
> Please review this at https://codereview.chromium.org/27438002/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+781, -224 lines):
>   M ash/shell/app_list.cc
>   M chrome/browser/ui/app_list/extension_app_model_builder.cc
>   M chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc
>   M chrome/browser/ui/app_list/fast_show_pickler.h
>   M chrome/browser/ui/app_list/fast_show_pickler.cc
>   M chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc
>   M ui/app_list/DEPS
>   M ui/app_list/app_list.gyp
>   M ui/app_list/app_list_item_model.h
>   M ui/app_list/app_list_model.h
>   M ui/app_list/app_list_model.cc
>   M ui/app_list/app_list_model_observer.h
>   M ui/app_list/cocoa/apps_grid_controller.mm
>   M ui/app_list/cocoa/apps_grid_controller_unittest.mm
>   A ui/app_list/test/app_list_model_unittest.cc
please, move this file near app_list_model.h in the directory above.

>   M ui/app_list/test/app_list_test_model.h
>   M ui/app_list/test/app_list_test_model.cc
>   M ui/app_list/views/app_list_main_view.cc
>   M ui/app_list/views/apps_grid_view.h
>   M ui/app_list/views/apps_grid_view.cc
>   M ui/app_list/views/apps_grid_view_unittest.cc
>   M ui/views/test/views_test_base.h
>

-- 
Thiago

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2013-10-18 22:14:26]

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/extension_app_model_builder.cc (right):

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/extension_app_model_builder.cc:168:
model_->DeleteItemAt(0, i);
On 2013/10/18 05:17:17, koz wrote:
> Should this be DeleteItemAt(p, i) ?
> 
> Also, why do we delete only items with type kAppType. Won't all the items need
> to be removed in a profile switch?

Yes, thanks. Done.

This code will require some refactoring with the new sync, but in principle
ExtensionAppModelBuilder shouldn't be dealing with folders (or bookmarks if we
add those).

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc (right):

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc:291:
EXPECT_EQ(kDefaultAppCount, model_->GetAppItems(0).item_count());
On 2013/10/18 05:17:17, koz wrote:
> GetAppItems() -> GetPage(), or similar?
-> GetAppItemsForPage()

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc (right):

https://codereview.chromium.org/27438002/diff/114001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/test/fast_show_pickler_unittest.cc:82: void
CopyAppListModel(const AppListModel* src, AppListModel* dest) {
On 2013/10/18 05:17:17, koz wrote:
> These two methods aren't just used for testing. FastShowPickler is dead code
> currently but the intent of it is to unpickle a model and then copy that over
an
> existing model. It's true one could change AppListView to not create its own
> model, then you wouldn't need to copy over it, but until then this should live
> with the real code.

I see. I'll move it back with a comment; I'll admit it's unclear to me what this
is/was/will be used for. I had assumed this function was only used for testing
since that's the only place it was called from. Is there an Issue for the change
that will use this?

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list.gyp
File ui/app_list/app_list.gyp (right):

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list.gyp...
ui/app_list/app_list.gyp:161: # TODO: Remove this dependency. See comment for
views_unitiests.
On 2013/10/18 05:17:17, koz wrote:
> typo - views_unittests

Done.

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:127: // Note: ui::ListModel is not a sorted list.
On 2013/10/18 05:17:17, koz wrote:
> We seem to assume that the items are ordered below (the "sort_order <
> apps->GetItemAt(item_idx)->GetSortOrder()" bit). Is this comment out of date?
Yeah, this kind of evolved badly. Made this simpler and more efficient too.

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:130: bool done = item_pages_.size() == 0;
On 2013/10/18 05:17:17, koz wrote:
> nit: item_pages_.empty()

Done.

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:83: // <= the number of pages, i.e. at most one new
page will be added.
On 2013/10/18 05:17:17, koz wrote:
> What does it return when the page isn't full? -1?
> When the page is full, it returns the index of the next page, I assume it
> doesn't add the item then? Or does it keep moving along until it finds a page
> that has capacity, adds it there and returns that page's index? Could you
> clarify in the comment?

Clarified.

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/test/app_lis...
File ui/app_list/test/app_list_model_unittest.cc (right):

https://codereview.chromium.org/27438002/diff/114001/ui/app_list/test/app_lis...
ui/app_list/test/app_list_model_unittest.cc:94: base::RunLoop run_loop;
On 2013/10/18 05:17:17, koz wrote:
> Why do we need to have a message loop in this test? I thought model changes
> propagated synchronously.
I suppose that's true. I tend to assume that an Observer might be asynchrnous,
but in this case it is not. Removed.

[xiyuan, 2013-10-18 23:04:07]

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_ite...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_ite...
ui/app_list/app_list_item_model.h:52: position_ = new_position;
Should we add an observer method for this as well? It would be useful when the
position is from sync instead of MoveItem.

On the sync topic, we might want to keep track of the page ordinal here too. And
having a method in AppListModel to rebuild the paging structure from it.

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:162: for (; item_idx < apps->item_count();
++item_idx) {
Can we make this loop of insertion an item part of ItemPage? That is, add an
AddItem method to ItemPage which insert an item in the page based on
item->GetSortOrder().

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:72: AppListItemModel* GetItemAt(size_t page_idx,
size_t item_idx);
Propose to use page id instead of index. It would be easier to use page id to
track the page than index. Maybe put a TODO if we don't get to it in this CL.

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model_observer.h (right):

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
ui/app_list/app_list_model_observer.h:24: virtual void OnListItemsAdded(size_t
page_index,
nit: Rename to OnPageItemsAdded? OnListItemxxxx is bit confusing.

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

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1183: size_t offset = page_index *
AppListModel::GetNumAppsPerPage();
int offset = GetModelIndexFromIndex(Index(page_index, 0));

GetModelIndexFromIndex does a similar thing but with consideration of a start
page.

--show-app-list-start-page would trigger the start page.

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1200: size_t index = page_index *
AppListModel::GetNumAppsPerPage() + start;
GetModelIndexFromIndex(Index(page_index, start));

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1216: size_t offset = page_index *
AppListModel::GetNumAppsPerPage();
use GetModelIndexFromIndex

[stevenjb, 2013-10-19 01:04:42]

Thanks for all the feedback!

I plan to think about this some more over the weekend and write up a doc.
Hopefully the doc will explain why I think the model changes are necessary for
syncing app lists across platforms.

My current thinking is that with some small changes to the existing Views and
Cocoa code, we can make this model stable with or without gaps at the ends of
pages, then tackle sync leisurely (https://codereview.chromium.org/29613004/
should address the short term sync issues).

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_ite...
File ui/app_list/app_list_item_model.h (right):

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_ite...
ui/app_list/app_list_item_model.h:52: position_ = new_position;
On 2013/10/18 23:04:08, xiyuan wrote:
> Should we add an observer method for this as well? It would be useful when the
> position is from sync instead of MoveItem.
> 
> On the sync topic, we might want to keep track of the page ordinal here too.
And
> having a method in AppListModel to rebuild the paging structure from it.

Yes, when we sync this will need to be more than a trivial setter. Not sure we
need an observer, but thus will definitely change to SetPosition() and will take
a page index as well as an item ordinal.

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.cc (right):

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.cc:162: for (; item_idx < apps->item_count();
++item_idx) {
On 2013/10/18 23:04:08, xiyuan wrote:
> Can we make this loop of insertion an item part of ItemPage? That is, add an
> AddItem method to ItemPage which insert an item in the page based on
> item->GetSortOrder().

That sounds reasonable. I kind of wanted to keep the logic together for now, but
can see moving it.

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model.h (right):

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
ui/app_list/app_list_model.h:72: AppListItemModel* GetItemAt(size_t page_idx,
size_t item_idx);
On 2013/10/18 23:04:08, xiyuan wrote:
> Propose to use page id instead of index. It would be easier to use page id to
> track the page than index. Maybe put a TODO if we don't get to it in this CL.

I'll think about that. I can see where it makes sense, but it might also make
the View implementation more difficult. Maybe we'll want both?

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
File ui/app_list/app_list_model_observer.h (right):

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/app_list_mod...
ui/app_list/app_list_model_observer.h:24: virtual void OnListItemsAdded(size_t
page_index,
On 2013/10/18 23:04:08, xiyuan wrote:
> nit: Rename to OnPageItemsAdded? OnListItemxxxx is bit confusing.

Sounds reasonable.

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

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1183: size_t offset = page_index *
AppListModel::GetNumAppsPerPage();
On 2013/10/18 23:04:08, xiyuan wrote:
> int offset = GetModelIndexFromIndex(Index(page_index, 0));
> 
> GetModelIndexFromIndex does a similar thing but with consideration of a start
> page.
> 
> --show-app-list-start-page would trigger the start page.

Ah, thanks. Will do.

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1200: size_t index = page_index *
AppListModel::GetNumAppsPerPage() + start;
On 2013/10/18 23:04:08, xiyuan wrote:
> GetModelIndexFromIndex(Index(page_index, start));

Ditto

https://codereview.chromium.org/27438002/diff/345001/ui/app_list/views/apps_g...
ui/app_list/views/apps_grid_view.cc:1216: size_t offset = page_index *
AppListModel::GetNumAppsPerPage();
On 2013/10/18 23:04:08, xiyuan wrote:
> use GetModelIndexFromIndex

Ditto