Reland: Keep poll intervals in prefs.
Cause for breakage: Side effect in DCHECK() which wasn't exercised on trybots
(apparently they have all DCHECKs enabled)
Without persisting them, server-side provided intervals are less efficient on
platforms with relatively short app lifetime.
This also allows lower poll intervals for latency-sensitive use cases in cases
when our notifications are not reliable enough (yet).
Bug: 832019
Change-Id: Ifcd55edcbb9fb17583caac847f6c110024a83e19
Reviewed-on: https://chromium-review.googlesource.com/1013457
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Tim Schumann <tschumann@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550984}diff --git a/chrome/browser/sync/test/integration/sync_client_command_test.cc b/chrome/browser/sync/test/integration/sync_client_command_test.cc
new file mode 100644
index 0000000..89c968c5
--- /dev/null
+++ b/chrome/browser/sync/test/integration/sync_client_command_test.cc
@@ -0,0 +1,91 @@
+// Copyright (c) 2018 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 "base/macros.h"
+#include "base/run_loop.h"
+#include "build/build_config.h"
+
+#include "chrome/browser/sync/test/integration/profile_sync_service_harness.h"
+#include "chrome/browser/sync/test/integration/session_hierarchy_match_checker.h"
+#include "chrome/browser/sync/test/integration/sessions_helper.h"
+#include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/common/webui_url_constants.h"
+#include "components/sync/base/sync_prefs.h"
+#include "components/sync/engine/polling_constants.h"
+#include "components/sync/protocol/client_commands.pb.h"
+#include "components/sync/test/fake_server/sessions_hierarchy.h"
+
+#include "components/sync/protocol/client_commands.pb.h"
+
+using sessions_helper::CheckInitialState;
+using sessions_helper::OpenTab;
+using syncer::SyncPrefs;
+
+namespace {
+
+class SyncClientCommandTest : public SyncTest {
+ public:
+ SyncClientCommandTest() : SyncTest(SINGLE_CLIENT) {}
+ ~SyncClientCommandTest() override {}
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(SyncClientCommandTest);
+};
+
+IN_PROC_BROWSER_TEST_F(SyncClientCommandTest, ShouldPersistPollIntervals) {
+ // Setup clients and verify no poll intervals are present yet.
+ ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+ std::unique_ptr<SyncPrefs> sync_prefs =
+ std::make_unique<SyncPrefs>(GetProfile(0)->GetPrefs());
+ EXPECT_TRUE(sync_prefs->GetShortPollInterval().is_zero());
+ EXPECT_TRUE(sync_prefs->GetLongPollInterval().is_zero());
+
+ // Execute a sync cycle and verify the client set up (and persisted) the
+ // default values.
+ ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+ EXPECT_EQ(sync_prefs->GetShortPollInterval().InSeconds(),
+ syncer::kDefaultShortPollIntervalSeconds);
+ EXPECT_EQ(sync_prefs->GetLongPollInterval().InSeconds(),
+ syncer::kDefaultLongPollIntervalSeconds);
+
+ // Simulate server-provided poll intervals and make sure they get persisted.
+ sync_pb::ClientCommand client_command;
+ client_command.set_set_sync_poll_interval(67);
+ client_command.set_set_sync_long_poll_interval(199);
+ GetFakeServer()->SetClientCommand(client_command);
+
+ // Trigger a sync-cycle.
+ ASSERT_TRUE(CheckInitialState(0));
+ ASSERT_TRUE(OpenTab(0, GURL(chrome::kChromeUIHistoryURL)));
+ SessionHierarchyMatchChecker checker(
+ fake_server::SessionsHierarchy(
+ {{GURL(chrome::kChromeUIHistoryURL).spec()}}),
+ GetSyncService(0), GetFakeServer());
+ EXPECT_TRUE(checker.Wait());
+
+ EXPECT_EQ(sync_prefs->GetShortPollInterval().InSeconds(), 67);
+ EXPECT_EQ(sync_prefs->GetLongPollInterval().InSeconds(), 199);
+}
+
+IN_PROC_BROWSER_TEST_F(SyncClientCommandTest, ShouldUsePollIntervalsFromPrefs) {
+ // Setup clients and provide new poll intervals via prefs.
+ ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+ std::unique_ptr<SyncPrefs> sync_prefs =
+ std::make_unique<SyncPrefs>(GetProfile(0)->GetPrefs());
+ sync_prefs->SetShortPollInterval(base::TimeDelta::FromSeconds(123));
+ sync_prefs->SetLongPollInterval(base::TimeDelta::FromSeconds(1234));
+
+ // Execute a sync cycle and verify this cycle used those intervals.
+ // This test assumes the SyncScheduler reads the actual intervals from the
+ // context. This is covered in the SyncSchedulerImpl's unittest.
+ ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+ EXPECT_EQ(
+ 123,
+ GetClient(0)->GetLastCycleSnapshot().short_poll_interval().InSeconds());
+ EXPECT_EQ(
+ 1234,
+ GetClient(0)->GetLastCycleSnapshot().long_poll_interval().InSeconds());
+}
+
+} // namespace
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index c838970..d0bfa28 100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -5272,6 +5272,7 @@
"../browser/sync/test/integration/single_client_wallet_sync_test.cc",
"../browser/sync/test/integration/single_client_wifi_credentials_sync_test.cc",
"../browser/sync/test/integration/sync_auth_test.cc",
+ "../browser/sync/test/integration/sync_client_command_test.cc",
"../browser/sync/test/integration/sync_errors_test.cc",
"../browser/sync/test/integration/sync_exponential_backoff_test.cc",
"../browser/sync/test/integration/two_client_app_list_sync_test.cc",
diff --git a/components/browser_sync/profile_sync_service.cc b/components/browser_sync/profile_sync_service.cc
index f89a149..3a11e525 100644
--- a/components/browser_sync/profile_sync_service.cc
+++ b/components/browser_sync/profile_sync_service.cc
@@ -955,6 +955,11 @@
UpdateLastSyncedTime();
if (!snapshot.poll_finish_time().is_null())
sync_prefs_.SetLastPollTime(snapshot.poll_finish_time());
+ DCHECK(!snapshot.short_poll_interval().is_zero());
+ sync_prefs_.SetShortPollInterval(snapshot.short_poll_interval());
+
+ DCHECK(!snapshot.long_poll_interval().is_zero());
+ sync_prefs_.SetLongPollInterval(snapshot.long_poll_interval());
if (IsDataTypeControllerRunning(syncer::SESSIONS) &&
snapshot.model_neutral_state().get_updates_request_types.Has(
diff --git a/components/browser_sync/profile_sync_service_unittest.cc b/components/browser_sync/profile_sync_service_unittest.cc
index 5bceb20c..dd1da3f 100644
--- a/components/browser_sync/profile_sync_service_unittest.cc
+++ b/components/browser_sync/profile_sync_service_unittest.cc
@@ -1071,7 +1071,14 @@
TEST_F(ProfileSyncServiceTest, ValidPointersInDTCMap) {
CreateService(ProfileSyncService::AUTO_START);
service()->OnSessionRestoreComplete();
- service()->OnSyncCycleCompleted(syncer::SyncCycleSnapshot());
+ service()->OnSyncCycleCompleted(syncer::SyncCycleSnapshot(
+ syncer::ModelNeutralState(), syncer::ProgressMarkerMap(), false, 0, 0, 0,
+ false, 0, base::Time::Now(), base::Time::Now(),
+ std::vector<int>(syncer::MODEL_TYPE_COUNT, 0),
+ std::vector<int>(syncer::MODEL_TYPE_COUNT, 0),
+ sync_pb::SyncEnums::UNKNOWN_ORIGIN,
+ /*short_poll_interval=*/base::TimeDelta::FromMinutes(30),
+ /*long_poll_interval=*/base::TimeDelta::FromMinutes(180)));
}
// The OpenTabsUIDelegate should only be accessable when PROXY_TABS is enabled.
diff --git a/components/suggestions/suggestions_service_impl_unittest.cc b/components/suggestions/suggestions_service_impl_unittest.cc
index e99c5d9..e6fc7cf 100644
--- a/components/suggestions/suggestions_service_impl_unittest.cc
+++ b/components/suggestions/suggestions_service_impl_unittest.cc
@@ -8,6 +8,7 @@
#include <memory>
#include <utility>
+#include <vector>
#include "base/bind.h"
#include "base/macros.h"
@@ -182,7 +183,9 @@
2, 7, false, 0, base::Time::Now(), base::Time::Now(),
std::vector<int>(syncer::MODEL_TYPE_COUNT, 0),
std::vector<int>(syncer::MODEL_TYPE_COUNT, 0),
- sync_pb::SyncEnums::UNKNOWN_ORIGIN)));
+ sync_pb::SyncEnums::UNKNOWN_ORIGIN,
+ /*short_poll_interval=*/base::TimeDelta::FromMinutes(30),
+ /*long_poll_interval=*/base::TimeDelta::FromMinutes(180))));
// These objects are owned by the SuggestionsService, but we keep the
// pointers around for testing.
test_suggestions_store_ = new TestSuggestionsStore();
diff --git a/components/sync/driver/glue/sync_backend_host_core.cc b/components/sync/driver/glue/sync_backend_host_core.cc
index ef24a5e..b8603bc 100644
--- a/components/sync/driver/glue/sync_backend_host_core.cc
+++ b/components/sync/driver/glue/sync_backend_host_core.cc
@@ -328,6 +328,8 @@
params.report_unrecoverable_error_function;
args.cancelation_signal = &stop_syncing_signal_;
args.saved_nigori_state = std::move(params.saved_nigori_state);
+ args.short_poll_interval = params.short_poll_interval;
+ args.long_poll_interval = params.long_poll_interval;
sync_manager_->Init(&args);
base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(
this, "SyncDirectory", base::ThreadTaskRunnerHandle::Get());
diff --git a/components/sync/driver/sync_service_base.cc b/components/sync/driver/sync_service_base.cc
index acd7836..4636db18 100644
--- a/components/sync/driver/sync_service_base.cc
+++ b/components/sync/driver/sync_service_base.cc
@@ -18,6 +18,7 @@
#include "components/sync/device_info/local_device_info_provider.h"
#include "components/sync/driver/sync_driver_switches.h"
#include "components/sync/engine/engine_components_factory_impl.h"
+#include "components/sync/engine/polling_constants.h"
namespace syncer {
@@ -154,6 +155,16 @@
base::Bind(ReportUnrecoverableError, channel_);
params.saved_nigori_state = crypto_->TakeSavedNigoriState();
sync_prefs_.GetInvalidationVersions(¶ms.invalidation_versions);
+ params.short_poll_interval = sync_prefs_.GetShortPollInterval();
+ if (params.short_poll_interval.is_zero()) {
+ params.short_poll_interval =
+ base::TimeDelta::FromSeconds(kDefaultShortPollIntervalSeconds);
+ }
+ params.long_poll_interval = sync_prefs_.GetLongPollInterval();
+ if (params.long_poll_interval.is_zero()) {
+ params.long_poll_interval =
+ base::TimeDelta::FromSeconds(kDefaultLongPollIntervalSeconds);
+ }
engine_->Initialize(std::move(params));
}
diff --git a/components/sync/driver/sync_service_utils_unittest.cc b/components/sync/driver/sync_service_utils_unittest.cc
index 60cfcb8..48928e3 100644
--- a/components/sync/driver/sync_service_utils_unittest.cc
+++ b/components/sync/driver/sync_service_utils_unittest.cc
@@ -49,12 +49,14 @@
}
SyncCycleSnapshot GetLastCycleSnapshot() const override {
if (sync_cycle_complete_) {
- return SyncCycleSnapshot(ModelNeutralState(), ProgressMarkerMap(), false,
- 5, 2, 7, false, 0, base::Time::Now(),
- base::Time::Now(),
- std::vector<int>(MODEL_TYPE_COUNT, 0),
- std::vector<int>(MODEL_TYPE_COUNT, 0),
- sync_pb::SyncEnums::UNKNOWN_ORIGIN);
+ return SyncCycleSnapshot(
+ ModelNeutralState(), ProgressMarkerMap(), false, 5, 2, 7, false, 0,
+ base::Time::Now(), base::Time::Now(),
+ std::vector<int>(MODEL_TYPE_COUNT, 0),
+ std::vector<int>(MODEL_TYPE_COUNT, 0),
+ sync_pb::SyncEnums::UNKNOWN_ORIGIN,
+ /*short_poll_interval=*/base::TimeDelta::FromMinutes(30),
+ /*long_poll_interval=*/base::TimeDelta::FromMinutes(180));
}
return SyncCycleSnapshot();
}
diff --git a/components/sync/engine/cycle/sync_cycle_snapshot.cc b/components/sync/engine/cycle/sync_cycle_snapshot.cc
index 20823b5..90a6fa4 100644
--- a/components/sync/engine/cycle/sync_cycle_snapshot.cc
+++ b/components/sync/engine/cycle/sync_cycle_snapshot.cc
@@ -36,7 +36,9 @@
base::Time poll_finish_time,
const std::vector<int>& num_entries_by_type,
const std::vector<int>& num_to_delete_entries_by_type,
- sync_pb::SyncEnums::GetUpdatesOrigin get_updates_origin)
+ sync_pb::SyncEnums::GetUpdatesOrigin get_updates_origin,
+ base::TimeDelta short_poll_interval,
+ base::TimeDelta long_poll_interval)
: model_neutral_state_(model_neutral_state),
download_progress_markers_(download_progress_markers),
is_silenced_(is_silenced),
@@ -50,6 +52,8 @@
num_entries_by_type_(num_entries_by_type),
num_to_delete_entries_by_type_(num_to_delete_entries_by_type),
get_updates_origin_(get_updates_origin),
+ short_poll_interval_(short_poll_interval),
+ long_poll_interval_(long_poll_interval),
is_initialized_(true) {}
SyncCycleSnapshot::SyncCycleSnapshot(const SyncCycleSnapshot& other) = default;
@@ -162,4 +166,12 @@
return get_updates_origin_;
}
+base::TimeDelta SyncCycleSnapshot::short_poll_interval() const {
+ return short_poll_interval_;
+}
+
+base::TimeDelta SyncCycleSnapshot::long_poll_interval() const {
+ return long_poll_interval_;
+}
+
} // namespace syncer
diff --git a/components/sync/engine/cycle/sync_cycle_snapshot.h b/components/sync/engine/cycle/sync_cycle_snapshot.h
index 62aee7e..ac4fb8c 100644
--- a/components/sync/engine/cycle/sync_cycle_snapshot.h
+++ b/components/sync/engine/cycle/sync_cycle_snapshot.h
@@ -42,7 +42,9 @@
base::Time poll_finish_time,
const std::vector<int>& num_entries_by_type,
const std::vector<int>& num_to_delete_entries_by_type,
- sync_pb::SyncEnums::GetUpdatesOrigin get_updates_origin);
+ sync_pb::SyncEnums::GetUpdatesOrigin get_updates_origin,
+ base::TimeDelta short_poll_interval,
+ base::TimeDelta long_poll_interval);
SyncCycleSnapshot(const SyncCycleSnapshot& other);
~SyncCycleSnapshot();
@@ -63,6 +65,8 @@
const std::vector<int>& num_entries_by_type() const;
const std::vector<int>& num_to_delete_entries_by_type() const;
sync_pb::SyncEnums::GetUpdatesOrigin get_updates_origin() const;
+ base::TimeDelta short_poll_interval() const;
+ base::TimeDelta long_poll_interval() const;
// Set iff this snapshot was not built using the default constructor.
bool is_initialized() const;
@@ -84,6 +88,9 @@
sync_pb::SyncEnums::GetUpdatesOrigin get_updates_origin_;
+ base::TimeDelta short_poll_interval_;
+ base::TimeDelta long_poll_interval_;
+
bool is_initialized_;
};
diff --git a/components/sync/engine/cycle/sync_cycle_snapshot_unittest.cc b/components/sync/engine/cycle/sync_cycle_snapshot_unittest.cc
index ca0f243..4a86538 100644
--- a/components/sync/engine/cycle/sync_cycle_snapshot_unittest.cc
+++ b/components/sync/engine/cycle/sync_cycle_snapshot_unittest.cc
@@ -41,13 +41,14 @@
const int kNumHierarchyConflicts = 1055;
const int kNumServerConflicts = 1057;
- SyncCycleSnapshot snapshot(model_neutral, download_progress_markers,
- kIsSilenced, kNumEncryptionConflicts,
- kNumHierarchyConflicts, kNumServerConflicts, false,
- 0, base::Time::Now(), base::Time::Now(),
- std::vector<int>(MODEL_TYPE_COUNT, 0),
- std::vector<int>(MODEL_TYPE_COUNT, 0),
- sync_pb::SyncEnums::UNKNOWN_ORIGIN);
+ SyncCycleSnapshot snapshot(
+ model_neutral, download_progress_markers, kIsSilenced,
+ kNumEncryptionConflicts, kNumHierarchyConflicts, kNumServerConflicts,
+ false, 0, base::Time::Now(), base::Time::Now(),
+ std::vector<int>(MODEL_TYPE_COUNT, 0),
+ std::vector<int>(MODEL_TYPE_COUNT, 0), sync_pb::SyncEnums::UNKNOWN_ORIGIN,
+ /*short_poll_interval=*/base::TimeDelta::FromMinutes(30),
+ /*long_poll_interval=*/base::TimeDelta::FromMinutes(180));
std::unique_ptr<base::DictionaryValue> value(snapshot.ToValue());
EXPECT_EQ(16u, value->size());
ExpectDictIntegerValue(model_neutral.num_successful_commits, *value,
diff --git a/components/sync/engine/engine_components_factory.h b/components/sync/engine/engine_components_factory.h
index 70c6b1f..8b7170f 100644
--- a/components/sync/engine/engine_components_factory.h
+++ b/components/sync/engine/engine_components_factory.h
@@ -10,6 +10,7 @@
#include <vector>
#include "base/files/file_path.h"
+#include "base/time/time.h"
#include "components/sync/engine/model_safe_worker.h"
namespace syncer {
@@ -100,7 +101,9 @@
const std::vector<SyncEngineEventListener*>& listeners,
DebugInfoGetter* debug_info_getter,
ModelTypeRegistry* model_type_registry,
- const std::string& invalidator_client_id) = 0;
+ const std::string& invalidator_client_id,
+ base::TimeDelta short_poll_interval,
+ base::TimeDelta long_poll_interval) = 0;
virtual std::unique_ptr<syncable::DirectoryBackingStore>
BuildDirectoryBackingStore(StorageOption storage,
diff --git a/components/sync/engine/engine_components_factory_impl.cc b/components/sync/engine/engine_components_factory_impl.cc
index efaa68a..41d73c9 100644
--- a/components/sync/engine/engine_components_factory_impl.cc
+++ b/components/sync/engine/engine_components_factory_impl.cc
@@ -67,14 +67,16 @@
const std::vector<SyncEngineEventListener*>& listeners,
DebugInfoGetter* debug_info_getter,
ModelTypeRegistry* model_type_registry,
- const std::string& invalidation_client_id) {
- return std::unique_ptr<SyncCycleContext>(
- new SyncCycleContext(connection_manager, directory, extensions_activity,
- listeners, debug_info_getter, model_type_registry,
- switches_.encryption_method == ENCRYPTION_KEYSTORE,
- switches_.pre_commit_updates_policy ==
- FORCE_ENABLE_PRE_COMMIT_UPDATE_AVOIDANCE,
- invalidation_client_id));
+ const std::string& invalidation_client_id,
+ base::TimeDelta short_poll_interval,
+ base::TimeDelta long_poll_interval) {
+ return std::make_unique<SyncCycleContext>(
+ connection_manager, directory, extensions_activity, listeners,
+ debug_info_getter, model_type_registry,
+ switches_.encryption_method == ENCRYPTION_KEYSTORE,
+ switches_.pre_commit_updates_policy ==
+ FORCE_ENABLE_PRE_COMMIT_UPDATE_AVOIDANCE,
+ invalidation_client_id, short_poll_interval, long_poll_interval);
}
std::unique_ptr<syncable::DirectoryBackingStore>
diff --git a/components/sync/engine/engine_components_factory_impl.h b/components/sync/engine/engine_components_factory_impl.h
index a16cc5e..9ddc96d 100644
--- a/components/sync/engine/engine_components_factory_impl.h
+++ b/components/sync/engine/engine_components_factory_impl.h
@@ -34,7 +34,9 @@
const std::vector<SyncEngineEventListener*>& listeners,
DebugInfoGetter* debug_info_getter,
ModelTypeRegistry* model_type_registry,
- const std::string& invalidator_client_id) override;
+ const std::string& invalidator_client_id,
+ base::TimeDelta short_poll_interval,
+ base::TimeDelta long_poll_interval) override;
std::unique_ptr<syncable::DirectoryBackingStore> BuildDirectoryBackingStore(
StorageOption storage,
diff --git a/components/sync/engine/sync_engine.h b/components/sync/engine/sync_engine.h
index e7d7484..9d9d4bf 100644
--- a/components/sync/engine/sync_engine.h
+++ b/components/sync/engine/sync_engine.h
@@ -15,6 +15,7 @@
#include "base/macros.h"
#include "base/memory/ref_counted.h"
#include "base/single_thread_task_runner.h"
+#include "base/time/time.h"
#include "components/sync/base/extensions_activity.h"
#include "components/sync/base/model_type.h"
#include "components/sync/base/weak_handle.h"
@@ -76,6 +77,10 @@
std::unique_ptr<SyncEncryptionHandler::NigoriState> saved_nigori_state;
std::map<ModelType, int64_t> invalidation_versions;
+ // Define the polling intervals. Must not be zero.
+ base::TimeDelta short_poll_interval;
+ base::TimeDelta long_poll_interval;
+
private:
DISALLOW_COPY_AND_ASSIGN(InitParams);
};
diff --git a/components/sync/engine/sync_manager.h b/components/sync/engine/sync_manager.h
index 8ece474..aee26e8 100644
--- a/components/sync/engine/sync_manager.h
+++ b/components/sync/engine/sync_manager.h
@@ -267,6 +267,10 @@
// Optional nigori state to be restored.
std::unique_ptr<SyncEncryptionHandler::NigoriState> saved_nigori_state;
+
+ // Define the polling intervals. Must not be zero.
+ base::TimeDelta short_poll_interval;
+ base::TimeDelta long_poll_interval;
};
SyncManager();
diff --git a/components/sync/engine/test_engine_components_factory.cc b/components/sync/engine/test_engine_components_factory.cc
index d71f1b53..29e7e05 100644
--- a/components/sync/engine/test_engine_components_factory.cc
+++ b/components/sync/engine/test_engine_components_factory.cc
@@ -37,7 +37,9 @@
const std::vector<SyncEngineEventListener*>& listeners,
DebugInfoGetter* debug_info_getter,
ModelTypeRegistry* model_type_registry,
- const std::string& invalidator_client_id) {
+ const std::string& invalidator_client_id,
+ base::TimeDelta short_poll_interval,
+ base::TimeDelta long_poll_interval) {
// Tests don't wire up listeners.
std::vector<SyncEngineEventListener*> empty_listeners;
return std::unique_ptr<SyncCycleContext>(new SyncCycleContext(
@@ -46,7 +48,7 @@
switches_.encryption_method == ENCRYPTION_KEYSTORE,
switches_.pre_commit_updates_policy ==
FORCE_ENABLE_PRE_COMMIT_UPDATE_AVOIDANCE,
- invalidator_client_id));
+ invalidator_client_id, short_poll_interval, long_poll_interval));
}
std::unique_ptr<syncable::DirectoryBackingStore>
diff --git a/components/sync/engine/test_engine_components_factory.h b/components/sync/engine/test_engine_components_factory.h
index 0f2ec77..6280fd5 100644
--- a/components/sync/engine/test_engine_components_factory.h
+++ b/components/sync/engine/test_engine_components_factory.h
@@ -34,7 +34,9 @@
const std::vector<SyncEngineEventListener*>& listeners,
DebugInfoGetter* debug_info_getter,
ModelTypeRegistry* model_type_registry,
- const std::string& invalidator_client_id) override;
+ const std::string& invalidator_client_id,
+ base::TimeDelta short_poll_interval,
+ base::TimeDelta long_poll_interval) override;
std::unique_ptr<syncable::DirectoryBackingStore> BuildDirectoryBackingStore(
StorageOption storage,
diff --git a/components/sync/engine_impl/cycle/sync_cycle.cc b/components/sync/engine_impl/cycle/sync_cycle.cc
index 7127f036..6f6e3c3 100644
--- a/components/sync/engine_impl/cycle/sync_cycle.cc
+++ b/components/sync/engine_impl/cycle/sync_cycle.cc
@@ -47,7 +47,8 @@
context_->notifications_enabled(), dir->GetEntriesCount(),
status_controller_->sync_start_time(),
status_controller_->poll_finish_time(), num_entries_by_type,
- num_to_delete_entries_by_type, get_updates_origin);
+ num_to_delete_entries_by_type, get_updates_origin,
+ context_->short_poll_interval(), context_->long_poll_interval());
return snapshot;
}
diff --git a/components/sync/engine_impl/cycle/sync_cycle_context.cc b/components/sync/engine_impl/cycle/sync_cycle_context.cc
index c50ea25..5dd33e4 100644
--- a/components/sync/engine_impl/cycle/sync_cycle_context.cc
+++ b/components/sync/engine_impl/cycle/sync_cycle_context.cc
@@ -17,7 +17,9 @@
ModelTypeRegistry* model_type_registry,
bool keystore_encryption_enabled,
bool client_enabled_pre_commit_update_avoidance,
- const std::string& invalidator_client_id)
+ const std::string& invalidator_client_id,
+ base::TimeDelta short_poll_interval,
+ base::TimeDelta long_poll_interval)
: connection_manager_(connection_manager),
directory_(directory),
extensions_activity_(extensions_activity),
@@ -31,7 +33,11 @@
client_enabled_pre_commit_update_avoidance_(
client_enabled_pre_commit_update_avoidance),
cookie_jar_mismatch_(false),
- cookie_jar_empty_(false) {
+ cookie_jar_empty_(false),
+ short_poll_interval_(short_poll_interval),
+ long_poll_interval_(long_poll_interval) {
+ DCHECK(!short_poll_interval.is_zero());
+ DCHECK(!long_poll_interval.is_zero());
std::vector<SyncEngineEventListener*>::const_iterator it;
for (it = listeners.begin(); it != listeners.end(); ++it)
listeners_.AddObserver(*it);
diff --git a/components/sync/engine_impl/cycle/sync_cycle_context.h b/components/sync/engine_impl/cycle/sync_cycle_context.h
index 1e7e0aef..400ba9f 100644
--- a/components/sync/engine_impl/cycle/sync_cycle_context.h
+++ b/components/sync/engine_impl/cycle/sync_cycle_context.h
@@ -11,6 +11,7 @@
#include <vector>
#include "base/macros.h"
+#include "base/time/time.h"
#include "components/sync/engine_impl/cycle/debug_info_getter.h"
#include "components/sync/engine_impl/model_type_registry.h"
#include "components/sync/engine_impl/sync_engine_event_listener.h"
@@ -47,7 +48,9 @@
ModelTypeRegistry* model_type_registry,
bool keystore_encryption_enabled,
bool client_enabled_pre_commit_update_avoidance,
- const std::string& invalidator_client_id);
+ const std::string& invalidator_client_id,
+ base::TimeDelta short_poll_interval,
+ base::TimeDelta long_poll_interval);
~SyncCycleContext();
@@ -116,6 +119,18 @@
void set_cookie_jar_empty(bool empty_jar) { cookie_jar_empty_ = empty_jar; }
+ base::TimeDelta short_poll_interval() const { return short_poll_interval_; }
+ void set_short_poll_interval(base::TimeDelta interval) {
+ DCHECK(!interval.is_zero());
+ short_poll_interval_ = interval;
+ }
+
+ base::TimeDelta long_poll_interval() const { return long_poll_interval_; }
+ void set_long_poll_interval(base::TimeDelta interval) {
+ DCHECK(!interval.is_zero());
+ long_poll_interval_ = interval;
+ }
+
private:
base::ObserverList<SyncEngineEventListener> listeners_;
@@ -173,6 +188,9 @@
// If there's a cookie jar mismatch, whether the cookie jar was empty or not.
bool cookie_jar_empty_;
+ base::TimeDelta short_poll_interval_;
+ base::TimeDelta long_poll_interval_;
+
DISALLOW_COPY_AND_ASSIGN(SyncCycleContext);
};
diff --git a/components/sync/engine_impl/js_sync_manager_observer_unittest.cc b/components/sync/engine_impl/js_sync_manager_observer_unittest.cc
index 8dc6370f..af2493e 100644
--- a/components/sync/engine_impl/js_sync_manager_observer_unittest.cc
+++ b/components/sync/engine_impl/js_sync_manager_observer_unittest.cc
@@ -61,12 +61,13 @@
}
TEST_F(JsSyncManagerObserverTest, OnSyncCycleCompleted) {
- SyncCycleSnapshot snapshot(ModelNeutralState(), ProgressMarkerMap(), false, 5,
- 2, 7, false, 0, base::Time::Now(),
- base::Time::Now(),
- std::vector<int>(MODEL_TYPE_COUNT, 0),
- std::vector<int>(MODEL_TYPE_COUNT, 0),
- sync_pb::SyncEnums::UNKNOWN_ORIGIN);
+ SyncCycleSnapshot snapshot(
+ ModelNeutralState(), ProgressMarkerMap(), false, 5, 2, 7, false, 0,
+ base::Time::Now(), base::Time::Now(),
+ std::vector<int>(MODEL_TYPE_COUNT, 0),
+ std::vector<int>(MODEL_TYPE_COUNT, 0), sync_pb::SyncEnums::UNKNOWN_ORIGIN,
+ /*short_poll_interval=*/base::TimeDelta::FromMinutes(30),
+ /*long_poll_interval=*/base::TimeDelta::FromMinutes(180));
base::DictionaryValue expected_details;
expected_details.Set("snapshot", snapshot.ToValue());
diff --git a/components/sync/engine_impl/sync_manager_impl.cc b/components/sync/engine_impl/sync_manager_impl.cc
index 2f3a289..8d890dc 100644
--- a/components/sync/engine_impl/sync_manager_impl.cc
+++ b/components/sync/engine_impl/sync_manager_impl.cc
@@ -200,6 +200,8 @@
DCHECK(!initialized_);
DCHECK(thread_checker_.CalledOnValidThread());
DCHECK(args->post_factory.get());
+ DCHECK(!args->short_poll_interval.is_zero());
+ DCHECK(!args->long_poll_interval.is_zero());
if (!args->enable_local_sync_backend) {
DCHECK(!args->credentials.account_id.empty());
DCHECK(!args->credentials.sync_token.empty());
@@ -301,7 +303,8 @@
cycle_context_ = args->engine_components_factory->BuildContext(
connection_manager_.get(), directory(), args->extensions_activity,
listeners, &debug_info_event_listener_, model_type_registry_.get(),
- args->invalidator_client_id);
+ args->invalidator_client_id, args->short_poll_interval,
+ args->long_poll_interval);
scheduler_ = args->engine_components_factory->BuildScheduler(
name_, cycle_context_.get(), args->cancelation_signal,
args->enable_local_sync_backend);
diff --git a/components/sync/engine_impl/sync_manager_impl_unittest.cc b/components/sync/engine_impl/sync_manager_impl_unittest.cc
index ef1a524..247da28 100644
--- a/components/sync/engine_impl/sync_manager_impl_unittest.cc
+++ b/components/sync/engine_impl/sync_manager_impl_unittest.cc
@@ -969,6 +969,8 @@
args.unrecoverable_error_handler =
MakeWeakHandle(mock_unrecoverable_error_handler_.GetWeakPtr());
args.cancelation_signal = &cancelation_signal_;
+ args.short_poll_interval = base::TimeDelta::FromMinutes(60);
+ args.long_poll_interval = base::TimeDelta::FromMinutes(120);
sync_manager_.Init(&args);
sync_manager_.GetEncryptionHandler()->AddObserver(&encryption_observer_);
diff --git a/components/sync/engine_impl/sync_scheduler_impl.cc b/components/sync/engine_impl/sync_scheduler_impl.cc
index def9afe7..5d15250 100644
--- a/components/sync/engine_impl/sync_scheduler_impl.cc
+++ b/components/sync/engine_impl/sync_scheduler_impl.cc
@@ -138,10 +138,8 @@
bool ignore_auth_credentials)
: name_(name),
started_(false),
- syncer_short_poll_interval_seconds_(
- TimeDelta::FromSeconds(kDefaultShortPollIntervalSeconds)),
- syncer_long_poll_interval_seconds_(
- TimeDelta::FromSeconds(kDefaultLongPollIntervalSeconds)),
+ syncer_short_poll_interval_seconds_(context->short_poll_interval()),
+ syncer_long_poll_interval_seconds_(context->long_poll_interval()),
mode_(CONFIGURATION_MODE),
delay_provider_(delay_provider),
syncer_(syncer),
diff --git a/components/sync/engine_impl/sync_scheduler_impl_unittest.cc b/components/sync/engine_impl/sync_scheduler_impl_unittest.cc
index d309241..824c0c6 100644
--- a/components/sync/engine_impl/sync_scheduler_impl_unittest.cc
+++ b/components/sync/engine_impl/sync_scheduler_impl_unittest.cc
@@ -115,7 +115,6 @@
void SetUp() override {
test_user_share_.SetUp();
- syncer_ = new testing::StrictMock<MockSyncer>();
delay_ = nullptr;
extensions_activity_ = new ExtensionsActivity();
@@ -141,9 +140,17 @@
model_type_registry_.get(),
true, // enable keystore encryption
false, // force enable pre-commit GU avoidance
- "fake_invalidator_client_id");
+ "fake_invalidator_client_id",
+ /*short_poll_interval=*/base::TimeDelta::FromMinutes(30),
+ /*long_poll_interval=*/base::TimeDelta::FromMinutes(120));
context_->set_notifications_enabled(true);
context_->set_account_name("Test");
+ RebuildScheduler();
+ }
+
+ void RebuildScheduler() {
+ // The old syncer is destroyed with the scheduler that owns it.
+ syncer_ = new testing::StrictMock<MockSyncer>();
scheduler_ = std::make_unique<SyncSchedulerImpl>(
"TestSyncScheduler", BackoffDelayProvider::FromDefaults(), context(),
syncer_, false);
@@ -674,6 +681,30 @@
AnalyzePollRun(times, kMinNumSamples, optimal_start, poll_interval);
}
+// Test that polling gets the intervals from the provided context.
+TEST_F(SyncSchedulerImplTest, ShouldUseInitialPollIntervalFromContext) {
+ SyncShareTimes times;
+ TimeDelta poll_interval(TimeDelta::FromMilliseconds(30));
+ context()->set_short_poll_interval(poll_interval);
+ context()->set_long_poll_interval(poll_interval);
+ RebuildScheduler();
+
+ EXPECT_CALL(*syncer(), PollSyncShare(_, _))
+ .Times(AtLeast(kMinNumSamples))
+ .WillRepeatedly(
+ DoAll(Invoke(test_util::SimulatePollSuccess),
+ RecordSyncShareMultiple(×, kMinNumSamples, true)));
+
+ TimeTicks optimal_start = TimeTicks::Now() + poll_interval;
+ StartSyncScheduler(base::Time());
+
+ // Run again to wait for polling.
+ RunLoop();
+
+ StopSyncScheduler();
+ AnalyzePollRun(times, kMinNumSamples, optimal_start, poll_interval);
+}
+
// Test that we reuse the previous poll time on startup, triggering the first
// poll based on when the last one happened. Subsequent polls should have the
// normal delay.
diff --git a/components/sync/engine_impl/syncer_proto_util.cc b/components/sync/engine_impl/syncer_proto_util.cc
index d6b5b4e..f564d37e 100644
--- a/components/sync/engine_impl/syncer_proto_util.cc
+++ b/components/sync/engine_impl/syncer_proto_util.cc
@@ -410,13 +410,27 @@
}
if (command.has_set_sync_long_poll_interval()) {
- cycle->delegate()->OnReceivedLongPollIntervalUpdate(
- base::TimeDelta::FromSeconds(command.set_sync_long_poll_interval()));
+ base::TimeDelta interval =
+ base::TimeDelta::FromSeconds(command.set_sync_long_poll_interval());
+ if (interval.is_zero()) {
+ DLOG(WARNING)
+ << "Received zero long poll interval from server. Ignoring.";
+ } else {
+ cycle->context()->set_long_poll_interval(interval);
+ cycle->delegate()->OnReceivedLongPollIntervalUpdate(interval);
+ }
}
if (command.has_set_sync_poll_interval()) {
- cycle->delegate()->OnReceivedShortPollIntervalUpdate(
- base::TimeDelta::FromSeconds(command.set_sync_poll_interval()));
+ base::TimeDelta interval =
+ base::TimeDelta::FromSeconds(command.set_sync_poll_interval());
+ if (interval.is_zero()) {
+ DLOG(WARNING)
+ << "Received zero short poll interval from server. Ignoring.";
+ } else {
+ cycle->context()->set_short_poll_interval(interval);
+ cycle->delegate()->OnReceivedShortPollIntervalUpdate(interval);
+ }
}
if (command.has_sessions_commit_delay_seconds()) {
diff --git a/components/sync/engine_impl/syncer_unittest.cc b/components/sync/engine_impl/syncer_unittest.cc
index c95fb2c..312946a9 100644
--- a/components/sync/engine_impl/syncer_unittest.cc
+++ b/components/sync/engine_impl/syncer_unittest.cc
@@ -288,7 +288,9 @@
debug_info_getter_.get(), model_type_registry_.get(),
true, // enable keystore encryption
false, // force enable pre-commit GU avoidance experiment
- "fake_invalidator_client_id");
+ "fake_invalidator_client_id",
+ /*short_poll_interval=*/base::TimeDelta::FromMinutes(30),
+ /*long_poll_interval=*/base::TimeDelta::FromMinutes(180));
syncer_ = new Syncer(&cancelation_signal_);
scheduler_ = std::make_unique<SyncSchedulerImpl>(
"TestSyncScheduler", BackoffDelayProvider::FromDefaults(),
diff --git a/components/sync/test/fake_server/fake_server.cc b/components/sync/test/fake_server/fake_server.cc
index e494b7e..a754fe6 100644
--- a/components/sync/test/fake_server/fake_server.cc
+++ b/components/sync/test/fake_server/fake_server.cc
@@ -21,8 +21,6 @@
#include "net/base/net_errors.h"
#include "net/http/http_status_code.h"
-using std::string;
-using std::vector;
using syncer::GetModelType;
using syncer::GetModelTypeFromSpecifics;
using syncer::LoopbackServer;
@@ -51,7 +49,7 @@
FakeServer::~FakeServer() {}
-void FakeServer::HandleCommand(const string& request,
+void FakeServer::HandleCommand(const std::string& request,
const base::Closure& completion_closure,
int* error_code,
int* response_code,
@@ -61,7 +59,7 @@
if (!network_enabled_) {
*error_code = net::ERR_FAILED;
*response_code = net::ERR_FAILED;
- *response = string();
+ *response = std::string();
completion_closure.Run();
return;
}
@@ -70,7 +68,7 @@
if (!authenticated_) {
*error_code = 0;
*response_code = net::HTTP_UNAUTHORIZED;
- *response = string();
+ *response = std::string();
completion_closure.Run();
return;
}
@@ -100,13 +98,10 @@
break;
// Don't care.
}
-
- int64_t response_code_large;
- syncer::HttpResponse::ServerConnectionCode server_status;
- base::ThreadRestrictions::SetIOAllowed(true);
- loopback_server_->HandleCommand(request, &server_status,
- &response_code_large, response);
- *response_code = static_cast<int>(response_code_large);
+ *response_code = SendToLoopbackServer(request, response);
+ if (*response_code == net::HTTP_OK) {
+ InjectClientCommand(response);
+ }
completion_closure.Run();
return;
}
@@ -116,6 +111,26 @@
completion_closure.Run();
}
+int FakeServer::SendToLoopbackServer(const std::string& request,
+ std::string* response) {
+ int64_t response_code;
+ syncer::HttpResponse::ServerConnectionCode server_status;
+ base::ThreadRestrictions::SetIOAllowed(true);
+ loopback_server_->HandleCommand(request, &server_status, &response_code,
+ response);
+ return static_cast<int>(response_code);
+}
+
+void FakeServer::InjectClientCommand(std::string* response) {
+ sync_pb::ClientToServerResponse response_proto;
+ bool parse_ok = response_proto.ParseFromString(*response);
+ DCHECK(parse_ok) << "Unable to parse-back the server response";
+ if (response_proto.error_code() == sync_pb::SyncEnums::SUCCESS) {
+ *response_proto.mutable_client_command() = client_command_;
+ *response = response_proto.SerializeAsString();
+ }
+}
+
bool FakeServer::GetLastCommitMessage(sync_pb::ClientToServerMessage* message) {
if (!last_commit_message_.has_commit())
return false;
@@ -184,6 +199,12 @@
authenticated_ = false;
}
+void FakeServer::SetClientCommand(
+ const sync_pb::ClientCommand& client_command) {
+ DCHECK(thread_checker_.CalledOnValidThread());
+ client_command_ = client_command;
+}
+
bool FakeServer::TriggerError(const sync_pb::SyncEnums::ErrorType& error_type) {
DCHECK(thread_checker_.CalledOnValidThread());
if (triggered_actionable_error_.get()) {
@@ -197,8 +218,8 @@
bool FakeServer::TriggerActionableError(
const sync_pb::SyncEnums::ErrorType& error_type,
- const string& description,
- const string& url,
+ const std::string& description,
+ const std::string& url,
const sync_pb::SyncEnums::Action& action) {
DCHECK(thread_checker_.CalledOnValidThread());
if (error_type_ != sync_pb::SyncEnums::SUCCESS) {
diff --git a/components/sync/test/fake_server/fake_server.h b/components/sync/test/fake_server/fake_server.h
index 1355e38a..ab14de8 100644
--- a/components/sync/test/fake_server/fake_server.h
+++ b/components/sync/test/fake_server/fake_server.h
@@ -23,6 +23,7 @@
#include "components/sync/engine_impl/loopback_server/persistent_bookmark_entity.h"
#include "components/sync/engine_impl/loopback_server/persistent_tombstone_entity.h"
#include "components/sync/engine_impl/loopback_server/persistent_unique_client_entity.h"
+#include "components/sync/protocol/client_commands.pb.h"
#include "components/sync/protocol/sync.pb.h"
namespace fake_server {
@@ -107,6 +108,9 @@
// authentication error.
void SetUnauthenticated();
+ // Sets the provided |client_command| in all subsequent successful requests.
+ void SetClientCommand(const sync_pb::ClientCommand& client_command);
+
// Force the server to return |error_type| in the error_code field of
// ClientToServerResponse on all subsequent sync requests. This method should
// not be called if TriggerActionableError has previously been called. Returns
@@ -164,6 +168,8 @@
private:
// Returns whether a triggered error should be sent for the request.
bool ShouldSendTriggeredError() const;
+ int SendToLoopbackServer(const std::string& request, std::string* response);
+ void InjectClientCommand(std::string* response);
// Whether the server should act as if incoming connections are properly
// authenticated.
@@ -191,6 +197,9 @@
bool alternate_triggered_errors_;
int request_counter_;
+ // Client command to be included in every response.
+ sync_pb::ClientCommand client_command_;
+
// FakeServer's observers.
base::ObserverList<Observer, true> observers_;
