Remove dependency on BrowserThread::FILE from Sync code

Remove dependency on BrowserThread::FILE from Sync code

This change moves definition of a task runner for a sync group to a
native model code outside of Sync, in this case - to extensions code.

This might be a bit controversial because syncer::GROUP_FILE could
theoretically include sync types other than syncer::EXTENSION_SETTINGS
and syncer::APP_SETTINGS, but in reality those are the only two
possible sync types that currently sync on the FILE thread. And
since we are migrating to the new TaskScheduler API and sequences,
if we need to introduce a new sync type in the future it should
run on a new, separate sequence and avoid sharing the FILE group.

I added a new function that is supposed to return a singleton
instance of SequencedTaskRunner -  extensions::GetBackendTaskRunner().
For now the implementation of this function still uses
BrowserThread::FILE but that should be changed to use
LazySequencedTaskRunner as soon as the rest of extensions storage
code is modified to share the same task runner.

I noticed that extensions are unavailable of iOS and Android so I removed
the syncer::GROUP_FILE option from iOS and Android specific code.

To summarize, the next steps are:
1) Change extensions code to use extensions::GetBackendTaskRunner()
   everywhere the FILE thread is currently used.
2) Modify GetBackendTaskRunner() to use LazySequencedTaskRunner instead
   of the FILE thread task runner.

BUG=689520, 731903
Review-Url: https://codereview.chromium.org/2944993002
Cr-Commit-Position: refs/heads/master@{#482455}
Committed: https://chromium.googlesource.com/chromium/src/+/8caf9b2d9cc526400419885f23c373430a34a8f0

[stanisc, 2017-06-20 01:42:07]

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

[commit-bot: I haz the power, 2017-06-20 01:42:34]

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-06-20 02:15:32]

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

[commit-bot: I haz the power, 2017-06-20 02:15:33]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[stanisc, 2017-06-20 20:08:17]

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

[commit-bot: I haz the power, 2017-06-20 20:09: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-06-20 21:15:40]

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

[commit-bot: I haz the power, 2017-06-20 21:15:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[stanisc, 2017-06-20 22:00:49]

Description was changed from

==========
Task runner for sync::GROUP_FILE

BUG=
==========

to

==========
Remove dependency on BrowserThread::FILE in Sync code

This patch moves the responsibility to define a task runner for a
sync group outside of Sync, in this case - to extensions code.

This is a bit controversial because syncer::GROUP_FILE could include
models other than syncer::EXTENSION_SETTINGS and syncer::APP_SETTINGS,
but in reality those are the only two possible sync types that currently
sync on the FILE thread. And since we are migrating to the new 
TaskScheduler API and sequences, if we need to introduce a new sync
type it would likely run on a new, separate sequences. 

All this change does for now is that it introduces the following function
that gets the singleton task runner to be used in Sync code:
 extensions::GetBackendTaskRunner()

The implementation of this function still uses BrowserThread::FILE but
that should be changed to return an instance of LazySequencedTaskRunner
as soon as the rest of extensions code is modified to share the same task
runner.

I noticed that extensions are unavailable of iOS and Android so I removed
the syncer::GROUP_FILE option from iOS and Android specific code.

To summarize, the next steps are:
1) Change extensions code to use extensions::GetBackendTaskRunner()
   everywhere the FILE thread is currently used.
2) Modify GetBackendTaskRunner() to use LazySequencedTaskRunner instead
   of the FILE thread task runner.

BUG=689520,731903
==========

[stanisc, 2017-06-20 22:13:52]

Description was changed from

==========
Remove dependency on BrowserThread::FILE in Sync code

This patch moves the responsibility to define a task runner for a
sync group outside of Sync, in this case - to extensions code.

This is a bit controversial because syncer::GROUP_FILE could include
models other than syncer::EXTENSION_SETTINGS and syncer::APP_SETTINGS,
but in reality those are the only two possible sync types that currently
sync on the FILE thread. And since we are migrating to the new 
TaskScheduler API and sequences, if we need to introduce a new sync
type it would likely run on a new, separate sequences. 

