[Sync] ModelTypeStore factory shouldn't require valid PSS to function correctly

[Sync] ModelTypeStore factory shouldn't require valid PSS to function correctly

The issue is that PSS::GetModelTypeStoreFactory requires valid PSS which doesn't
have to be available when sync is disabled (for example in guest mode on
ChromeOS). The reason it is currently wired through PSS is that PSS conveniently
has path and blocking task runner required for ModelTypeStore.

I added static GetModelTypeStoreFactory function that takes profile path and
SequencedWorkerPool. It ensures that, for a given path, all task runners use the
same sequence token and thus properly sequneced. Datatypes that wish to be
hosted in shared leveldb database should use this factory function.

I also fixed issue that backend_map_ is accessed from multiple threads and thus
might get corrupt. I wrapped it into a class with lock.

BUG=688533
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2732333003
Cr-Commit-Position: refs/heads/master@{#455749}
Committed: https://chromium.googlesource.com/chromium/src/+/222de9fb1677cdabfbf8998c17dd0e394f71a81e

[pavely, 2017-03-07 23:45:19]

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

[commit-bot: I haz the power, 2017-03-07 23:45:47]

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-07 23:59:58]

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

[commit-bot: I haz the power, 2017-03-07 23:59:59]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[pavely, 2017-03-08 00:03:19]

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

[commit-bot: I haz the power, 2017-03-08 00:03:57]

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-08 00:37:40]

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

[commit-bot: I haz the power, 2017-03-08 00:37:41]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pavely, 2017-03-08 01:32:24]

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

[commit-bot: I haz the power, 2017-03-08 01:34:00]

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-08 02:06:45]

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

[commit-bot: I haz the power, 2017-03-08 02:06:45]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pavely, 2017-03-08 18:07:42]

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

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

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-08 18:15:24]

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

[commit-bot: I haz the power, 2017-03-08 18:15:25]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pavely, 2017-03-08 18:32:10]

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

[commit-bot: I haz the power, 2017-03-08 18:33:51]

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-08 18:37:33]

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

[commit-bot: I haz the power, 2017-03-08 18:37:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[pavely, 2017-03-08 18:47:55]

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

[commit-bot: I haz the power, 2017-03-08 18:48:49]

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

[skym, 2017-03-08 19:28:15]

lgtm

https://codereview.chromium.org/2732333003/diff/60001/components/browser_sync...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2732333003/diff/60001/components/browser_sync...
components/browser_sync/profile_sync_service.cc:1681:
blocking_pool->GetNamedSequenceToken(path);
Cool!

https://codereview.chromium.org/2732333003/diff/60001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (right):

https://codereview.chromium.org/2732333003/diff/60001/components/browser_sync...
components/browser_sync/profile_sync_service.h:554: // the sync LevelDB backend.
|base_path| should be set to profile path.
And what should |sequenced_worker_pool| be?

https://codereview.chromium.org/2732333003/diff/60001/components/sync/model_i...
File components/sync/model_impl/model_type_store_backend.cc (right):

https://codereview.chromium.org/2732333003/diff/60001/components/sync/model_i...
components/sync/model_impl/model_type_store_backend.cc:86: if (it ==
backends_.end())
a ternary would probably still be a single line, just saying :)

[commit-bot: I haz the power, 2017-03-08 19:59:28]

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

[commit-bot: I haz the power, 2017-03-08 19:59:29]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-03-08 20:26:02]

Description was changed from

==========
[Sync] ModelTypeStore factory shouldn't require valid PSS to function correctly

The issue is that PSS::GetModelTypeStoreFactory requires valid PSS which doesn't
have to be available when sync is disabled (for example in guest mode on
ChromeOS). The reason it is currently wired through PSS is that PSS conveniently
has path and blocking task runner required for ModelTypeStore.

I added static GetModelTypeStoreFactory function that takes profile path and
SequencedWorkerPool. It ensures that, for a given path, all task runners use the
same sequence token and thus properly sequneced. Datatypes that wish to be
hosted in shared leveldb database should use this factory function.

I also fixed issue that backend_map_ is accessed from multiple threads and thus
might get corrupt. I wrapped it into a class with lock.

BUG=688533
R=skym@chromium.org
==========

to

==========
[Sync] ModelTypeStore factory shouldn't require valid PSS to function correctly

The issue is that PSS::GetModelTypeStoreFactory requires valid PSS which doesn't
have to be available when sync is disabled (for example in guest mode on
ChromeOS). The reason it is currently wired through PSS is that PSS conveniently
has path and blocking task runner required for ModelTypeStore.

