Prevent appearing INVALID position ordinal in sync.

Prevent appearing INVALID position ordinal in sync.

This CL fixes the cases when sync item for app appears
without valid position ordinal in sync which is crash
condition for Chrome prior to M53. This also has code
that fix sync data.

BUG=677647
TEST=Create/open several accounts, no crash reported
     in debug build, including newly added DCHECK.
TEST=Problem accounts stop crashing in M52 after
     signing-in in new build.

Committed: https://crrev.com/4364c7ba2ff5c2a3af54212eb7c756cc3a06afa7
Cr-Commit-Position: refs/heads/master@{#441524}

[khmel, 2017-01-04 20:22:52]

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

[khmel, 2017-01-04 20:22:53]

Hi Steven,

PTAL

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

https://codereview.chromium.org/2614803002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/chrome_app_list_item.cc:65: extensions::AppSorting*
app_sorting = GetAppSorting();
This does not guaranty generating position for all items.
https://cs.chromium.org/chromium/src/ui/app_list/app_list_item_list.cc?q=Ensu...
fixes such items however this is too late and before settings position there app
comes to sync service, when its position is still INVALID.

[stevenjb, 2017-01-04 20:34:26]

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

https://codereview.chromium.org/2614803002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:854: // builds prior to
M53.
nit: reference crbug.

https://codereview.chromium.org/2614803002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:868: VLOG(1) << "Failed
to fix sync item from existing app " << sync_item;
Maybe use:
sync_item->item_ordinal = syncer::StringOrdinal::CreateInitialOrdinal()
and modify logging?

https://codereview.chromium.org/2614803002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:872: VLOG(1) << "Fixing
sync item by generate new position ordinal "
nit: s/generate/generating/
also: ":" separating sync_item in all of these.

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

https://codereview.chromium.org/2614803002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/chrome_app_list_item.cc:65: extensions::AppSorting*
app_sorting = GetAppSorting();
On 2017/01/04 20:22:52, khmel wrote:
> This does not guaranty generating position for all items.
>
https://cs.chromium.org/chromium/src/ui/app_list/app_list_item_list.cc?q=Ensu...
> fixes such items however this is too late and before settings position there
app
> comes to sync service, when its position is still INVALID.

Aha. Good catch.

[khmel, 2017-01-04 21:15:13]

Thank you for quick review! PTAL updated version

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

https://codereview.chromium.org/2614803002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:854: // builds prior to
M53.
On 2017/01/04 20:34:26, stevenjb wrote:
> nit: reference crbug.

Done.

https://codereview.chromium.org/2614803002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:868: VLOG(1) << "Failed
to fix sync item from existing app " << sync_item;
On 2017/01/04 20:34:26, stevenjb wrote:
> Maybe use:
> sync_item->item_ordinal = syncer::StringOrdinal::CreateInitialOrdinal()
> and modify logging?

Good point!

https://codereview.chromium.org/2614803002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/app_list_syncable_service.cc:872: VLOG(1) << "Fixing
sync item by generate new position ordinal "
On 2017/01/04 20:34:26, stevenjb wrote:
> nit: s/generate/generating/
> also: ":" separating sync_item in all of these.

Done.

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

https://codereview.chromium.org/2614803002/diff/1/chrome/browser/ui/app_list/...
chrome/browser/ui/app_list/chrome_app_list_item.cc:65: extensions::AppSorting*
app_sorting = GetAppSorting();
On 2017/01/04 20:34:26, stevenjb wrote:
> On 2017/01/04 20:22:52, khmel wrote:
> > This does not guaranty generating position for all items.
> >
>
https://cs.chromium.org/chromium/src/ui/app_list/app_list_item_list.cc?q=Ensu...
> > fixes such items however this is too late and before settings position there
> app
> > comes to sync service, when its position is still INVALID.
> 
> Aha. Good catch.

:( Kind of late. Also removed double set_position call.

[stevenjb, 2017-01-04 21:42:54]

lgtm

https://codereview.chromium.org/2614803002/diff/40001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/2614803002/diff/40001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_syncable_service.cc:868: << "Generate new
position ordinal: " << sync_item;
nit: Generating

[khmel, 2017-01-04 21:52:47]

Patchset #4 (id:60001) has been deleted

[khmel, 2017-01-04 21:56:19]

Patchset #3 (id:40001) has been deleted

[khmel, 2017-01-04 21:58:35]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-01-04 21:58:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org
Link to the patchset: https://codereview.chromium.org/2614803002/#ps80001
(title: "comment update")

[commit-bot: I haz the power, 2017-01-04 21:59:10]

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-01-04 22:34:50]

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

[commit-bot: I haz the power, 2017-01-04 22:34:51]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[khmel, 2017-01-04 23:28:12]

Hi Steven,

Can you please confirm changes in unit tests?

PTAL
Thanks

https://codereview.chromium.org/2614803002/diff/100001/chrome/browser/ui/app_...
File chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc (right):

https://codereview.chromium.org/2614803002/diff/100001/chrome/browser/ui/app_...
chrome/browser/ui/app_list/extension_app_model_builder_unittest.cc:75: const
char kDefaultApps[] = "Packaged App 1,Packaged App 2,Hosted App";
chrome/test/data/extensions/app_list/Preferences contains 't' for page_ordinal
for "Hosted App" and 'n' for Packaged App *. So new order looks correct.
Alternative fix to change 't' in file above to something < 'n'

[stevenjb, 2017-01-04 23:37:56]

lgtm

[khmel, 2017-01-04 23:38:18]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-01-04 23:38:44]

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-01-05 00:12:07]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483573098993890,
"parent_rev": "b8637bb112de97bdcac42d536c6a291e110f16ba", "commit_rev":
"2ae3dbe34aec14b4b41d466a73715349a8a446af"}