All this change does for now is that it introduces the following function
that gets the singleton task runner to be used in Sync code:
 extensions::GetBackendTaskRunner()

The implementation of this function still uses BrowserThread::FILE but
that should be changed to return an instance of LazySequencedTaskRunner
as soon as the rest of extensions code is modified to share the same task
runner.

I noticed that extensions are unavailable of iOS and Android so I removed
the syncer::GROUP_FILE option from iOS and Android specific code.

To summarize, the next steps are:
1) Change extensions code to use extensions::GetBackendTaskRunner()
   everywhere the FILE thread is currently used.
2) Modify GetBackendTaskRunner() to use LazySequencedTaskRunner instead
   of the FILE thread task runner.

BUG=689520,731903
==========

to

==========
Remove dependency on BrowserThread::FILE from Sync code

This change moves definition of a task runner for a sync group to a
native model code outside of Sync, in this case - to extensions code.

This might be a bit controversial because syncer::GROUP_FILE could
theoretically include sync types other than syncer::EXTENSION_SETTINGS
and syncer::APP_SETTINGS, but in reality those are the only two
possible sync types that currently sync on the FILE thread. And
since we are migrating to the new TaskScheduler API and sequences,
if we need to introduce a new sync type in the future it should
run on a new, separate sequence and avoid sharing the FILE group.

I added a new function that is supposed to return a singleton
instance of SequencedTaskRunner -  extensions::GetBackendTaskRunner().
For now the implementation of this function still uses
BrowserThread::FILE but that should be changed to use
LazySequencedTaskRunner as soon as the rest of extensions storage
code is modified to share the same task runner.

I noticed that extensions are unavailable of iOS and Android so I removed
the syncer::GROUP_FILE option from iOS and Android specific code.

To summarize, the next steps are:
1) Change extensions code to use extensions::GetBackendTaskRunner()
   everywhere the FILE thread is currently used.
2) Modify GetBackendTaskRunner() to use LazySequencedTaskRunner instead
   of the FILE thread task runner.

BUG=689520,731903
==========

[stanisc, 2017-06-20 22:15:25]

stanisc@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, sdefresne@chromium.org, skym@chromium.org

[stanisc, 2017-06-20 22:15:26]

PTAL!

rdevlin.cronin@chromium.org - extensions

sdefresne@chromium.org - ios

skym@chromium.org - sync

[skym, 2017-06-20 22:22:37]

lgtm

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/extension_settings_helper.cc (right):

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/extension_settings_helper.cc:42:
CHECK(extensions::GetBackendTaskRunner()->RunsTasksInCurrentSequence());
Would it be more reasonable to have this EXPECT_TRUE instead of CHECK?

[Devlin, 2017-06-21 00:25:21]

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/backend_task_runner.cc (right):

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.cc:11: //
TODO(stanisc): crbug.com/731903 : change to LazySequencedTaskRunner once
nit: no space after crbug.com/731903

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.cc:13: 
nit: remove this blank line

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/backend_task_runner.h (right):

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.h:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
no (c) in new files

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.h:13: // Gets the
singleton task runner for running Extensions storage backend tasks.
Maybe explain what a "storage backend" task includes?

[stanisc, 2017-06-21 02:00:26]

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

[commit-bot: I haz the power, 2017-06-21 02:00:44]

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-06-21 03:26:30]

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

[commit-bot: I haz the power, 2017-06-21 03:26:31]

Dry run: This issue passed the CQ dry run.

[stanisc, 2017-06-21 17:20:55]

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/backend_task_runner.cc (right):

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.cc:11: //
TODO(stanisc): crbug.com/731903 : change to LazySequencedTaskRunner once
On 2017/06/21 00:25:20, Devlin wrote:
> nit: no space after crbug.com/731903