I added static GetModelTypeStoreFactory function that takes profile path and
SequencedWorkerPool. It ensures that, for a given path, all task runners use the
same sequence token and thus properly sequneced. Datatypes that wish to be
hosted in shared leveldb database should use this factory function.

I also fixed issue that backend_map_ is accessed from multiple threads and thus
might get corrupt. I wrapped it into a class with lock.

BUG=688533
R=skym@chromium.org
==========

[pavely, 2017-03-08 20:26:02]

pavely@chromium.org changed reviewers:
+ olivierrobin@chromium.org, skau@chromium.org

[pavely, 2017-03-08 20:27:56]

Adding reviewers for files outside of sync. Guys, PTAL:

Olivier: ios/chrome/browser/reading_list/reading_list_model_factory.cc

Sean: chrome/browser/chromeos/printing/printers_manager_factory.cc

[pavely, 2017-03-08 20:40:01]

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

[commit-bot: I haz the power, 2017-03-08 20:40:46]

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

[pavely, 2017-03-08 20:41:12]

https://codereview.chromium.org/2732333003/diff/60001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (right):

https://codereview.chromium.org/2732333003/diff/60001/components/browser_sync...
components/browser_sync/profile_sync_service.h:554: // the sync LevelDB backend.
|base_path| should be set to profile path.
On 2017/03/08 19:28:15, skym wrote:
> And what should |sequenced_worker_pool| be?

Done.

https://codereview.chromium.org/2732333003/diff/60001/components/sync/model_i...
File components/sync/model_impl/model_type_store_backend.cc (right):

https://codereview.chromium.org/2732333003/diff/60001/components/sync/model_i...
components/sync/model_impl/model_type_store_backend.cc:86: if (it ==
backends_.end())
On 2017/03/08 19:28:15, skym wrote:
> a ternary would probably still be a single line, just saying :)

Done.

[skau, 2017-03-08 21:10:24]

lgtm chrome/browser/chromeos/printing/*

Thanks for the fix!

[commit-bot: I haz the power, 2017-03-08 21:45:50]

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

[commit-bot: I haz the power, 2017-03-08 21:45:51]

Dry run: This issue passed the CQ dry run.

[Olivier, 2017-03-09 08:23:36]

ios LGTM

[pavely, 2017-03-09 15:18:51]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-03-09 15:18:52]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2732333003/#ps80001
(title: "Address comments")

[commit-bot: I haz the power, 2017-03-09 15:19:06]

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-09 15:24:03]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489072731908310,
"parent_rev": "dce9f6e285ea530bc2350ae082c1ba29af929553", "commit_rev":
"222de9fb1677cdabfbf8998c17dd0e394f71a81e"}

[commit-bot: I haz the power, 2017-03-09 15:24:45]

Description was changed from

==========
[Sync] ModelTypeStore factory shouldn't require valid PSS to function correctly

The issue is that PSS::GetModelTypeStoreFactory requires valid PSS which doesn't
have to be available when sync is disabled (for example in guest mode on
ChromeOS). The reason it is currently wired through PSS is that PSS conveniently
has path and blocking task runner required for ModelTypeStore.

I added static GetModelTypeStoreFactory function that takes profile path and
SequencedWorkerPool. It ensures that, for a given path, all task runners use the
same sequence token and thus properly sequneced. Datatypes that wish to be
hosted in shared leveldb database should use this factory function.

I also fixed issue that backend_map_ is accessed from multiple threads and thus
might get corrupt. I wrapped it into a class with lock.

BUG=688533
R=skym@chromium.org
==========

to

==========
[Sync] ModelTypeStore factory shouldn't require valid PSS to function correctly

The issue is that PSS::GetModelTypeStoreFactory requires valid PSS which doesn't
have to be available when sync is disabled (for example in guest mode on
ChromeOS). The reason it is currently wired through PSS is that PSS conveniently
has path and blocking task runner required for ModelTypeStore.

I added static GetModelTypeStoreFactory function that takes profile path and
SequencedWorkerPool. It ensures that, for a given path, all task runners use the
same sequence token and thus properly sequneced. Datatypes that wish to be
hosted in shared leveldb database should use this factory function.

I also fixed issue that backend_map_ is accessed from multiple threads and thus
might get corrupt. I wrapped it into a class with lock.

BUG=688533
R=skym@chromium.org

Review-Url: https://codereview.chromium.org/2732333003
Cr-Commit-Position: refs/heads/master@{#455749}
Committed:
https://chromium.googlesource.com/chromium/src/+/222de9fb1677cdabfbf8998c17dd...
==========

[commit-bot: I haz the power, 2017-03-09 15:24:47]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/222de9fb1677cdabfbf8998c17dd...