arc: Handle position conflict in app list items.

arc: Handle position conflict in app list items.

It is not guaranteed that sync items have unique positions. This may
cause crash in case web store app has conflicting pos with some other
app.

TEST=Unit test added
BUG=692802
Review-Url: https://codereview.chromium.org/2732633003
Cr-Commit-Position: refs/heads/master@{#455195}
Committed: https://chromium.googlesource.com/chromium/src/+/11f9ee62d5cb9fbc3d3f1458e145b06be67f78ae

[khmel, 2017-03-04 02:57:05]

khmel@chromium.org changed reviewers:
+ stevenjb@chromium.org

[khmel, 2017-03-04 02:57:06]

Hi Steven,

PTAL

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (left):

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:1147:
static_cast<AppListFolderItem*>(model_->AddItem(std::move(new_folder)));
It is not safe to add oem_folder to model and then call GetOemFolderPos. It may
happen that newly created folder will be placed after web store app and
unintentionally counted.

[khmel, 2017-03-04 02:57:15]

The CQ bit was checked by khmel@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 02:57:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 04:14:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-04 04:14:45]

Dry run: This issue passed the CQ dry run.

[stevenjb, 2017-03-06 21:35:15]

Ugh. Thanks for tracking this down.

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:1197: continue;
This is confusing to me. If you don't like the previous method of finding the
web store index, can we write this something like:

int web_store_idx = GetIndexOf(kWebStoreAppId);
if (web_store_idx < 0)
  log error, return;
// Skip items...

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:19: constexpr
char kOemFolderId[] = "ddb1da55-d478-4243-8642-56d3041f0263";
nit: Make this a public member of AppListSyncableService (or just put it in the
app_list namespace and declare it in the header).

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:81: const
std::string some_app_id("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
Make write a simple helper to create a string > kWebStoreAppId? Use below also.
(I had too look up kWebStoreAppId to ensure this was valid).

[khmel, 2017-03-06 22:38:15]

Thanks Steven for taking a look. PTAL updated version.

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:1197: continue;
On 2017/03/06 21:35:15, stevenjb wrote:
> This is confusing to me. If you don't like the previous method of finding the
> web store index, can we write this something like:
> 
> int web_store_idx = GetIndexOf(kWebStoreAppId);
> if (web_store_idx < 0)
>   log error, return;
> // Skip items...

Thanks for idea! Hopefully |item_list| already has required helper.

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:19: constexpr
char kOemFolderId[] = "ddb1da55-d478-4243-8642-56d3041f0263";
On 2017/03/06 21:35:15, stevenjb wrote:
> nit: Make this a public member of AppListSyncableService (or just put it in
the
> app_list namespace and declare it in the header).

Done

https://codereview.chromium.org/2732633003/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:81: const
std::string some_app_id("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb");
On 2017/03/06 21:35:15, stevenjb wrote:
> Make write a simple helper to create a string > kWebStoreAppId? Use below
also.
> (I had too look up kWebStoreAppId to ensure this was valid).

Done

[stevenjb, 2017-03-06 22:52:06]

lgtm

https://codereview.chromium.org/2732633003/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2732633003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:47: }
Fancy :)

[khmel, 2017-03-06 22:54:15]

Thank you for review!

https://codereview.chromium.org/2732633003/diff/20001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc (right):

https://codereview.chromium.org/2732633003/diff/20001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc:47: }
On 2017/03/06 22:52:06, stevenjb wrote:
> Fancy :)

:)

[khmel, 2017-03-06 22:54:24]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-06 22:55:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 00:14:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 00:14:27]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[khmel, 2017-03-07 01:35:16]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-07 01:36:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 05:03:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 05:03:34]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[khmel, 2017-03-07 05:27:25]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-07 05:28:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 07:05:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 07:05:30]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[khmel, 2017-03-07 15:32:53]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-07 15:33:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 18:43:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 18:43:15]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[khmel, 2017-03-07 18:45:47]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-07 18:46:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 20:20:57]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1488912347032360,
"parent_rev": "dde665fa0a7577e619e819c13c9e4af95d8b19b1", "commit_rev":
"11f9ee62d5cb9fbc3d3f1458e145b06be67f78ae"}

[commit-bot: I haz the power, 2017-03-07 20:21:43]

Description was changed from

==========
arc: Handle position conflict in app list items.

It is not guaranteed that sync items have unique positions. This may
cause crash in case web store app has conflicting pos with some other
app.

TEST=Unit test added
BUG=692802
==========

to

==========
arc: Handle position conflict in app list items.

It is not guaranteed that sync items have unique positions. This may
cause crash in case web store app has conflicting pos with some other
app.

TEST=Unit test added
BUG=692802

Review-Url: https://codereview.chromium.org/2732633003
Cr-Commit-Position: refs/heads/master@{#455195}
Committed:
https://chromium.googlesource.com/chromium/src/+/11f9ee62d5cb9fbc3d3f1458e145...
==========

[commit-bot: I haz the power, 2017-03-07 20:21:44]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/11f9ee62d5cb9fbc3d3f1458e145...