Done.

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.cc:13: 
On 2017/06/21 00:25:20, Devlin wrote:
> nit: remove this blank line

Done.

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/backend_task_runner.h (right):

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.h:1: // Copyright (c)
2017 The Chromium Authors. All rights reserved.
On 2017/06/21 00:25:21, Devlin wrote:
> no (c) in new files

Done.

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.h:13: // Gets the
singleton task runner for running Extensions storage backend tasks.
On 2017/06/21 00:25:20, Devlin wrote:
> Maybe explain what a "storage backend" task includes?

I don't actually know exactly what needs to be posted on this task runner.
I think at a minimum anything related to SyncStorageBackend needs to be on this
runner to ensure that the same sequence is shared with the sync.
It looks like both StorageFrontend and SyncValueStoreCache need to be switched
but likely more extensions code that is currently posting on the FILE thread
might need to be switched to this task runner as well. I'd appreciate your help
in figuring that out. I'll follow up on that in a separate patch.

[Devlin, 2017-06-21 19:19:32]

extensions lgtm with an optional suggetion.

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/backend_task_runner.h (right):

https://codereview.chromium.org/2944993002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.h:13: // Gets the
singleton task runner for running Extensions storage backend tasks.
On 2017/06/21 17:20:55, stanisc wrote:
> On 2017/06/21 00:25:20, Devlin wrote:
> > Maybe explain what a "storage backend" task includes?
> 
> I don't actually know exactly what needs to be posted on this task runner.
> I think at a minimum anything related to SyncStorageBackend needs to be on
this
> runner to ensure that the same sequence is shared with the sync.
> It looks like both StorageFrontend and SyncValueStoreCache need to be switched
> but likely more extensions code that is currently posting on the FILE thread
> might need to be switched to this task runner as well. I'd appreciate your
help
> in figuring that out. I'll follow up on that in a separate patch.
> 

Yeah, I'm sure there will be more code to be updated, but having this CL
targeted is good.  I like your new phrasing since it's clear what this is used
for currently.

https://codereview.chromium.org/2944993002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/backend_task_runner.h (right):

https://codereview.chromium.org/2944993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.h:14:
scoped_refptr<base::SequencedTaskRunner> GetBackendTaskRunner();
Since this is currently only used for SyncStorageBackend tasks, it might make
sense to instead put this as a static method in the SyncStorageBackend class,
e.g.

class SyncStorageBackend {
 public:
  static scoped_refptr<base::SequencedTaskRunner> GetTaskRunner();
}

[stanisc, 2017-06-22 22:34:16]

https://codereview.chromium.org/2944993002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/backend_task_runner.h (right):

https://codereview.chromium.org/2944993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.h:14:
scoped_refptr<base::SequencedTaskRunner> GetBackendTaskRunner();
On 2017/06/21 19:19:31, Devlin wrote:
> Since this is currently only used for SyncStorageBackend tasks, it might make
> sense to instead put this as a static method in the SyncStorageBackend class,
> e.g.
> 
> class SyncStorageBackend {
>  public:
>   static scoped_refptr<base::SequencedTaskRunner> GetTaskRunner();
> }

SyncStorageBackend seems like a wrong place. I think this task runner would have
to trickle all the way to LeveldbValueStore because it currently does a bunch of
DCHECK_CURRENTLY_ON(BrowserThread::FILE) checks. There might be multiple
solutions:
1) Put the task runner in a class that would logically apply to the lower levels
of implementation of the storage like LeveldbValueStore but still make it
reasonable to be
accessed from the sync code.
2) Pass the task runner into LeveldbValueStore so that it can validate that it
is 
running on the right task runner sequence.
3) Remove these BrowserThread::FILE checks from LeveldbValueStore and replace
them 
with something more generic like AssertIOAllowed.

If we pick #2 or #3 than it should be OK to define the task runner at
SyncStorageBackend level. That should also allow to have different task runners
for different storages in the extensions subsystem rather than serialize
everything on the same task runner.
Would you prefer this approach or one singleton task runner used for everything
within extensions?
prefer this

