New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 731903 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
not on Chrome anymore
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Migrate Sync to Task Scheduler API

Project Member Reported by stanisc@chromium.org, Jun 9 2017

Issue description

The goal of this is to remove dependence of Sync code on BrowserThreads and replace with per-model instances of SequencedTaskRunner.

Perhaps we could do the migration in two steps.

1) Introduce one sequenced task runner singleton per current Sync group and have both native models and sync code use that. That will be used as a direct replacement for browser threads used in ChromeSyncClient::CreateModelWorkerForGroup. This should allow minimal changes in the sync code.

2) Refactor Sync to get rid of the group concept and move to 1:1 mapping of model specific sequenced task runner to model type in Sync code. History model already works this way so that is an example to follow.

Assigning this to myself for now
 
This has landed a week ago:

Task Scheduler API migration: change BrowserThreadModelWorker to SequencedModelWorker

BrowserThreads are being deprecated in favor of TaskScheduler API.
This change refactors BrowserThreadModelWorker preparing it to
run on a sequence rather than a browser thread.

Most of this change is a trivial renaming:
BrowserThreadModelWorker -> SequencedModelWorker
IsOnModelThread -> IsOnModelSequence

BUG= 689520 ,  731903 
Review-Url: https://codereview.chromium.org/2942773002
Cr-Commit-Position: refs/heads/master@{#480569}
Committed: https://chromium.googlesource.com/chromium/src/+/5c6a294f5b440b85ba9e9156d1f040e869a53b58
Commit 5c6a294f... initially landed in 61.0.3136.0
No merges found.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8caf9b2d9cc526400419885f23c373430a34a8f0

commit 8caf9b2d9cc526400419885f23c373430a34a8f0
Author: stanisc <stanisc@chromium.org>
Date: Mon Jun 26 23:24:08 2017

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}

[modify] https://crrev.com/8caf9b2d9cc526400419885f23c373430a34a8f0/chrome/browser/extensions/BUILD.gn
[add] https://crrev.com/8caf9b2d9cc526400419885f23c373430a34a8f0/chrome/browser/extensions/api/storage/backend_task_runner.cc
[add] https://crrev.com/8caf9b2d9cc526400419885f23c373430a34a8f0/chrome/browser/extensions/api/storage/backend_task_runner.h
[modify] https://crrev.com/8caf9b2d9cc526400419885f23c373430a34a8f0/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/8caf9b2d9cc526400419885f23c373430a34a8f0/chrome/browser/sync/glue/extension_setting_data_type_controller.cc
[modify] https://crrev.com/8caf9b2d9cc526400419885f23c373430a34a8f0/chrome/browser/sync/test/integration/extension_settings_helper.cc
[modify] https://crrev.com/8caf9b2d9cc526400419885f23c373430a34a8f0/ios/chrome/browser/sync/ios_chrome_sync_client.mm

Status: Started (was: Untriaged)
Owner: pkasting@chromium.org
pkasting@, assigning this to you since you took chrome_sync_client.cc and a few other files related to migration of Autofill. 

The approach that I used for migration of the sync FILE group is to make a singleton SequencedTaskRunner on Extensions side and use it from both Extensions and Sync code. I think you could use the same approach for migration of Autofill / Sync DB group.
Owner: stanisc@chromium.org
Sorry, I can't migrate all of sync.  I'll be touching a few files in a few places, but I cannot own the larger bug here.
Status: Fixed (was: Started)
Looks like this is now done.

Sign in to add a comment