[commit-bot: I haz the power, 2017-01-05 00:12:35]

Description was changed from

==========
Prevent appearing INVALID position ordinal in sync.

This CL fixes the cases when sync item for app appears
without valid position ordinal in sync which is crash
condition for Chrome prior to M53. This also has code
that fix sync data.

BUG=677647
TEST=Create/open several accounts, no crash reported
     in debug build, including newly added DCHECK.
TEST=Problem accounts stop crashing in M52 after
     signing-in in new build.
==========

to

==========
Prevent appearing INVALID position ordinal in sync.

This CL fixes the cases when sync item for app appears
without valid position ordinal in sync which is crash
condition for Chrome prior to M53. This also has code
that fix sync data.

BUG=677647
TEST=Create/open several accounts, no crash reported
     in debug build, including newly added DCHECK.
TEST=Problem accounts stop crashing in M52 after
     signing-in in new build.

Review-Url: https://codereview.chromium.org/2614803002
==========

[commit-bot: I haz the power, 2017-01-05 00:12:36]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2017-01-05 00:15:28]

Description was changed from

==========
Prevent appearing INVALID position ordinal in sync.

This CL fixes the cases when sync item for app appears
without valid position ordinal in sync which is crash
condition for Chrome prior to M53. This also has code
that fix sync data.

BUG=677647
TEST=Create/open several accounts, no crash reported
     in debug build, including newly added DCHECK.
TEST=Problem accounts stop crashing in M52 after
     signing-in in new build.

Review-Url: https://codereview.chromium.org/2614803002
==========

to

==========
Prevent appearing INVALID position ordinal in sync.

This CL fixes the cases when sync item for app appears
without valid position ordinal in sync which is crash
condition for Chrome prior to M53. This also has code
that fix sync data.

BUG=677647
TEST=Create/open several accounts, no crash reported
     in debug build, including newly added DCHECK.
TEST=Problem accounts stop crashing in M52 after
     signing-in in new build.

Committed: https://crrev.com/4364c7ba2ff5c2a3af54212eb7c756cc3a06afa7
Cr-Commit-Position: refs/heads/master@{#441524}
==========

[commit-bot: I haz the power, 2017-01-05 00:15:31]

Patchset 4 (id:??) landed as
https://crrev.com/4364c7ba2ff5c2a3af54212eb7c756cc3a06afa7
Cr-Commit-Position: refs/heads/master@{#441524}