[stanisc, 2017-06-23 20:56:44]

Patchset #5 (id:80001) has been deleted

[stanisc, 2017-06-23 21:00:23]

sdefresne@, please review a small change in ios_chrome_sync_client.mm

https://codereview.chromium.org/2944993002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/backend_task_runner.h (right):

https://codereview.chromium.org/2944993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.h:14:
scoped_refptr<base::SequencedTaskRunner> GetBackendTaskRunner();
On 2017/06/22 22:34:16, stanisc wrote:
> On 2017/06/21 19:19:31, Devlin wrote:
> > Since this is currently only used for SyncStorageBackend tasks, it might
make
> > sense to instead put this as a static method in the SyncStorageBackend
class,
> > e.g.
> > 
> > class SyncStorageBackend {
> >  public:
> >   static scoped_refptr<base::SequencedTaskRunner> GetTaskRunner();
> > }
> 
> SyncStorageBackend seems like a wrong place. I think this task runner would
have
> to trickle all the way to LeveldbValueStore because it currently does a bunch
of
> DCHECK_CURRENTLY_ON(BrowserThread::FILE) checks. There might be multiple
> solutions:
> 1) Put the task runner in a class that would logically apply to the lower
levels
> of implementation of the storage like LeveldbValueStore but still make it
> reasonable to be
> accessed from the sync code.
> 2) Pass the task runner into LeveldbValueStore so that it can validate that it
> is 
> running on the right task runner sequence.
> 3) Remove these BrowserThread::FILE checks from LeveldbValueStore and replace
> them 
> with something more generic like AssertIOAllowed.
> 
> If we pick #2 or #3 than it should be OK to define the task runner at
> SyncStorageBackend level. That should also allow to have different task
runners
> for different storages in the extensions subsystem rather than serialize
> everything on the same task runner.
> Would you prefer this approach or one singleton task runner used for
everything
> within extensions?
> prefer this 

I'll leave it this way and re-evaluate in the next patch when I convert
SyncStorageBackend and other related extensions classes.

[Devlin, 2017-06-23 21:23:35]

https://codereview.chromium.org/2944993002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/backend_task_runner.h (right):

https://codereview.chromium.org/2944993002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/backend_task_runner.h:14:
scoped_refptr<base::SequencedTaskRunner> GetBackendTaskRunner();
On 2017/06/23 21:00:23, stanisc wrote:
> On 2017/06/22 22:34:16, stanisc wrote:
> > On 2017/06/21 19:19:31, Devlin wrote:
> > > Since this is currently only used for SyncStorageBackend tasks, it might
> make
> > > sense to instead put this as a static method in the SyncStorageBackend
> class,
> > > e.g.
> > > 
> > > class SyncStorageBackend {
> > >  public:
> > >   static scoped_refptr<base::SequencedTaskRunner> GetTaskRunner();
> > > }
> > 
> > SyncStorageBackend seems like a wrong place. I think this task runner would
> have
> > to trickle all the way to LeveldbValueStore because it currently does a
bunch
> of
> > DCHECK_CURRENTLY_ON(BrowserThread::FILE) checks. There might be multiple
> > solutions:
> > 1) Put the task runner in a class that would logically apply to the lower
> levels
> > of implementation of the storage like LeveldbValueStore but still make it
> > reasonable to be
> > accessed from the sync code.
> > 2) Pass the task runner into LeveldbValueStore so that it can validate that
it
> > is 
> > running on the right task runner sequence.
> > 3) Remove these BrowserThread::FILE checks from LeveldbValueStore and
replace
> > them 
> > with something more generic like AssertIOAllowed.
> > 
> > If we pick #2 or #3 than it should be OK to define the task runner at
> > SyncStorageBackend level. That should also allow to have different task
> runners
> > for different storages in the extensions subsystem rather than serialize
> > everything on the same task runner.
> > Would you prefer this approach or one singleton task runner used for
> everything
> > within extensions?
> > prefer this 
> 
> I'll leave it this way and re-evaluate in the next patch when I convert
> SyncStorageBackend and other related extensions classes.

