summaryrefslogtreecommitdiffstats
path: root/sync
diff options
context:
space:
mode:
Diffstat (limited to 'sync')
-rw-r--r--sync/engine/verify_updates_command.cc66
-rw-r--r--sync/protocol/client_debug_info.proto7
-rw-r--r--sync/protocol/get_updates_caller_info.proto45
-rw-r--r--sync/protocol/sync.proto35
-rw-r--r--sync/protocol/sync_proto.gyp1
-rw-r--r--sync/sessions/session_state.cc6
-rw-r--r--sync/sessions/session_state.h3
-rw-r--r--sync/sessions/session_state_unittest.cc9
-rw-r--r--sync/sessions/status_controller.cc6
-rw-r--r--sync/sessions/status_controller.h1
-rw-r--r--sync/sessions/sync_session.cc1
11 files changed, 128 insertions, 52 deletions
diff --git a/sync/engine/verify_updates_command.cc b/sync/engine/verify_updates_command.cc
index ab555a2..c4ebe67 100644
--- a/sync/engine/verify_updates_command.cc
+++ b/sync/engine/verify_updates_command.cc
@@ -22,6 +22,54 @@ using syncable::WriteTransaction;
using syncable::GET_BY_ID;
using syncable::SYNCER;
+namespace {
+
+// This function attempts to determine whether or not this update is genuinely
+// new, or if it is a reflection of one of our own commits.
+//
+// There is a known inaccuracy in its implementation. If this update ends up
+// being applied to a local item with a different ID, we will count the change
+// as being a non-reflection update. Fortunately, the server usually updates
+// our IDs correctly in its commit response, so a new ID during GetUpdate should
+// be rare.
+//
+// The only secnarios I can think of where this might happen are:
+// - We commit a new item to the server, but we don't persist the
+// server-returned new ID to the database before we shut down. On the GetUpdate
+// following the next restart, we will receive an update from the server that
+// updates its local ID.
+// - When two attempts to create an item with identical UNIQUE_CLIENT_TAG values
+// collide at the server. I have seen this in testing. When it happens, the
+// test server will send one of the clients a response to upate its local ID so
+// that both clients will refer to the item using the same ID going forward. In
+// this case, we're right to assume that the update is not a reflection.
+//
+// For more information, see SyncerUtil::FindLocalIdToUpdate().
+bool UpdateContainsNewVersion(syncable::BaseTransaction *trans,
+ const SyncEntity &update) {
+ int64 existing_version = -1; // The server always sends positive versions.
+ syncable::Entry existing_entry(trans, GET_BY_ID, update.id());
+ if (existing_entry.good())
+ existing_version = existing_entry.Get(syncable::BASE_VERSION);
+
+ return existing_version < update.version();
+}
+
+// In the event that IDs match, but tags differ AttemptReuniteClient tag
+// will have refused to unify the update.
+// We should not attempt to apply it at all since it violates consistency
+// rules.
+VerifyResult VerifyTagConsistency(const SyncEntity& entry,
+ const syncable::MutableEntry& same_id) {
+ if (entry.has_client_defined_unique_tag() &&
+ entry.client_defined_unique_tag() !=
+ same_id.Get(syncable::UNIQUE_CLIENT_TAG)) {
+ return VERIFY_FAIL;
+ }
+ return VERIFY_UNDECIDED;
+}
+} // namespace
+
VerifyUpdatesCommand::VerifyUpdatesCommand() {}
VerifyUpdatesCommand::~VerifyUpdatesCommand() {}
@@ -62,6 +110,8 @@ SyncerError VerifyUpdatesCommand::ModelChangingExecuteImpl(
session->routing_info());
status->mutable_update_progress()->AddVerifyResult(result.value, update);
status->increment_num_updates_downloaded_by(1);
+ if (!UpdateContainsNewVersion(&trans, update))
+ status->increment_num_reflected_updates_downloaded_by(1);
if (update.deleted())
status->increment_num_tombstone_updates_downloaded_by(1);
}
@@ -69,22 +119,6 @@ SyncerError VerifyUpdatesCommand::ModelChangingExecuteImpl(
return SYNCER_OK;
}
-namespace {
-// In the event that IDs match, but tags differ AttemptReuniteClient tag
-// will have refused to unify the update.
-// We should not attempt to apply it at all since it violates consistency
-// rules.
-VerifyResult VerifyTagConsistency(const SyncEntity& entry,
- const syncable::MutableEntry& same_id) {
- if (entry.has_client_defined_unique_tag() &&
- entry.client_defined_unique_tag() !=
- same_id.Get(syncable::UNIQUE_CLIENT_TAG)) {
- return VERIFY_FAIL;
- }
- return VERIFY_UNDECIDED;
-}
-} // namespace
-
VerifyUpdatesCommand::VerifyUpdateResult VerifyUpdatesCommand::VerifyUpdate(
syncable::WriteTransaction* trans, const SyncEntity& entry,
const ModelSafeRoutingInfo& routes) {
diff --git a/sync/protocol/client_debug_info.proto b/sync/protocol/client_debug_info.proto
index 15216f8..6a0fa54 100644
--- a/sync/protocol/client_debug_info.proto
+++ b/sync/protocol/client_debug_info.proto
@@ -11,6 +11,8 @@ option retain_unknown_fields = true;
package sync_pb;
+import "get_updates_caller_info.proto";
+
// The additional info here is from SyncerStatus. They get sent when the event
// SYNC_CYCLE_COMPLETED is sent.
message SyncCycleCompletedEventInfo {
@@ -31,6 +33,11 @@ message SyncCycleCompletedEventInfo {
optional int32 num_hierarchy_conflicts = 5;
optional int32 num_simple_conflicts = 6;
optional int32 num_server_conflicts = 7;
+
+ // Counts to track the effective usefulness of our GetUpdate requests.
+ optional int32 num_updates_downloaded = 8;
+ optional int32 num_reflected_updates_downloaded = 9;
+ optional GetUpdatesCallerInfo caller_info = 10;
}
message DebugEventInfo {
diff --git a/sync/protocol/get_updates_caller_info.proto b/sync/protocol/get_updates_caller_info.proto
new file mode 100644
index 0000000..41ad2ef
--- /dev/null
+++ b/sync/protocol/get_updates_caller_info.proto
@@ -0,0 +1,45 @@
+// Copyright (c) 2012 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.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+option retain_unknown_fields = true;
+
+package sync_pb;
+
+message GetUpdatesCallerInfo {
+ enum GetUpdatesSource {
+ UNKNOWN = 0; // The source was not set by the caller.
+ FIRST_UPDATE = 1; // First request after browser restart. Not to
+ // be confused with "NEW_CLIENT".
+ LOCAL = 2; // The source of the update was a local change.
+ NOTIFICATION = 3; // The source of the update was a p2p notification.
+ PERIODIC = 4; // The source of the update was periodic polling.
+ SYNC_CYCLE_CONTINUATION = 5; // The source of the update was a
+ // continuation of a previous update.
+ CLEAR_PRIVATE_DATA = 6; // Source is a call to remove all private data
+ NEWLY_SUPPORTED_DATATYPE = 7; // The client is in configuration mode
+ // because it's syncing all datatypes, and
+ // support for a new datatype was recently
+ // released via a software auto-update.
+ MIGRATION = 8; // The client is in configuration mode because a
+ // MIGRATION_DONE error previously returned by the
+ // server necessitated resynchronization.
+ NEW_CLIENT = 9; // The client is in configuration mode because the
+ // user enabled sync for the first time. Not to be
+ // confused with FIRST_UPDATE.
+ RECONFIGURATION = 10; // The client is in configuration mode because the
+ // user opted to sync a different set of datatypes.
+ DATATYPE_REFRESH = 11; // A datatype has requested a refresh. This is
+ // typically used when datatype's have custom
+ // sync UI, e.g. sessions.
+ }
+
+ required GetUpdatesSource source = 1;
+
+ // True only if notifications were enabled for this GetUpdateMessage.
+ optional bool notifications_enabled = 2;
+};
+
diff --git a/sync/protocol/sync.proto b/sync/protocol/sync.proto
index 6191000..dccf99f 100644
--- a/sync/protocol/sync.proto
+++ b/sync/protocol/sync.proto
@@ -19,6 +19,7 @@ import "app_setting_specifics.proto";
import "app_specifics.proto";
import "autofill_specifics.proto";
import "bookmark_specifics.proto";
+import "get_updates_caller_info.proto";
import "extension_setting_specifics.proto";
import "extension_specifics.proto";
import "nigori_specifics.proto";
@@ -355,40 +356,6 @@ message CommitMessage {
repeated ChromiumExtensionsActivity extensions_activity = 3;
};
-message GetUpdatesCallerInfo {
- enum GetUpdatesSource {
- UNKNOWN = 0; // The source was not set by the caller.
- FIRST_UPDATE = 1; // First request after browser restart. Not to
- // be confused with "NEW_CLIENT".
- LOCAL = 2; // The source of the update was a local change.
- NOTIFICATION = 3; // The source of the update was a p2p notification.
- PERIODIC = 4; // The source of the update was periodic polling.
- SYNC_CYCLE_CONTINUATION = 5; // The source of the update was a
- // continuation of a previous update.
- CLEAR_PRIVATE_DATA = 6; // Source is a call to remove all private data
- NEWLY_SUPPORTED_DATATYPE = 7; // The client is in configuration mode
- // because it's syncing all datatypes, and
- // support for a new datatype was recently
- // released via a software auto-update.
- MIGRATION = 8; // The client is in configuration mode because a
- // MIGRATION_DONE error previously returned by the
- // server necessitated resynchronization.
- NEW_CLIENT = 9; // The client is in configuration mode because the
- // user enabled sync for the first time. Not to be
- // confused with FIRST_UPDATE.
- RECONFIGURATION = 10; // The client is in configuration mode because the
- // user opted to sync a different set of datatypes.
- DATATYPE_REFRESH = 11; // A datatype has requested a refresh. This is
- // typically used when datatype's have custom
- // sync UI, e.g. sessions.
- }
-
- required GetUpdatesSource source = 1;
-
- // True only if notifications were enabled for this GetUpdateMessage.
- optional bool notifications_enabled = 2;
-};
-
message DataTypeProgressMarker {
// An integer identifying the data type whose progress is tracked by this
// marker. The legitimate values of this field correspond to the protobuf
diff --git a/sync/protocol/sync_proto.gyp b/sync/protocol/sync_proto.gyp
index 28198b8..c9d5627 100644
--- a/sync/protocol/sync_proto.gyp
+++ b/sync/protocol/sync_proto.gyp
@@ -18,6 +18,7 @@
'bookmark_specifics.proto',
'client_commands.proto',
'client_debug_info.proto',
+ 'get_updates_caller_info.proto',
'encryption.proto',
'extension_setting_specifics.proto',
'extension_specifics.proto',
diff --git a/sync/sessions/session_state.cc b/sync/sessions/session_state.cc
index 2c68125..f49c41d 100644
--- a/sync/sessions/session_state.cc
+++ b/sync/sessions/session_state.cc
@@ -50,6 +50,7 @@ SyncerStatus::SyncerStatus()
num_successful_bookmark_commits(0),
num_updates_downloaded_total(0),
num_tombstone_updates_downloaded_total(0),
+ num_reflected_updates_downloaded_total(0),
num_local_overwrites(0),
num_server_overwrites(0) {
}
@@ -67,6 +68,8 @@ DictionaryValue* SyncerStatus::ToValue() const {
num_updates_downloaded_total);
value->SetInteger("numTombstoneUpdatesDownloadedTotal",
num_tombstone_updates_downloaded_total);
+ value->SetInteger("numReflectedUpdatesDownloadedTotal",
+ num_reflected_updates_downloaded_total);
value->SetInteger("numLocalOverwrites", num_local_overwrites);
value->SetInteger("numServerOverwrites", num_server_overwrites);
return value;
@@ -113,6 +116,7 @@ SyncSessionSnapshot::SyncSessionSnapshot(
int num_server_conflicts,
bool did_commit_items,
const SyncSourceInfo& source,
+ bool notifications_enabled,
size_t num_entries,
base::Time sync_start_time,
bool retry_scheduled)
@@ -131,6 +135,7 @@ SyncSessionSnapshot::SyncSessionSnapshot(
num_server_conflicts(num_server_conflicts),
did_commit_items(did_commit_items),
source(source),
+ notifications_enabled(notifications_enabled),
num_entries(num_entries),
sync_start_time(sync_start_time),
retry_scheduled(retry_scheduled) {
@@ -170,6 +175,7 @@ DictionaryValue* SyncSessionSnapshot::ToValue() const {
value->SetBoolean("didCommitItems", did_commit_items);
value->SetInteger("numEntries", num_entries);
value->Set("source", source.ToValue());
+ value->SetBoolean("notificationsEnabled", notifications_enabled);
return value;
}
diff --git a/sync/sessions/session_state.h b/sync/sessions/session_state.h
index 676fe5b..7403758 100644
--- a/sync/sessions/session_state.h
+++ b/sync/sessions/session_state.h
@@ -72,6 +72,7 @@ struct SyncerStatus {
// Download event counters.
int num_updates_downloaded_total;
int num_tombstone_updates_downloaded_total;
+ int num_reflected_updates_downloaded_total;
// If the syncer encountered a MIGRATION_DONE code, these are the types that
// the client must now "migrate", by purging and re-downloading all updates.
@@ -121,6 +122,7 @@ struct SyncSessionSnapshot {
int num_server_conflicts,
bool did_commit_items,
const SyncSourceInfo& source,
+ bool notifications_enabled,
size_t num_entries,
base::Time sync_start_time,
bool retry_scheduled);
@@ -146,6 +148,7 @@ struct SyncSessionSnapshot {
const int num_server_conflicts;
const bool did_commit_items;
const SyncSourceInfo source;
+ const bool notifications_enabled;
const size_t num_entries;
base::Time sync_start_time;
const bool retry_scheduled;
diff --git a/sync/sessions/session_state_unittest.cc b/sync/sessions/session_state_unittest.cc
index b18964a..a37dc17 100644
--- a/sync/sessions/session_state_unittest.cc
+++ b/sync/sessions/session_state_unittest.cc
@@ -49,11 +49,12 @@ TEST_F(SessionStateTest, SyncerStatusToValue) {
status.num_successful_bookmark_commits = 10;
status.num_updates_downloaded_total = 100;
status.num_tombstone_updates_downloaded_total = 200;
+ status.num_reflected_updates_downloaded_total = 50;
status.num_local_overwrites = 15;
status.num_server_overwrites = 18;
scoped_ptr<DictionaryValue> value(status.ToValue());
- EXPECT_EQ(7u, value->size());
+ EXPECT_EQ(8u, value->size());
ExpectDictBooleanValue(status.invalid_store, *value, "invalidStore");
ExpectDictIntegerValue(status.num_successful_commits,
*value, "numSuccessfulCommits");
@@ -63,6 +64,8 @@ TEST_F(SessionStateTest, SyncerStatusToValue) {
*value, "numUpdatesDownloadedTotal");
ExpectDictIntegerValue(status.num_tombstone_updates_downloaded_total,
*value, "numTombstoneUpdatesDownloadedTotal");
+ ExpectDictIntegerValue(status.num_reflected_updates_downloaded_total,
+ *value, "numReflectedUpdatesDownloadedTotal");
ExpectDictIntegerValue(status.num_local_overwrites,
*value, "numLocalOverwrites");
ExpectDictIntegerValue(status.num_server_overwrites,
@@ -141,11 +144,12 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
kNumServerConflicts,
kDidCommitItems,
source,
+ false,
0,
base::Time::Now(),
false);
scoped_ptr<DictionaryValue> value(snapshot.ToValue());
- EXPECT_EQ(15u, value->size());
+ EXPECT_EQ(16u, value->size());
ExpectDictDictionaryValue(*expected_syncer_status_value, *value,
"syncerStatus");
ExpectDictIntegerValue(kNumServerChangesRemaining, *value,
@@ -169,6 +173,7 @@ TEST_F(SessionStateTest, SyncSessionSnapshotToValue) {
ExpectDictBooleanValue(kDidCommitItems, *value,
"didCommitItems");
ExpectDictDictionaryValue(*expected_source_value, *value, "source");
+ ExpectDictBooleanValue(false, *value, "notificationsEnabled");
}
} // namespace
diff --git a/sync/sessions/status_controller.cc b/sync/sessions/status_controller.cc
index 2ae1a96..19fdbaf 100644
--- a/sync/sessions/status_controller.cc
+++ b/sync/sessions/status_controller.cc
@@ -116,6 +116,12 @@ void StatusController::increment_num_tombstone_updates_downloaded_by(
value;
}
+void StatusController::increment_num_reflected_updates_downloaded_by(
+ int value) {
+ shared_.syncer_status.mutate()->num_reflected_updates_downloaded_total +=
+ value;
+}
+
void StatusController::set_num_server_changes_remaining(
int64 changes_remaining) {
if (shared_.num_server_changes_remaining.value() != changes_remaining)
diff --git a/sync/sessions/status_controller.h b/sync/sessions/status_controller.h
index 6a2491c..c2a3cfc 100644
--- a/sync/sessions/status_controller.h
+++ b/sync/sessions/status_controller.h
@@ -214,6 +214,7 @@ class StatusController {
void increment_num_successful_bookmark_commits();
void increment_num_updates_downloaded_by(int value);
void increment_num_tombstone_updates_downloaded_by(int value);
+ void increment_num_reflected_updates_downloaded_by(int value);
void set_types_needing_local_migration(syncable::ModelTypeSet types);
void set_unsynced_handles(const std::vector<int64>& unsynced_handles);
void increment_num_local_overwrites();
diff --git a/sync/sessions/sync_session.cc b/sync/sessions/sync_session.cc
index 2a93b44..307ae97 100644
--- a/sync/sessions/sync_session.cc
+++ b/sync/sessions/sync_session.cc
@@ -165,6 +165,7 @@ SyncSessionSnapshot SyncSession::TakeSnapshot() const {
status_controller_->TotalNumServerConflictingItems(),
status_controller_->did_commit_items(),
source_,
+ context_->notifications_enabled(),
dir->GetEntriesCount(),
status_controller_->sync_start_time(),
!Succeeded());
generated by cgit v1.1 at 2024-09-09 13:41:01 +0000