Separate the NTP app ordering from the app list app ordering

Separate the NTP app ordering from the app list app ordering

This patch separates the ordering logic for the NTP apps page and the app list app page. This includes adding a new sync field for the app list ordinal. 

BUG=245728
TBR=zea@chromium.org

[calamity, 2013-06-14 02:05:33]

Main changes:
chrome/browser/extensions/extension_ordering.{h,cc}
chrome/browser/extensions/app_list_extension_ordering.{h,cc}

Sync changes are pretty much all in sync/ and
chrome/browser/extensions/app_sync_*.

Tests and test accessories will arrive after review of the api design.

[calamity, 2013-06-18 05:54:20]

Made some of the changes suggested and added tests.

[koz (OOO until 15th September), 2013-06-18 07:41:36]

initial pass

https://codereview.chromium.org/17038002/diff/1/chrome/browser/ui/app_list/ex...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/17038002/diff/1/chrome/browser/ui/app_list/ex...
chrome/browser/ui/app_list/extension_app_item.cc:130: // Handles only
predecessor or only successor case.
This handles more than just that case - it also handles them both being defined.

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:1474:
ordering->SetAppListOrdinalForSync(
Why do we need to Erase() here? Can we not clobber the ordinal with
SetAppListOrdinalForSync()

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:442:
verifier_ordering->InsertAtNextAvailable(extension_misc::kChromeAppId);
This might read better like

MoveChromeAppToEnd(ordering);
MoveChromeAppToEnd(verifier_ordering);

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_ordering.cc (right):

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:28: if
(ordinal.IsValid()) {
nit: no curlies

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:29:
SetAppListOrdinal(*ext_it, ordinal);
It's a bit weird that GetAppListOrdinal() reads from prefs but
SetAppListOrdinal() writes to a map. I suggest renaming GetAppListOrdinal() to
GetAppListOrdinalFromPrefs().

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:41: if
(web_store_app_ordinal.IsValid()) {
no curlies

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:47: if
(web_store_app_ordinal.IsValid()) {
no curlies

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:53: for
(AppListOrdinalMap::iterator ordinal_map_it = app_list_ordinal_map_
nit: break this line after the =

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:57: ExtensionIdSet&
extension_set = ordinal_map_it->second;
I think this function would read better if it made use of a helper function,
something like:

AssignUniqueOrdinalsBetweenBounds(extension_set, lower_bound, upper_bound);

WDYT?

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:61: ++next_it;
next_it = ordinal_map_it + 1 ?

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:93:
syncer::StringOrdinal ordinal = syncer::StringOrdinal::CreateInitialOrdinal();
Consider using the ternary operator here.

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:94: if
(!app_list_ordinal_map_.empty()) {
nit: no curlies

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:102:
syncer::StringOrdinal ordinal = syncer::StringOrdinal::CreateInitialOrdinal();
Consider using the ternary operator here.

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:104: ordinal =
app_list_ordinal_map_.rbegin()->first.CreateAfter();
nit: no curlies

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:112:
InsertAtBack(extension_id);
Do callers actually distinguish between InsertAtNextAvailable() and
InsertAtBack()? I'd remove this function and replace it with calls to
InsertAtBack().

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:127: // We only need
to change the StringOrdinal if there are neighbours.
nit: if there is at least one neighbour?

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:162: if
(ordinal_map_it == app_list_ordinal_map_.end())
DCHECK_NE() ?

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:197:
DCHECK(!Contains(extension_id));
Can we just call Erase(extension_id) instead?

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_ordering.h (right):

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.h:20: // single ordinal.
s/This ordering is.../This class defines the order by assigning each app a
single string ordinal/.

Perhaps expand this comment by saying that this class works on an in-memory map
of ordinals which it uses to resolve conflicts generated by sync and which it
commits to prefs for persistence.

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.h:31: // consistent.
Won't they potentially be inconsistent between calls to
SetAppListOrdinalForSync() and FixSyncCollisions()?

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.h:40: void
SetAppListOrdinalForSync(const std::string& extension_id,
How about UpdateAppListOrdinalFromSync()?

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.h:43: // Resolves any
conflicts that might be created as a result of syncing that
s/result of syncing/result of calls to SetAppListOrdinalForSync()/

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.h:70: bool
ExtensionPrecedes(const std::string& extension1,
nit: extension_id1 and extension_id2

https://codereview.chromium.org/17038002/diff/21029/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_ordering.h:72: // Returns true if
|extension_id| is in the ordering.
nit: newline before this line

[calamity, 2013-06-19 05:09:17]

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ext...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ext...
chrome/browser/extensions/extension_service.cc:1474:
ordering->SetAppListOrdinalForSync(
On 2013/06/18 07:41:36, koz wrote:
> Why do we need to Erase() here? Can we not clobber the ordinal with
> SetAppListOrdinalForSync()

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/syn...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/syn...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:442:
verifier_ordering->InsertAtNextAvailable(extension_misc::kChromeAppId);
On 2013/06/18 07:41:36, koz wrote:
> This might read better like
> 
> MoveChromeAppToEnd(ordering);
> MoveChromeAppToEnd(verifier_ordering);

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_extension_ordering.cc (right):

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:28: if
(ordinal.IsValid()) {
On 2013/06/18 07:41:36, koz wrote:
> nit: no curlies

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:29:
SetAppListOrdinal(*ext_it, ordinal);
On 2013/06/18 07:41:36, koz wrote:
> It's a bit weird that GetAppListOrdinal() reads from prefs but
> SetAppListOrdinal() writes to a map. I suggest renaming GetAppListOrdinal() to
> GetAppListOrdinalFromPrefs().

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:41: if
(web_store_app_ordinal.IsValid()) {
On 2013/06/18 07:41:36, koz wrote:
> no curlies

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:47: if
(web_store_app_ordinal.IsValid()) {
On 2013/06/18 07:41:36, koz wrote:
> no curlies

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:53: for
(AppListOrdinalMap::iterator ordinal_map_it = app_list_ordinal_map_
On 2013/06/18 07:41:36, koz wrote:
> nit: break this line after the =

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:57: ExtensionIdSet&
extension_set = ordinal_map_it->second;
On 2013/06/18 07:41:36, koz wrote:
> I think this function would read better if it made use of a helper function,
> something like:
> 
> AssignUniqueOrdinalsBetweenBounds(extension_set, lower_bound, upper_bound);
> 
> WDYT?

I'd rather keep all the logic in one place so anybody that reads this can do so
without having to scroll to the top of the page. Added some comments so that it
breaks down the logic blocks a bit more.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:61: ++next_it;
On 2013/06/18 07:41:36, koz wrote:
> next_it = ordinal_map_it + 1 ?

Nocompile =(

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:93:
syncer::StringOrdinal ordinal = syncer::StringOrdinal::CreateInitialOrdinal();
On 2013/06/18 07:41:36, koz wrote:
> Consider using the ternary operator here.

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:94: if
(!app_list_ordinal_map_.empty()) {
On 2013/06/18 07:41:36, koz wrote:
> nit: no curlies

N/A

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:102:
syncer::StringOrdinal ordinal = syncer::StringOrdinal::CreateInitialOrdinal();
On 2013/06/18 07:41:36, koz wrote:
> Consider using the ternary operator here.

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:104: ordinal =
app_list_ordinal_map_.rbegin()->first.CreateAfter();
On 2013/06/18 07:41:36, koz wrote:
> nit: no curlies

N/A

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:112:
InsertAtBack(extension_id);
On 2013/06/18 07:41:36, koz wrote:
> Do callers actually distinguish between InsertAtNextAvailable() and
> InsertAtBack()? I'd remove this function and replace it with calls to
> InsertAtBack().

Ok. This was mostly so extension_sorting with its first not-full page placement
rules could fit underneath the extension_ordering interface but that's gone now.
Do you think this should be renamed app_list_extension_sorting for consistency
then?

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:127: // We only need
to change the StringOrdinal if there are neighbours.
On 2013/06/18 07:41:36, koz wrote:
> nit: if there is at least one neighbour?

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:162: if
(ordinal_map_it == app_list_ordinal_map_.end())
On 2013/06/18 07:41:36, koz wrote:
> DCHECK_NE() ?

DCHECK_NE tries to print the values which can't be done here.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:197:
DCHECK(!Contains(extension_id));
On 2013/06/18 07:41:36, koz wrote:
> Can we just call Erase(extension_id) instead?

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_extension_ordering.h (right):

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.h:20: // single ordinal.
On 2013/06/18 07:41:36, koz wrote:
> s/This ordering is.../This class defines the order by assigning each app a
> single string ordinal/.
> 
> Perhaps expand this comment by saying that this class works on an in-memory
map
> of ordinals which it uses to resolve conflicts generated by sync and which it
> commits to prefs for persistence.

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.h:31: // consistent.
On 2013/06/18 07:41:36, koz wrote:
> Won't they potentially be inconsistent between calls to
> SetAppListOrdinalForSync() and FixSyncCollisions()?

I don't think so. SetAppListOrdinalForSync will set the ordinal in the prefs as
well as the internal data structure.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.h:40: void
SetAppListOrdinalForSync(const std::string& extension_id,
On 2013/06/18 07:41:36, koz wrote:
> How about UpdateAppListOrdinalFromSync()?

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.h:43: // Resolves any
conflicts that might be created as a result of syncing that
On 2013/06/18 07:41:36, koz wrote:
> s/result of syncing/result of calls to SetAppListOrdinalForSync()/

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.h:70: bool
ExtensionPrecedes(const std::string& extension1,
On 2013/06/18 07:41:36, koz wrote:
> nit: extension_id1 and extension_id2

Done.

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.h:72: // Returns true if
|extension_id| is in the ordering.
On 2013/06/18 07:41:36, koz wrote:
> nit: newline before this line

Done.

[koz (OOO until 15th September), 2013-06-19 06:11:45]

lgtm

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_extension_ordering.cc (right):

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:112:
InsertAtBack(extension_id);
On 2013/06/19 05:09:17, calamity wrote:
> On 2013/06/18 07:41:36, koz wrote:
> > Do callers actually distinguish between InsertAtNextAvailable() and
> > InsertAtBack()? I'd remove this function and replace it with calls to
> > InsertAtBack().
> 
> Ok. This was mostly so extension_sorting with its first not-full page
placement
> rules could fit underneath the extension_ordering interface but that's gone
now.
> Do you think this should be renamed app_list_extension_sorting for consistency
> then?

Cool, that's a good idea.

[tapted, 2013-06-19 10:47:58]

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_extension_ordering.cc (right):

https://chromiumcodereview.appspot.com/17038002/diff/21029/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:61: ++next_it;
On 2013/06/19 05:09:17, calamity wrote:
> On 2013/06/18 07:41:36, koz wrote:
> > next_it = ordinal_map_it + 1 ?
> 
> Nocompile =(

driveby trivia: C++11 has std::next(const iterator,
number_of_times_to_increment_it) -> iterator_copy

C++03 only has std::advance which modifies its argument, so not what you want
here.

[xiyuan, 2013-06-19 17:43:12]

Thank you for looking into this. It's the right thing to use separate ordering
data from NTP.

Could we support two level of ordinals in the new app list ordering? That is,
one for page and one for the app in the page. Right now, app list presents the
apps as a continuous linear list but we want to support arbitrary pages in a
similar way as NTP does.

It might be easier to base the work on ExtensionSorting, e.g. move common code
into a ExtensionSortingBase and have ExtensionSortingNTP and
ExtensionSortingAppList.

Thoughts?

https://chromiumcodereview.appspot.com/17038002/diff/40001/chrome/browser/ui/...
File chrome/browser/ui/app_list/app_list_extension_ordering.cc (right):

https://chromiumcodereview.appspot.com/17038002/diff/40001/chrome/browser/ui/...
chrome/browser/ui/app_list/app_list_extension_ordering.cc:46:
SetAppListOrdinal(extension_misc::kChromeAppId, chrome_app_ordinal);
This would break the ChromeOS default app ordering. See the |default_ordinals_|
of ExtensionSorting, we probably need to move it here and simplify
ExtensionSorting.

[koz (OOO until 15th September), 2013-06-20 01:44:14]

On 2013/06/19 17:43:12, xiyuan wrote:
> Thank you for looking into this. It's the right thing to use separate ordering
> data from NTP.
> 
> Could we support two level of ordinals in the new app list ordering? That is,
> one for page and one for the app in the page. Right now, app list presents the
> apps as a continuous linear list but we want to support arbitrary pages in a
> similar way as NTP does.
> 
> It might be easier to base the work on ExtensionSorting, e.g. move common code
> into a ExtensionSortingBase and have ExtensionSortingNTP and
> ExtensionSortingAppList.
> 
> Thoughts?
> 
>
https://chromiumcodereview.appspot.com/17038002/diff/40001/chrome/browser/ui/...
> File chrome/browser/ui/app_list/app_list_extension_ordering.cc (right):
> 
>
https://chromiumcodereview.appspot.com/17038002/diff/40001/chrome/browser/ui/...
> chrome/browser/ui/app_list/app_list_extension_ordering.cc:46:
> SetAppListOrdinal(extension_misc::kChromeAppId, chrome_app_ordinal);
> This would break the ChromeOS default app ordering. See the
|default_ordinals_|
> of ExtensionSorting, we probably need to move it here and simplify
> ExtensionSorting.

Are there concrete plans for making the app list have pages like the NTP? I was
thinking it would be better for the app list to have a simpler representation if
pages aren't needed.

[xiyuan, 2013-06-20 03:31:31]

On 2013/06/20 01:44:14, koz wrote:
> On 2013/06/19 17:43:12, xiyuan wrote:
> > Thank you for looking into this. It's the right thing to use separate
ordering
> > data from NTP.
> > 
> > Could we support two level of ordinals in the new app list ordering? That
is,
> > one for page and one for the app in the page. Right now, app list presents
the
> > apps as a continuous linear list but we want to support arbitrary pages in a
> > similar way as NTP does.
> > 
> > It might be easier to base the work on ExtensionSorting, e.g. move common
code
> > into a ExtensionSortingBase and have ExtensionSortingNTP and
> > ExtensionSortingAppList.
> > 
> > Thoughts?
> > 
> >
>
https://chromiumcodereview.appspot.com/17038002/diff/40001/chrome/browser/ui/...
> > File chrome/browser/ui/app_list/app_list_extension_ordering.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/17038002/diff/40001/chrome/browser/ui/...
> > chrome/browser/ui/app_list/app_list_extension_ordering.cc:46:
> > SetAppListOrdinal(extension_misc::kChromeAppId, chrome_app_ordinal);
> > This would break the ChromeOS default app ordering. See the
> |default_ordinals_|
> > of ExtensionSorting, we probably need to move it here and simplify
> > ExtensionSorting.
> 
> Are there concrete plans for making the app list have pages like the NTP? I
was
> thinking it would be better for the app list to have a simpler representation
if
> pages aren't needed.

That was part of the original app list design but I did not find a good way to
implement it.
kuscher@ filed a tracking bug http://crbug.com/161781. We could discuss it
there.

Meanwhile, my only concern for the CL is then the default app order for ChromeOS
is missed.

[calamity, 2013-07-05 04:47:34]

Added default app ordinal support and renamed to app_list_extension_ordering.

[xiyuan, 2013-07-05 06:06:17]

We probably should get a sync engineer to review the app sync data part change
as well.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:33: void
MoveChromeAppToEnd(AppListExtensionSorting* ordering) {
nit: insert an empty line before and shift left two spaces

namespace {

void MoveChromeAppToEnd(...) {
...
}

}  // namespace

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:15: namespace {
nit:

namespace {

const char kPrefAppListOrdinal[] = "app_list_ordinal";

}  // namesapce

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:48: const std::string
extension_id,
const std::string&

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:49:
std::vector<std::string>& uninitialized_extensions) {
I don't think we should pass in this. Instead we should make this function
return true/false and update it on the caller side based on the return value.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:60: #if
defined(OS_CHROMEOS)
nit: #if always starts at col 0

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:74: :
app_list_ordinal_map_.begin()->first.CreateBefore();
I don't think we should use existing ordinals as a base. The default ordinals
should be a fixed set and not changing from run to run.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:94:
SetAppListOrdinal(it->first, it->second);
What if user has changed the default app's position? Wouldn't this reset the
user position?

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:220: if
(!Contains(extension_id1))
Would the unknown ID case ever going to happen? If not, we should CHECK or
DCHECK the unknown id case.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:40: protected:
nit: insert en empty line before

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:46:
MockExtensionService extension_service_;
DISALLOW_COPY_AND_ASSIGN(...);

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:50: public:
nit: fix wrong indent here and the rest of the file.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/apps_model_builder_unittest.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/apps_model_builder_unittest.cc:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
We no longer need to update license headers.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/extension_app_item.cc:58: AppListExtensionSorting*
GetExtensionSorting(Profile* profile) {
nit: Update function name. Either make it "GetSorting" or
"GetAppListExtensionSorting" to avoid confusion with ExtensionSorting.

[koz (OOO until 15th September), 2013-07-08 01:02:23]

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.h:34: // reflect the same
data.
Could you elaborate more on what |extension_ids| is supposed to contain? From
the code it seems that it will always include the default apps on CROS, but
never the WebStore and Chrome apps on other platforms. (Is that right?)

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.h:87:
std::vector<std::string>& uninitialized_extensions);
nit: newline after this line

[calamity, 2013-07-11 03:31:00]

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/sync/test/...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/sync/test/...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:33: void
MoveChromeAppToEnd(AppListExtensionSorting* ordering) {
On 2013/07/05 06:06:18, xiyuan wrote:
> nit: insert an empty line before and shift left two spaces
> 
> namespace {
> 
> void MoveChromeAppToEnd(...) {
> ...
> }
> 
> }  // namespace

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:15: namespace {
On 2013/07/05 06:06:18, xiyuan wrote:
> nit:
> 
> namespace {
> 
> const char kPrefAppListOrdinal[] = "app_list_ordinal";
> 
> }  // namesapce

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:48: const std::string
extension_id,
On 2013/07/05 06:06:18, xiyuan wrote:
> const std::string&

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:49:
std::vector<std::string>& uninitialized_extensions) {
On 2013/07/05 06:06:18, xiyuan wrote:
> I don't think we should pass in this. Instead we should make this function
> return true/false and update it on the caller side based on the return value.

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:60: #if
defined(OS_CHROMEOS)
On 2013/07/05 06:06:18, xiyuan wrote:
> nit: #if always starts at col 0

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:74: :
app_list_ordinal_map_.begin()->first.CreateBefore();
On 2013/07/05 06:06:18, xiyuan wrote:
> I don't think we should use existing ordinals as a base. The default ordinals
> should be a fixed set and not changing from run to run.

This is handling when default ordinals are requested when other ordinals already
exist. I think the default ordinals should change to provide sane behavior for
default apps. For example, if we add a new default app in the future, using a
hard-coded ordinal will result in the app being placed in an arbitrary position
in the list. It probably makes more sense to add it at the end rather than the
beginning actually.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:94:
SetAppListOrdinal(it->first, it->second);
On 2013/07/05 06:06:18, xiyuan wrote:
> What if user has changed the default app's position? Wouldn't this reset the
> user position?

This is only called if the default app is in uninitialized_extensions which is
filled with extensions that don't have ordinals.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:220: if
(!Contains(extension_id1))
On 2013/07/05 06:06:18, xiyuan wrote:
> Would the unknown ID case ever going to happen? If not, we should CHECK or
> DCHECK the unknown id case. 

Done. This now DCHECKs but maintains reasonable failure behavior.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.h:34: // reflect the same
data.
On 2013/07/08 01:02:24, koz wrote:
> Could you elaborate more on what |extension_ids| is supposed to contain? From
> the code it seems that it will always include the default apps on CROS, but
> never the WebStore and Chrome apps on other platforms. (Is that right?)

It has all installed apps but I think component apps are loaded elsewhere.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.h:87:
std::vector<std::string>& uninitialized_extensions);
On 2013/07/08 01:02:24, koz wrote:
> nit: newline after this line

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:40: protected:
On 2013/07/05 06:06:18, xiyuan wrote:
> nit: insert en empty line before

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:46:
MockExtensionService extension_service_;
On 2013/07/05 06:06:18, xiyuan wrote:
> DISALLOW_COPY_AND_ASSIGN(...);

Doesn't compile. I get 'no appropriate default constructor available'.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:50: public:
On 2013/07/05 06:06:18, xiyuan wrote:
> nit: fix wrong indent here and the rest of the file.

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/apps_model_builder_unittest.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/apps_model_builder_unittest.cc:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2013/07/05 06:06:18, xiyuan wrote:
> We no longer need to update license headers.

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/extension_app_item.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/extension_app_item.cc:58: AppListExtensionSorting*
GetExtensionSorting(Profile* profile) {
On 2013/07/05 06:06:18, xiyuan wrote:
> nit: Update function name. Either make it "GetSorting" or
> "GetAppListExtensionSorting" to avoid confusion with ExtensionSorting.

Done.

[xiyuan, 2013-07-11 05:39:29]

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:74: :
app_list_ordinal_map_.begin()->first.CreateBefore();
On 2013/07/11 03:31:00, calamity wrote:
> On 2013/07/05 06:06:18, xiyuan wrote:
> > I don't think we should use existing ordinals as a base. The default
ordinals
> > should be a fixed set and not changing from run to run.
> 
> This is handling when default ordinals are requested when other ordinals
already
> exist. I think the default ordinals should change to provide sane behavior for
> default apps. For example, if we add a new default app in the future, using a
> hard-coded ordinal will result in the app being placed in an arbitrary
position
> in the list. It probably makes more sense to add it at the end rather than the
> beginning actually.

Fixed ordinals do not result in arbitrary position. They would cause conflicts
and would be resolved. Their position should be close to where we intend to put
them (if user has not changed it too much). 

Assuming we have old default apps A,B,C and user apps UA, UB that follows.
- New default app D that is supposed go after C
  Fixed ordinal: D an UA might create a conflict and should give either 
    A,B,C,D,UA,UB or A,B,C,UA,D,UB (depends UA and D's app id)
  New algo would give A,B,C,UA,UB,D
- New default app B1 that is supposed to go between B and C
  Fixed: A,B,B1,C,UA,UB or A,B,C,B1,UA,UB
  New: A,B,C,UA,UB,B1

I personally feel fixed ordinal is closer to my expectation. However, I agree
that fixed ordinal could appear random if user changed the order a lot. And the
new code would always put them in a fixed location, which is good. But neither
front nor end feels right.  I don't know what is the best thing to do here. So
I'll leave it to you to make the call.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:220: if
(!Contains(extension_id1))
On 2013/07/11 03:31:00, calamity wrote:
> On 2013/07/05 06:06:18, xiyuan wrote:
> > Would the unknown ID case ever going to happen? If not, we should CHECK or
> > DCHECK the unknown id case. 
> 
> Done. This now DCHECKs but maintains reasonable failure behavior.

We don't need both. If the failure is not supposed to happen, we should CHECK or
DCHECK. Otherwise, we should keep the "if" and not CHECK.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:46:
MockExtensionService extension_service_;
On 2013/07/11 03:31:00, calamity wrote:
> On 2013/07/05 06:06:18, xiyuan wrote:
> > DISALLOW_COPY_AND_ASSIGN(...);
> 
> Doesn't compile. I get 'no appropriate default constructor available'.

You need a default constructor. Try add

AppListExtensionSortingTest() {}
virtual ~AppListExtensionSortingTest() {}

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:34: void
MoveChromeAppToEnd(AppListExtensionSorting* ordering) {
Get rid of the indent. It should start at col 0.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form...

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:17: const char
kPrefAppListOrdinal[] = "app_list_ordinal";
fix indent here too.

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:30:
std::vector<std::string> uninitialized_extensions;
nit: I wonder if we should use a std::set for this.

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:63:
chromeos::default_app_order::Get(&app_ids);
nit: two-space indent

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:70: kDefaultAppOrder,
kDefaultAppOrder + arraysize(kDefaultAppOrder));
nit: two-space indent

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:87: :
ordinal.CreateBetween(app_list_ordinal_map_.begin()->first);
This should be updated since new default apps are put after existing ones now.

[koz (OOO until 15th September), 2013-07-12 00:46:28]

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.h:34: // reflect the same
data.
On 2013/07/11 03:31:00, calamity wrote:
> On 2013/07/08 01:02:24, koz wrote:
> > Could you elaborate more on what |extension_ids| is supposed to contain?
From
> > the code it seems that it will always include the default apps on CROS, but
> > never the WebStore and Chrome apps on other platforms. (Is that right?)
> 
> It has all installed apps but I think component apps are loaded elsewhere.

I meant to elaborate in the comment :-) The correctness of this code depends on
exactly what |extension_ids| are expected here.

[calamity, 2013-07-15 07:02:32]

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:74: :
app_list_ordinal_map_.begin()->first.CreateBefore();
On 2013/07/11 05:39:29, xiyuan (OOO until 7-18) wrote:
> On 2013/07/11 03:31:00, calamity wrote:
> > On 2013/07/05 06:06:18, xiyuan wrote:
> > > I don't think we should use existing ordinals as a base. The default
> ordinals
> > > should be a fixed set and not changing from run to run.
> > 
> > This is handling when default ordinals are requested when other ordinals
> already
> > exist. I think the default ordinals should change to provide sane behavior
for
> > default apps. For example, if we add a new default app in the future, using
a
> > hard-coded ordinal will result in the app being placed in an arbitrary
> position
> > in the list. It probably makes more sense to add it at the end rather than
the
> > beginning actually.
> 
> Fixed ordinals do not result in arbitrary position. They would cause conflicts
> and would be resolved. Their position should be close to where we intend to
put
> them (if user has not changed it too much). 
> 
> Assuming we have old default apps A,B,C and user apps UA, UB that follows.
> - New default app D that is supposed go after C
>   Fixed ordinal: D an UA might create a conflict and should give either 
>     A,B,C,D,UA,UB or A,B,C,UA,D,UB (depends UA and D's app id)
>   New algo would give A,B,C,UA,UB,D
> - New default app B1 that is supposed to go between B and C
>   Fixed: A,B,B1,C,UA,UB or A,B,C,B1,UA,UB
>   New: A,B,C,UA,UB,B1
> 
> I personally feel fixed ordinal is closer to my expectation. However, I agree
> that fixed ordinal could appear random if user changed the order a lot. And
the
> new code would always put them in a fixed location, which is good. But neither
> front nor end feels right.  I don't know what is the best thing to do here. So
> I'll leave it to you to make the call. 

After thinking about it, I think front feels more correct. We definitely want
the webstore and chrome apps to appear at the front of the order.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:220: if
(!Contains(extension_id1))
On 2013/07/11 05:39:29, xiyuan (OOO until 7-18) wrote:
> On 2013/07/11 03:31:00, calamity wrote:
> > On 2013/07/05 06:06:18, xiyuan wrote:
> > > Would the unknown ID case ever going to happen? If not, we should CHECK or
> > > DCHECK the unknown id case. 
> > 
> > Done. This now DCHECKs but maintains reasonable failure behavior.
> 
> We don't need both. If the failure is not supposed to happen, we should CHECK
or
> DCHECK. Otherwise, we should keep the "if" and not CHECK.

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting.h:34: // reflect the same
data.
On 2013/07/12 00:46:29, koz wrote:
> On 2013/07/11 03:31:00, calamity wrote:
> > On 2013/07/08 01:02:24, koz wrote:
> > > Could you elaborate more on what |extension_ids| is supposed to contain?
> From
> > > the code it seems that it will always include the default apps on CROS,
but
> > > never the WebStore and Chrome apps on other platforms. (Is that right?)
> > 
> > It has all installed apps but I think component apps are loaded elsewhere.
> 
> I meant to elaborate in the comment :-) The correctness of this code depends
on
> exactly what |extension_ids| are expected here.

Done.

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
File chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc (right):

https://codereview.chromium.org/17038002/diff/81001/chrome/browser/ui/app_lis...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:46:
MockExtensionService extension_service_;
On 2013/07/11 05:39:29, xiyuan (OOO until 7-18) wrote:
> On 2013/07/11 03:31:00, calamity wrote:
> > On 2013/07/05 06:06:18, xiyuan wrote:
> > > DISALLOW_COPY_AND_ASSIGN(...);
> > 
> > Doesn't compile. I get 'no appropriate default constructor available'.
> 
> You need a default constructor. Try add
> 
> AppListExtensionSortingTest() {}
> virtual ~AppListExtensionSortingTest() {}

Done.

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:34: void
MoveChromeAppToEnd(AppListExtensionSorting* ordering) {
On 2013/07/11 05:39:30, xiyuan (OOO until 7-18) wrote:
> Get rid of the indent. It should start at col 0.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespace_Form...

Done.

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:17: const char
kPrefAppListOrdinal[] = "app_list_ordinal";
On 2013/07/11 05:39:30, xiyuan (OOO until 7-18) wrote:
> fix indent here too.

Done.

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:30:
std::vector<std::string> uninitialized_extensions;
On 2013/07/11 05:39:30, xiyuan (OOO until 7-18) wrote:
> nit: I wonder if we should use a std::set for this.

Done. Do you know of any reason extension_prefs doesn't initialize with an
ExtensionIdSet?

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:63:
chromeos::default_app_order::Get(&app_ids);
On 2013/07/11 05:39:30, xiyuan (OOO until 7-18) wrote:
> nit: two-space indent

Done.

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:70: kDefaultAppOrder,
kDefaultAppOrder + arraysize(kDefaultAppOrder));
On 2013/07/11 05:39:30, xiyuan (OOO until 7-18) wrote:
> nit: two-space indent

Done.

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:87: :
ordinal.CreateBetween(app_list_ordinal_map_.begin()->first);
On 2013/07/11 05:39:30, xiyuan (OOO until 7-18) wrote:
> This should be updated since new default apps are put after existing ones now.

Done. Switched back to adding to front.

[xiyuan, 2013-07-18 22:22:18]

LGTM

Thank you for making the changes.

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/106002/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:30:
std::vector<std::string> uninitialized_extensions;
On 2013/07/15 07:02:33, calamity wrote:
> On 2013/07/11 05:39:30, xiyuan (OOO until 7-18) wrote:
> > nit: I wonder if we should use a std::set for this.
> 
> Done. Do you know of any reason extension_prefs doesn't initialize with an
> ExtensionIdSet?

You mean ExtensionSorting, right? I am not aware of why ExtensionIdSet is not
used. If ordering is not important, we probably should use a set.

[calamity, 2013-07-19 01:56:01]

+zea for sync/ and chrome/browser/sync/ OWNERS

[calamity, 2013-07-30 06:19:55]

Friendly ping for zea@.

[calamity, 2013-08-08 01:29:34]

-zea
+atwilson for sync OWNERS

[Andrew T Wilson (Slow), 2013-08-14 07:30:22]

On 2013/08/08 01:29:34, calamity wrote:
> -zea
> +atwilson for sync OWNERS

LGTM. I would probably still TBR zea since he's knows this code better, but the
sync changes look pretty benign.

[calamity, 2013-08-19 06:11:29]

+benwells for chrome/browser/chrome_notification_types.h

Added migration from the NTP ordering so that the app list ordering will
maintain its order from before this patch.

[benwells, 2013-08-19 06:28:49]

lgtm, but I'm not an owner for that file ...

[calamity, 2013-08-19 07:16:49]

+sky for chrome/browser/chrome_notification_types.h OWNERS.

[sky, 2013-08-19 15:58:54]

https://codereview.chromium.org/17038002/diff/136002/chrome/browser/chrome_no...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/17038002/diff/136002/chrome/browser/chrome_no...
chrome/browser/chrome_notification_types.h:1020:
NOTIFICATION_APP_LIST_REORDERED,
Aren't we in the process of nuking NotificationService?
https://code.google.com/p/chromium/issues/detail?id=268984

[calamity, 2013-08-20 05:05:17]

https://codereview.chromium.org/17038002/diff/136002/chrome/browser/chrome_no...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/17038002/diff/136002/chrome/browser/chrome_no...
chrome/browser/chrome_notification_types.h:1020:
NOTIFICATION_APP_LIST_REORDERED,
On 2013/08/19 15:58:55, sky wrote:
> Aren't we in the process of nuking NotificationService?
> https://code.google.com/p/chromium/issues/detail?id=268984

Added AppListExtensionSortingObserver.

[koz (OOO until 15th September), 2013-08-20 07:16:56]

https://codereview.chromium.org/17038002/diff/157001/chrome/browser/extension...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/17038002/diff/157001/chrome/browser/extension...
chrome/browser/extensions/extension_prefs.cc:1610: extension_sorting_.get());
Better to have this take the list of extension ids that is pulled from
extension_sorting_ rather than extension_sorting_ itself. That way you have an
interface that is simpler to test and you don't introduce a dependency on
ExtensionSorting to AppListSorting.

https://codereview.chromium.org/17038002/diff/157001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/157001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:75:
ntp_sorted_extensions.begin(); ntp_it != ntp_sorted_extensions.end();
I think it's more common to put the middle expression of the for on a new line
in cases like these.

https://codereview.chromium.org/17038002/diff/157001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://codereview.chromium.org/17038002/diff/157001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.h:85: 
nit: no newline

https://codereview.chromium.org/17038002/diff/157001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc (right):

https://codereview.chromium.org/17038002/diff/157001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:178:
syncer::StringOrdinal::CreateInitialOrdinal());
CreateInitialOrdinal() always returns the same StringOrdinal, right? Can you put
that in a variable and reuse it to make the intent clear, eg:

syncer::StringOrdinal page1 = CreateInitialOrdinal();
extension_sorting()->SetPageOrdinal(kPackagedApp2Id, page1);
extension_sorting()->SetPageOrdinal(kPackagedApp1Id, page1);

syncer::StringOrdinal first_app = CreateInitialOrdinal();
extension_sorting()->SetAppOrdinal(kPackagedApp2Id, first_app);
extension_sorting()->SetAppOrdinal(kPackagedApp2Id, first_app.CreateAfter());

[calamity, 2013-08-22 00:59:24]

There was an issue caused by:
* installing an app
* cancelling the install, leaving the app_list_ordinal pref set in prefs
* installing it again
* dragging the app

This was because the app had an ordinal in the prefs that didn't match up with
an ordinal in the data structures. I fixed this by making the Contains()
function check the in-memory data structure as well. We could also just have
Erase  () ignore the case of not being able to find the extension in the
in-memory structure.

I also changed the tests to leverage the testing framework a little more.

[koz (OOO until 15th September), 2013-08-22 01:31:13]

https://chromiumcodereview.appspot.com/17038002/diff/201001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://chromiumcodereview.appspot.com/17038002/diff/201001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_extension_sorting.h:38: // contain the Web
Store and Chrome component apps. If not NULL, the order
This comment needs updating wrt |ntp_sorted_extensions|

https://chromiumcodereview.appspot.com/17038002/diff/201001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc (right):

https://chromiumcodereview.appspot.com/17038002/diff/201001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:193:
first_page);
nit: this can go on the previous line

https://chromiumcodereview.appspot.com/17038002/diff/201001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:196:
first_page);
nit: same here

https://chromiumcodereview.appspot.com/17038002/diff/201001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:202:
first_app);
nit: same here

https://chromiumcodereview.appspot.com/17038002/diff/201001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:205:
first_app.CreateAfter());
nit: same here

[calamity, 2013-08-22 01:39:21]

https://codereview.chromium.org/17038002/diff/201001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://codereview.chromium.org/17038002/diff/201001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.h:38: // contain the Web
Store and Chrome component apps. If not NULL, the order
On 2013/08/22 01:31:14, koz wrote:
> This comment needs updating wrt |ntp_sorted_extensions|

Done.

https://codereview.chromium.org/17038002/diff/201001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc (right):

https://codereview.chromium.org/17038002/diff/201001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:193:
first_page);
On 2013/08/22 01:31:14, koz wrote:
> nit: this can go on the previous line

Done.

https://codereview.chromium.org/17038002/diff/201001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:196:
first_page);
On 2013/08/22 01:31:14, koz wrote:
> nit: same here

Done.

https://codereview.chromium.org/17038002/diff/201001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:202:
first_app);
On 2013/08/22 01:31:14, koz wrote:
> nit: same here

Done.

https://codereview.chromium.org/17038002/diff/201001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting_unittest.cc:205:
first_app.CreateAfter());
On 2013/08/22 01:31:14, koz wrote:
> nit: same here

Done.

[koz (OOO until 15th September), 2013-08-22 03:14:42]

lgtm

[commit-bot: I haz the power, 2013-08-27 02:50:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/17038002/214001

[commit-bot: I haz the power, 2013-08-27 03:32:04]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[calamity, 2013-09-02 03:43:19]

Fixed extension_service_unittests by ensuring the prefs aren't touched during
extension service creation.

[koz (OOO until 15th September), 2013-09-02 06:07:54]

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:109: for (size_t i = 0;
i < app_ids.size(); ++i) {
app_ids -> default_app_order ?

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:262: if (it !=
default_initialized_extensions_.end()) {
Could you add a comment here explaining why you're doing what you're doing?

By my reading it seems that default_initialized_extensions_ contains ordinals
for extensions that don't have an ordinal in their prefs, but that do have a
hard coded natural order. We write them to the prefs on read because X.

Depending on what X is we might want to reconsider our approach.

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.h:116: DefaultOrdinalMap
default_initialized_extensions_;
I think these guys (default_initialized_extensions_ and app_list_ordinal_map_)
deserve comments.

[calamity, 2013-09-03 00:33:38]

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:109: for (size_t i = 0;
i < app_ids.size(); ++i) {
On 2013/09/02 06:07:55, koz wrote:
> app_ids -> default_app_order ?

Done.

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:262: if (it !=
default_initialized_extensions_.end()) {
On 2013/09/02 06:07:55, koz wrote:
> Could you add a comment here explaining why you're doing what you're doing?
> 
> By my reading it seems that default_initialized_extensions_ contains ordinals
> for extensions that don't have an ordinal in their prefs, but that do have a
> hard coded natural order. We write them to the prefs on read because X.
> 
> Depending on what X is we might want to reconsider our approach.

Done.

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://codereview.chromium.org/17038002/diff/251001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.h:116: DefaultOrdinalMap
default_initialized_extensions_;
On 2013/09/02 06:07:55, koz wrote:
> I think these guys (default_initialized_extensions_ and app_list_ordinal_map_)
> deserve comments.

Done.

[koz (OOO until 15th September), 2013-09-03 00:55:18]

https://codereview.chromium.org/17038002/diff/261001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://codereview.chromium.org/17038002/diff/261001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:264: // by
initialization).
This makes me uneasy. It sounds like the tests are making too many assumptions
here and should be fixed, rather than us structuring our code differently to
avoid invalidating those assumptions.

Looking at ExtensionSystem it does a bunch of preference mangling at startup
time (MigratePermissions() and MigrateDisableReasons() so it's definitely valid
to write prefs at that time.

Can you investigate those tests and find out if it's possible to fix them? If
they are just opportunistic asserts sprinkled throughout the tests it may be
possible to simply delete them - they are after all not valid assumptions.

[calamity, 2013-09-03 09:17:16]

https://chromiumcodereview.appspot.com/17038002/diff/261001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_extension_sorting.cc (right):

https://chromiumcodereview.appspot.com/17038002/diff/261001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_extension_sorting.cc:264: // by
initialization).
On 2013/09/03 00:55:19, koz wrote:
> This makes me uneasy. It sounds like the tests are making too many assumptions
> here and should be fixed, rather than us structuring our code differently to
> avoid invalidating those assumptions.
> 
> Looking at ExtensionSystem it does a bunch of preference mangling at startup
> time (MigratePermissions() and MigrateDisableReasons() so it's definitely
valid
> to write prefs at that time.
> 
> Can you investigate those tests and find out if it's possible to fix them? If
> they are just opportunistic asserts sprinkled throughout the tests it may be
> possible to simply delete them - they are after all not valid assumptions.

Done.

[koz (OOO until 15th September), 2013-09-04 00:26:04]

Cool, thanks for doing that. lgtm

https://chromiumcodereview.appspot.com/17038002/diff/230001/chrome/browser/ex...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/17038002/diff/230001/chrome/browser/ex...
chrome/browser/extensions/extension_service_unittest.cc:4328: int pref_count =
GetPrefKeyCount();
nit: here and elsewhere this would be better named as expected_pref_count.

https://chromiumcodereview.appspot.com/17038002/diff/230001/chrome/browser/ui...
File chrome/browser/ui/app_list/app_list_extension_sorting.h (right):

https://chromiumcodereview.appspot.com/17038002/diff/230001/chrome/browser/ui...
chrome/browser/ui/app_list/app_list_extension_sorting.h:115:
extensions::ExtensionIdList default_initialized_extensions_;
A comment for this member is still useful :-)

[commit-bot: I haz the power, 2013-09-06 07:03:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/17038002/243001

[commit-bot: I haz the power, 2013-09-06 07:54:15]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[stevenjb, 2013-10-08 18:10:07]

Please hold off on committing this, we need to sync up on the design.

On the one hand this is great since we want to separate app list ordering from
the NTP, on the other hand there are two issues I am concerned about:

1. We need to support positioning for browser shortcuts and folders.
2. We want to be able to support absolute positioning so that the app list
ordering is identical across devices, regardless of pre-installed apps or
"system" apps: Chrome, App Store, Files, OEM folder.

I'll try to set up a meeting at a convenient time for everyone.

https://codereview.chromium.org/17038002/diff/303001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/application_launch.cc (right):

https://codereview.chromium.org/17038002/diff/303001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:123: LOG(ERROR) <<
"OpenApplicationWindow";
VLOG

https://codereview.chromium.org/17038002/diff/303001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/application_launch.cc:209: LOG(ERROR) <<
"LaunchType:" << launch_type;
VLOG