I think #3 (preferring AssertIOAllowed) is probably the most appropriate (but no
need to do that in this CL).

[sdefresne, 2017-06-25 21:22:50]

ios/ lgtm

[stanisc, 2017-06-26 21:44:26]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2017-06-26 21:44:26]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2944993002/#ps100001
(title: "More explicit about returning nullptr ModelSafeWorker for FILE group on
Android and iOS")

[commit-bot: I haz the power, 2017-06-26 21:44:51]

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-06-26 23:24:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498513466381140,
"parent_rev": "c3ffa5f9c0f6c2cb7405fdfb09d3b59801caf250", "commit_rev":
"8caf9b2d9cc526400419885f23c373430a34a8f0"}

[commit-bot: I haz the power, 2017-06-26 23:24:23]

Description was changed from

==========
Remove dependency on BrowserThread::FILE from Sync code

This change moves definition of a task runner for a sync group to a
native model code outside of Sync, in this case - to extensions code.

This might be a bit controversial because syncer::GROUP_FILE could
theoretically include sync types other than syncer::EXTENSION_SETTINGS
and syncer::APP_SETTINGS, but in reality those are the only two
possible sync types that currently sync on the FILE thread. And
since we are migrating to the new TaskScheduler API and sequences,
if we need to introduce a new sync type in the future it should
run on a new, separate sequence and avoid sharing the FILE group.

I added a new function that is supposed to return a singleton
instance of SequencedTaskRunner -  extensions::GetBackendTaskRunner().
For now the implementation of this function still uses
BrowserThread::FILE but that should be changed to use
LazySequencedTaskRunner as soon as the rest of extensions storage
code is modified to share the same task runner.

I noticed that extensions are unavailable of iOS and Android so I removed
the syncer::GROUP_FILE option from iOS and Android specific code.

To summarize, the next steps are:
1) Change extensions code to use extensions::GetBackendTaskRunner()
   everywhere the FILE thread is currently used.
2) Modify GetBackendTaskRunner() to use LazySequencedTaskRunner instead
   of the FILE thread task runner.

BUG=689520,731903
==========

to

==========
Remove dependency on BrowserThread::FILE from Sync code

This change moves definition of a task runner for a sync group to a
native model code outside of Sync, in this case - to extensions code.

This might be a bit controversial because syncer::GROUP_FILE could
theoretically include sync types other than syncer::EXTENSION_SETTINGS
and syncer::APP_SETTINGS, but in reality those are the only two
possible sync types that currently sync on the FILE thread. And
since we are migrating to the new TaskScheduler API and sequences,
if we need to introduce a new sync type in the future it should
run on a new, separate sequence and avoid sharing the FILE group.

I added a new function that is supposed to return a singleton
instance of SequencedTaskRunner -  extensions::GetBackendTaskRunner().
For now the implementation of this function still uses
BrowserThread::FILE but that should be changed to use
LazySequencedTaskRunner as soon as the rest of extensions storage
code is modified to share the same task runner.

I noticed that extensions are unavailable of iOS and Android so I removed
the syncer::GROUP_FILE option from iOS and Android specific code.

To summarize, the next steps are:
1) Change extensions code to use extensions::GetBackendTaskRunner()
   everywhere the FILE thread is currently used.
2) Modify GetBackendTaskRunner() to use LazySequencedTaskRunner instead
   of the FILE thread task runner.

BUG=689520,731903

Review-Url: https://codereview.chromium.org/2944993002
Cr-Commit-Position: refs/heads/master@{#482455}
Committed:
https://chromium.googlesource.com/chromium/src/+/8caf9b2d9cc526400419885f23c3...
==========

[commit-bot: I haz the power, 2017-06-26 23:24:25]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8caf9b2d9cc526400419885f23c3...