From 67c7e0a792abf19a09f7aa7be5b319a08ff76dc6 Mon Sep 17 00:00:00 2001 From: "tim@chromium.org" Date: Sat, 4 May 2013 13:56:41 +0000 Subject: [PATCH] sync: SyncableService support for starting sync. Still has no actual effect as operational bits are behind the --sync-enable-deferred-startup flag. If you pass that flag, autofill will now be permitted to kick off sync init (in fact, it will be the only thing that can kick off sync init, so you probably don't want to pass the flag yet!). Next I'll add a fallback timer and plumb the flare to more datatypes; non Sync API types will have to call ProfileSyncService directly. TBR=dhollowa@chromium.org ^ For web_data_service_factory as changes are just basic consequence of the parameter change to AutocompleteSyncableService already LGTM'd. BUG=80194 Review URL: https://chromiumcodereview.appspot.com/14018026 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@198309 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/sync/glue/sync_start_util.cc | 51 ++++++++++++++++ chrome/browser/sync/glue/sync_start_util.h | 33 ++++++++++ chrome/browser/sync/profile_sync_service.cc | 61 ++++++++++++++++--- chrome/browser/sync/profile_sync_service.h | 18 ++++-- .../webdata/autocomplete_syncable_service.cc | 19 ++++-- .../webdata/autocomplete_syncable_service.h | 7 +++ .../webdata/web_data_service_factory.cc | 5 ++ chrome/chrome_browser.gypi | 2 + sync/api/syncable_service.h | 10 +++ 9 files changed, 187 insertions(+), 19 deletions(-) create mode 100644 chrome/browser/sync/glue/sync_start_util.cc create mode 100644 chrome/browser/sync/glue/sync_start_util.h diff --git a/chrome/browser/sync/glue/sync_start_util.cc b/chrome/browser/sync/glue/sync_start_util.cc new file mode 100644 index 000000000000..e3fd31fc80de --- /dev/null +++ b/chrome/browser/sync/glue/sync_start_util.cc @@ -0,0 +1,51 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/sync/glue/sync_start_util.h" + +#include "base/bind.h" +#include "base/files/file_path.h" +#include "chrome/browser/browser_process.h" +#include "chrome/browser/profiles/profile.h" +#include "chrome/browser/profiles/profile_manager.h" +#include "chrome/browser/sync/profile_sync_service.h" +#include "chrome/browser/sync/profile_sync_service_factory.h" +#include "content/public/browser/browser_thread.h" + +namespace { + +void StartSyncOnUIThread(const base::FilePath& profile, + syncer::ModelType type) { + ProfileManager* profile_manager = g_browser_process->profile_manager(); + Profile* p = profile_manager->GetProfileByPath(profile); + if (!p) { + DVLOG(2) << "Profile not found, can't start sync."; + return; + } + + ProfileSyncService* service = ProfileSyncServiceFactory::GetForProfile(p); + if (!service) { + DVLOG(2) << "No ProfileSyncService for profile, can't start sync."; + return; + } + service->OnDataTypeRequestsSyncStartup(type); +} + +void StartSyncProxy(const base::FilePath& profile, + syncer::ModelType type) { + content::BrowserThread::PostTask( + content::BrowserThread::UI, FROM_HERE, + base::Bind(&StartSyncOnUIThread, profile, type)); +} + +} // namespace + +namespace sync_start_util { + +syncer::SyncableService::StartSyncFlare GetFlareForSyncableService( + const base::FilePath& profile_path) { + return base::Bind(&StartSyncProxy, profile_path); +} + +} // namespace sync_start_util diff --git a/chrome/browser/sync/glue/sync_start_util.h b/chrome/browser/sync/glue/sync_start_util.h new file mode 100644 index 000000000000..473091979593 --- /dev/null +++ b/chrome/browser/sync/glue/sync_start_util.h @@ -0,0 +1,33 @@ +// Copyright 2013 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. +// +// Various utilities for kicking off sync initialization from data types or +// other services. + +#ifndef CHROME_BROWSER_SYNC_GLUE_SYNC_START_UTIL_H_ +#define CHROME_BROWSER_SYNC_GLUE_SYNC_START_UTIL_H_ + +#include "sync/api/syncable_service.h" + +namespace base { +class FilePath; +} + +namespace sync_start_util { + +// Creates a StartSyncFlare that a SyncableService can use to tell +// ProfileSyncService it needs sync to start ASAP. Typically this would be +// given to the SyncableService on construction. +// +// The flare built by this function is designed to be Run()able from any thread +// so that non-UI types don't have to deal with posting tasks. +// +// |profile_path| is used to get a hold of the actual Profile* once the +// request to start sync is safely in UI Thread land. +syncer::SyncableService::StartSyncFlare GetFlareForSyncableService( + const base::FilePath& profile_path); + +} // namespace sync_start_util + +#endif // CHROME_BROWSER_SYNC_GLUE_SYNC_START_UTIL_H_ diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index da473e4540cc..6bde4cca7f81 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -140,6 +140,7 @@ ProfileSyncService::ProfileSyncService(ProfileSyncComponentsFactory* factory, sync_prefs_(profile_ ? profile_->GetPrefs() : NULL), invalidator_storage_(profile_ ? profile_->GetPrefs(): NULL), sync_service_url_(kDevServerUrl), + data_type_requested_sync_startup_(false), is_first_time_sync_configure_(false), backend_initialized_(false), is_auth_in_progress_(false), @@ -305,7 +306,12 @@ void ProfileSyncService::TryStart() { // for performance reasons and maximizing parallelism at chrome startup, we // defer the heavy lifting for sync init until things have calmed down. if (HasSyncSetupCompleted()) { - StartUp(STARTUP_BACKEND_DEFERRED); + if (!data_type_requested_sync_startup_) + StartUp(STARTUP_BACKEND_DEFERRED); + else if (start_up_time_.is_null()) + StartUp(STARTUP_IMMEDIATE); + else + StartUpSlowBackendComponents(); } else if (setup_in_progress_ || auto_start_enabled_) { // We haven't completed sync setup. Start immediately if the user explicitly // kicked this off or we're supposed to automatically start syncing. @@ -527,23 +533,58 @@ void ProfileSyncService::StartUp(StartUpDeferredOption deferred_option) { return; } - StartUpSlowBackendComponents(STARTUP_IMMEDIATE); + StartUpSlowBackendComponents(); } -void ProfileSyncService::StartUpSlowBackendComponents( - StartUpDeferredOption deferred_option) { +void ProfileSyncService::OnDataTypeRequestsSyncStartup( + syncer::ModelType type) { + DCHECK(syncer::UserTypes().Has(type)); + if (backend_.get()) { + DVLOG(1) << "A data type requested sync startup, but it looks like " + "something else beat it to the punch."; + return; + } + + if (!GetPreferredDataTypes().Has(type)) { + // We can get here as datatype SyncableServices are typically wired up + // to the native datatype even if sync isn't enabled. + DVLOG(1) << "Dropping sync startup request because type " + << syncer::ModelTypeToString(type) << "not enabled."; + return; + } + + if (CommandLine::ForCurrentProcess()->HasSwitch( + switches::kSyncEnableDeferredStartup)) { + DVLOG(2) << "Data type requesting sync startup: " + << syncer::ModelTypeToString(type); + // Measure the time spent waiting for init and the type that triggered it. + // We could measure the time spent deferred on a per-datatype basis, but + // for now this is probably sufficient. + if (!start_up_time_.is_null()) { + // TODO(tim): Cache |type| and move this tracking to StartUp. I'd like + // to pull all the complicated init logic and state out of + // ProfileSyncService and have only a StartUp method, though. One step + // at a time. Bug 80149. + base::TimeDelta time_deferred = base::Time::Now() - start_up_time_; + UMA_HISTOGRAM_TIMES("Sync.Startup.TimeDeferred", time_deferred); + UMA_HISTOGRAM_ENUMERATION("Sync.Startup.TypeTriggeringInit", + ModelTypeToHistogramInt(type), + syncer::MODEL_TYPE_COUNT); + } + data_type_requested_sync_startup_ = true; + TryStart(); + } + DVLOG(2) << "Ignoring data type request for sync startup: " + << syncer::ModelTypeToString(type); +} + +void ProfileSyncService::StartUpSlowBackendComponents() { // Don't start up multiple times. if (backend_) { DVLOG(1) << "Skipping bringing up backend host."; return; } - DCHECK(!start_up_time_.is_null()); - if (deferred_option == STARTUP_BACKEND_DEFERRED) { - base::TimeDelta time_deferred = base::Time::Now() - start_up_time_; - UMA_HISTOGRAM_TIMES("Sync.Startup.TimeDeferred", time_deferred); - } - DCHECK(IsSyncEnabledAndLoggedIn()); CreateBackend(); diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index 61b570058ee6..5d3a4531f7aa 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -625,6 +625,12 @@ class ProfileSyncService : public ProfileSyncServiceBase, const invalidation::ObjectId& id, const std::string& payload); + // Called when a datatype (SyncableService) has a need for sync to start + // ASAP, presumably because a local change event has occurred but we're + // still in deferred start mode, meaning the SyncableService hasn't been + // told to MergeDataAndStartSyncing yet. + void OnDataTypeRequestsSyncStartup(syncer::ModelType type); + protected: // Used by test classes that derive from ProfileSyncService. virtual browser_sync::SyncBackendHost* GetBackendForTest(); @@ -742,10 +748,8 @@ class ProfileSyncService : public ProfileSyncServiceBase, }; void StartUp(StartUpDeferredOption deferred_option); - // Starts up the backend sync components. |deferred_option| specifies whether - // this is being called as part of an immediate startup or startup was - // originally deferred and we're finally getting around to finishing. - void StartUpSlowBackendComponents(StartUpDeferredOption deferred_option); + // Starts up the backend sync components. + void StartUpSlowBackendComponents(); // About-flags experiment names for datatypes that aren't enabled by default // yet. @@ -825,6 +829,12 @@ class ProfileSyncService : public ProfileSyncServiceBase, // called. base::Time start_up_time_; + // Whether we have received a signal from a SyncableService requesting that + // sync starts as soon as possible. + // TODO(tim): Move this and other TryStart related logic + state to separate + // class. Bug 80149. + bool data_type_requested_sync_startup_; + // The time that OnConfigureStart is called. This member is zero if // OnConfigureStart has not yet been called, and is reset to zero once // OnConfigureDone is called. diff --git a/chrome/browser/webdata/autocomplete_syncable_service.cc b/chrome/browser/webdata/autocomplete_syncable_service.cc index f61d76a79a09..ed05a2b85198 100644 --- a/chrome/browser/webdata/autocomplete_syncable_service.cc +++ b/chrome/browser/webdata/autocomplete_syncable_service.cc @@ -127,6 +127,11 @@ AutocompleteSyncableService::AutocompleteSyncableService() DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); } +void AutocompleteSyncableService::InjectStartSyncFlare( + const syncer::SyncableService::StartSyncFlare& flare) { + flare_ = flare; +} + syncer::SyncMergeResult AutocompleteSyncableService::MergeDataAndStartSyncing( syncer::ModelType type, const syncer::SyncDataList& initial_sync_data, @@ -304,12 +309,16 @@ syncer::SyncError AutocompleteSyncableService::ProcessSyncChanges( void AutocompleteSyncableService::AutofillEntriesChanged( const AutofillChangeList& changes) { - // Check if sync is on. If we receive notification prior to the sync being set - // up we are going to process all when MergeData..() is called. If we receive - // notification after the sync exited, it will be sinced next time Chrome - // starts. - if (sync_processor_.get()) + // Check if sync is on. If we recieve this notification prior to sync being + // started, we'll notify sync to start as soon as it can and later process + // all entries when MergeData..() is called. If we receive this notification + // sync has exited, it will be synced next time Chrome starts. + if (sync_processor_.get()) { ActOnChanges(changes); + } else if (!flare_.is_null()) { + flare_.Run(syncer::AUTOFILL); + flare_.Reset(); + } } bool AutocompleteSyncableService::LoadAutofillData( diff --git a/chrome/browser/webdata/autocomplete_syncable_service.h b/chrome/browser/webdata/autocomplete_syncable_service.h index fd3244c74443..a5bfb5d82eb4 100644 --- a/chrome/browser/webdata/autocomplete_syncable_service.h +++ b/chrome/browser/webdata/autocomplete_syncable_service.h @@ -79,6 +79,11 @@ class AutocompleteSyncableService void UpdateCullSetting(bool cull_expired_entries); bool cull_expired_entries() const { return cull_expired_entries_; } + // Provides a StartSyncFlare to the SyncableService. See + // sync_start_util for more. + void InjectStartSyncFlare( + const syncer::SyncableService::StartSyncFlare& flare); + protected: explicit AutocompleteSyncableService( autofill::AutofillWebDataService* web_data_service); @@ -165,6 +170,8 @@ class AutocompleteSyncableService // via UpdateCullingSetting. bool cull_expired_entries_; + syncer::SyncableService::StartSyncFlare flare_; + DISALLOW_COPY_AND_ASSIGN(AutocompleteSyncableService); }; diff --git a/chrome/browser/webdata/web_data_service_factory.cc b/chrome/browser/webdata/web_data_service_factory.cc index 99c342659e9f..66bcef71cf9f 100644 --- a/chrome/browser/webdata/web_data_service_factory.cc +++ b/chrome/browser/webdata/web_data_service_factory.cc @@ -9,6 +9,7 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/profiles/incognito_helpers.h" #include "chrome/browser/profiles/profile_dependency_manager.h" +#include "chrome/browser/sync/glue/sync_start_util.h" #include "chrome/browser/ui/profile_error_dialog.h" #include "chrome/browser/webdata/autocomplete_syncable_service.h" #include "chrome/browser/webdata/autofill_profile_syncable_service.h" @@ -40,12 +41,15 @@ void ProfileErrorCallback(sql::InitStatus status) { void InitSyncableServicesOnDBThread( scoped_refptr autofill_web_data, + const syncer::SyncableService::StartSyncFlare& flare, const std::string& app_locale) { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::DB)); // Currently only Autocomplete and Autofill profiles use the new Sync API, but // all the database data should migrate to this API over time. AutocompleteSyncableService::CreateForWebDataService(autofill_web_data); + AutocompleteSyncableService::FromWebDataService( + autofill_web_data)->InjectStartSyncFlare(flare); AutofillProfileSyncableService::CreateForWebDataService( autofill_web_data, app_locale); } @@ -95,6 +99,7 @@ WebDataServiceWrapper::WebDataServiceWrapper(Profile* profile) { BrowserThread::DB, FROM_HERE, base::Bind(&InitSyncableServicesOnDBThread, autofill_web_data_, + sync_start_util::GetFlareForSyncableService(path), g_browser_process->GetApplicationLocale())); } diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 5ba2a2614fb8..41978c215814 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1998,6 +1998,8 @@ 'browser/sync/glue/sync_backend_host.h', 'browser/sync/glue/sync_backend_registrar.cc', 'browser/sync/glue/sync_backend_registrar.h', + 'browser/sync/glue/sync_start_util.cc', + 'browser/sync/glue/sync_start_util.h', 'browser/sync/glue/synced_device_tracker.cc', 'browser/sync/glue/synced_device_tracker.h', 'browser/sync/glue/synced_session.cc', diff --git a/sync/api/syncable_service.h b/sync/api/syncable_service.h index 13c282ef5a60..13225ca98463 100644 --- a/sync/api/syncable_service.h +++ b/sync/api/syncable_service.h @@ -7,6 +7,7 @@ #include +#include "base/callback.h" #include "base/compiler_specific.h" #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" @@ -28,6 +29,15 @@ class SYNC_EXPORT SyncableService : public SyncChangeProcessor, public base::SupportsWeakPtr { public: + // A StartSyncFlare is useful when your SyncableService has a need for sync + // to start ASAP, typically because a local change event has occurred but + // MergeDataAndStartSyncing hasn't been called yet, meaning you don't have a + // SyncChangeProcessor. The sync subsystem will respond soon after invoking + // Run() on your flare by calling MergeDataAndStartSyncing. The ModelType + // parameter is included so that the recieving end can track usage and timing + // statistics, make optimizations or tradeoffs by type, etc. + typedef base::Callback StartSyncFlare; + // Informs the service to begin syncing the specified synced datatype |type|. // The service should then merge |initial_sync_data| into it's local data, // calling |sync_processor|'s ProcessSyncChanges as necessary to reconcile the