app_list: Use NTP ordering in ExtensionSorting to sort apps.

app_list: Use NTP ordering in ExtensionSorting to sort apps.

BUG=117090, 149750
TEST=Verify apps in app list show up in the same order as NTP on other platforms.

R=sky@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158483

[sky, 2012-09-21 23:10:52]

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:25: bool AppPrecedes(const
ExtensionAppItem* app1, const ExtensionAppItem* app2) {
Is there some way to share this with what the NTP does so that the two are
always in sync?

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:121: *end =
model_->item_count() - 1;
Is it possible item_count is 0, so that this overflows?

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:138: for (size_t i = 0; i <
apps.size(); ++i) {
Add a comment as to what this is doing.

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:146: for (size_t j =
insert_index; j <= end; ++j) {
Can this use FindApp?

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:172: syncer::StringOrdinal
page_ordinal = app->GetPageOrdinal();
const syncer::StringOrdinal& ?

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:174: if
(!AppPrecedes(GetAppAt(i), app)) {
It would be nice to use a binary search here.

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:215: DCHECK(item->type() ==
ChromeAppListItem::TYPE_APP);
DCHECK_EQ

[xiyuan, 2012-09-24 20:39:40]

CL updated. Please take another look.

Thanks.

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
File chrome/browser/ui/app_list/apps_model_builder.cc (right):

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:25: bool AppPrecedes(const
ExtensionAppItem* app1, const ExtensionAppItem* app2) {
On 2012/09/21 23:10:53, sky wrote:
> Is there some way to share this with what the NTP does so that the two are
> always in sync?

NTP sorts its app list in js.

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:121: *end =
model_->item_count() - 1;
On 2012/09/21 23:10:53, sky wrote:
> Is it possible item_count is 0, so that this overflows?

There should have at least two bundled apps. So item_count should never be 0.

I guess I probably should simplify the logic here until we do have other items
in the list.

Removed this function.

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:138: for (size_t i = 0; i <
apps.size(); ++i) {
On 2012/09/21 23:10:53, sky wrote:
> Add a comment as to what this is doing.

Done.

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:146: for (size_t j =
insert_index; j <= end; ++j) {
On 2012/09/21 23:10:53, sky wrote:
> Can this use FindApp?

FindApp is based on extension id. Here we are using just pointer value and only
in the unsorted part of the list.

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:172: syncer::StringOrdinal
page_ordinal = app->GetPageOrdinal();
On 2012/09/21 23:10:53, sky wrote:
> const syncer::StringOrdinal& ?

Removed. It's left-over from a CL that passes |page_ordianl| to app list item
model. It's no needed in this CL. 

Same goes to the TODO below.

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:174: if
(!AppPrecedes(GetAppAt(i), app)) {
On 2012/09/21 23:10:53, sky wrote:
> It would be nice to use a binary search here.

Done.

http://codereview.chromium.org/10957048/diff/1/chrome/browser/ui/app_list/app...
chrome/browser/ui/app_list/apps_model_builder.cc:215: DCHECK(item->type() ==
ChromeAppListItem::TYPE_APP);
On 2012/09/21 23:10:53, sky wrote:
> DCHECK_EQ

Done.

[sky, 2012-09-24 21:11:17]

LGTM

[commit-bot: I haz the power, 2012-09-24 22:54:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xiyuan@chromium.org/10957048/4001

[commit-bot: I haz the power, 2012-09-25 01:09:11]

Change committed as 158483