Make it clear what last_sync_timestamp actually tracks. Update
last_sync_timestamp from the new_timestamp only, never from per-entry
timestamps. Use what the server sends us to know whether or not there
are more updates to fetch. Eliminate some unnecessarily complicated
logic having to do with the # of updates returned -- that's always a red
herring; with server-side filtering, it is indeed possible for 0 updates
to be returned along with a new timestamp.
BUG=37373
TEST=manual testing of 2 browser sync; unit tests.
Review URL: http://codereview.chromium.org/1161006
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@42413 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/cocoa/preferences_window_controller.h b/chrome/browser/cocoa/preferences_window_controller.h
index 33fce3ac..03a8ca5 100644
--- a/chrome/browser/cocoa/preferences_window_controller.h
+++ b/chrome/browser/cocoa/preferences_window_controller.h
@@ -24,7 +24,7 @@
// A window controller that handles the preferences window. The bulk of the
// work is handled via Cocoa Bindings and getter/setter methods that wrap
// cross-platform PrefMember objects. When prefs change in the back-end
-// (that is, outside of this UI), our observer recieves a notification and can
+// (that is, outside of this UI), our observer receives a notification and can
// tickle the KVO to update the UI so we are always in sync. The bindings are
// specified in the nib file. Preferences are persisted into the back-end
// as they are changed in the UI, and are thus immediately available even while
diff --git a/chrome/browser/extensions/extension_message_service.cc b/chrome/browser/extensions/extension_message_service.cc
index a58c600..45b53045 100644
--- a/chrome/browser/extensions/extension_message_service.cc
+++ b/chrome/browser/extensions/extension_message_service.cc
@@ -328,7 +328,7 @@
const std::string& channel_name) {
DCHECK_EQ(MessageLoop::current()->type(), MessageLoop::TYPE_UI);
- // TODO(mpcomplete): notify source if reciever doesn't exist
+ // TODO(mpcomplete): notify source if receiver doesn't exist
if (!source)
return false; // Closed while in flight.
diff --git a/chrome/browser/importer/firefox_importer_unittest_utils.h b/chrome/browser/importer/firefox_importer_unittest_utils.h
index 6db8ee4..3eb12d6 100644
--- a/chrome/browser/importer/firefox_importer_unittest_utils.h
+++ b/chrome/browser/importer/firefox_importer_unittest_utils.h
@@ -44,7 +44,7 @@
#if defined(OS_MACOSX)
// Blocks until either a timeout is reached, or until the client process
// responds to an IPC message.
- // Returns true if a reply was recieved successfully and false if the
+ // Returns true if a reply was received successfully and false if the
// the operation timed out.
bool WaitForClientResponse();
diff --git a/chrome/browser/sync/engine/download_updates_command.cc b/chrome/browser/sync/engine/download_updates_command.cc
index c7ad790e..59d5e5e 100644
--- a/chrome/browser/sync/engine/download_updates_command.cc
+++ b/chrome/browser/sync/engine/download_updates_command.cc
@@ -39,8 +39,8 @@
LOG(ERROR) << "Scoped dir lookup failed!";
return;
}
- LOG(INFO) << "Getting updates from ts " << dir->last_sync_timestamp();
- get_updates->set_from_timestamp(dir->last_sync_timestamp());
+ LOG(INFO) << "Getting updates from ts " << dir->last_download_timestamp();
+ get_updates->set_from_timestamp(dir->last_download_timestamp());
// We want folders for our associated types, always. If we were to set
// this to false, the server would send just the non-container items
@@ -67,10 +67,15 @@
StatusController* status = session->status_controller();
if (!ok) {
status->increment_num_consecutive_errors();
- LOG(ERROR) << "PostClientToServerMessage() failed";
+ status->mutable_updates_response()->Clear();
+ LOG(ERROR) << "PostClientToServerMessage() failed during GetUpdates";
return;
}
status->mutable_updates_response()->CopyFrom(update_response);
+
+ LOG(INFO) << "GetUpdates from ts " << get_updates->from_timestamp()
+ << " returned " << update_response.get_updates().entries_size()
+ << " updates.";
}
void DownloadUpdatesCommand::SetRequestedTypes(
diff --git a/chrome/browser/sync/engine/process_updates_command.cc b/chrome/browser/sync/engine/process_updates_command.cc
index 03af3033..77a12724 100644
--- a/chrome/browser/sync/engine/process_updates_command.cc
+++ b/chrome/browser/sync/engine/process_updates_command.cc
@@ -25,6 +25,15 @@
ProcessUpdatesCommand::ProcessUpdatesCommand() {}
ProcessUpdatesCommand::~ProcessUpdatesCommand() {}
+bool ProcessUpdatesCommand::ModelNeutralExecuteImpl(SyncSession* session) {
+ const GetUpdatesResponse& updates =
+ session->status_controller()->updates_response().get_updates();
+ const int update_count = updates.entries_size();
+
+ // Don't bother processing updates if there were none.
+ return update_count != 0;
+}
+
void ProcessUpdatesCommand::ModelChangingExecuteImpl(SyncSession* session) {
syncable::ScopedDirLookup dir(session->context()->directory_manager(),
session->context()->account_name());
@@ -33,38 +42,8 @@
return;
}
- const GetUpdatesResponse& updates =
- session->status_controller()->updates_response().get_updates();
- const int update_count = updates.entries_size();
-
- LOG(INFO) << "Get updates from ts " << dir->last_sync_timestamp() <<
- " returned " << update_count << " updates.";
-
StatusController* status = session->status_controller();
- if (updates.has_changes_remaining()) {
- int64 changes_left = updates.changes_remaining();
- LOG(INFO) << "Changes remaining:" << changes_left;
- status->set_num_server_changes_remaining(changes_left);
- }
- int64 new_timestamp = 0;
- if (updates.has_new_timestamp()) {
- new_timestamp = updates.new_timestamp();
- LOG(INFO) << "Get Updates got new timestamp: " << new_timestamp;
- if (0 == update_count) {
- if (new_timestamp > dir->last_sync_timestamp()) {
- dir->set_last_sync_timestamp(new_timestamp);
- status->set_got_new_timestamp();
- }
- return;
- }
- }
-
- // If we have updates that are ALL supposed to be skipped, we don't want to
- // get them again. In fact, the account's final updates are all supposed to
- // be skipped and we DON'T step past them, we will sync forever.
- int64 latest_skip_timestamp = 0;
- bool any_non_skip_results = false;
const sessions::UpdateProgress& progress(status->update_progress());
vector<sessions::VerifiedUpdate>::const_iterator it;
for (it = progress.VerifiedUpdatesBegin();
@@ -72,46 +51,21 @@
++it) {
const sync_pb::SyncEntity& update = it->second;
- any_non_skip_results = (it->first != VERIFY_SKIP);
- if (!any_non_skip_results) {
- // ALL updates were to be skipped, including this one.
- if (update.sync_timestamp() > latest_skip_timestamp) {
- latest_skip_timestamp = update.sync_timestamp();
- }
- } else {
- latest_skip_timestamp = 0;
- }
-
if (it->first != VERIFY_SUCCESS && it->first != VERIFY_UNDELETE)
continue;
switch (ProcessUpdate(dir, update)) {
case SUCCESS_PROCESSED:
case SUCCESS_STORED:
- // We can update the timestamp because we store the update even if we
- // can't apply it now.
- if (update.sync_timestamp() > new_timestamp)
- new_timestamp = update.sync_timestamp();
break;
default:
NOTREACHED();
break;
}
-
- }
-
- if (latest_skip_timestamp > new_timestamp)
- new_timestamp = latest_skip_timestamp;
-
- if (new_timestamp > dir->last_sync_timestamp()) {
- dir->set_last_sync_timestamp(new_timestamp);
- status->set_got_new_timestamp();
}
status->set_num_consecutive_errors(0);
- // TODO(tim): Related to bug 30665, the Directory needs last sync timestamp
- // per data type. Until then, use UNSPECIFIED.
- status->set_current_sync_timestamp(syncable::UNSPECIFIED,
- dir->last_sync_timestamp());
+
+ // TODO(nick): The following line makes no sense to me.
status->set_syncing(true);
return;
}
diff --git a/chrome/browser/sync/engine/process_updates_command.h b/chrome/browser/sync/engine/process_updates_command.h
index e1d1434..21e07ac 100644
--- a/chrome/browser/sync/engine/process_updates_command.h
+++ b/chrome/browser/sync/engine/process_updates_command.h
@@ -32,6 +32,7 @@
virtual ~ProcessUpdatesCommand();
// ModelChangingSyncerCommand implementation.
+ virtual bool ModelNeutralExecuteImpl(sessions::SyncSession* session);
virtual void ModelChangingExecuteImpl(sessions::SyncSession* session);
private:
diff --git a/chrome/browser/sync/engine/store_timestamps_command.cc b/chrome/browser/sync/engine/store_timestamps_command.cc
new file mode 100755
index 0000000..1625e82
--- /dev/null
+++ b/chrome/browser/sync/engine/store_timestamps_command.cc
@@ -0,0 +1,49 @@
+// Copyright (c) 2010 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/engine/store_timestamps_command.h"
+
+#include "chrome/browser/sync/sessions/status_controller.h"
+#include "chrome/browser/sync/sessions/sync_session.h"
+#include "chrome/browser/sync/syncable/directory_manager.h"
+#include "chrome/browser/sync/syncable/syncable.h"
+
+namespace browser_sync {
+
+StoreTimestampsCommand::StoreTimestampsCommand() {}
+StoreTimestampsCommand::~StoreTimestampsCommand() {}
+
+void StoreTimestampsCommand::ExecuteImpl(sessions::SyncSession* session) {
+ syncable::ScopedDirLookup dir(session->context()->directory_manager(),
+ session->context()->account_name());
+
+ if (!dir.good()) {
+ LOG(ERROR) << "Scoped dir lookup failed!";
+ return;
+ }
+
+ const GetUpdatesResponse& updates =
+ session->status_controller()->updates_response().get_updates();
+
+ sessions::StatusController* status = session->status_controller();
+ if (updates.has_changes_remaining()) {
+ int64 changes_left = updates.changes_remaining();
+ LOG(INFO) << "Changes remaining:" << changes_left;
+ status->set_num_server_changes_remaining(changes_left);
+ }
+
+ if (updates.has_new_timestamp()) {
+ LOG(INFO) << "Get Updates got new timestamp: " << updates.new_timestamp();
+ if (updates.new_timestamp() > dir->last_download_timestamp()) {
+ dir->set_last_download_timestamp(updates.new_timestamp());
+ }
+ }
+
+ // TODO(tim): Related to bug 30665, the Directory needs last sync timestamp
+ // per data type. Until then, use UNSPECIFIED.
+ status->set_current_sync_timestamp(syncable::UNSPECIFIED,
+ dir->last_download_timestamp());
+}
+
+} // namespace browser_sync
diff --git a/chrome/browser/sync/engine/store_timestamps_command.h b/chrome/browser/sync/engine/store_timestamps_command.h
new file mode 100755
index 0000000..7d0e47f
--- /dev/null
+++ b/chrome/browser/sync/engine/store_timestamps_command.h
@@ -0,0 +1,39 @@
+// Copyright (c) 2010 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.
+
+#ifndef CHROME_BROWSER_SYNC_ENGINE_STORE_TIMESTAMPS_COMMAND_H_
+#define CHROME_BROWSER_SYNC_ENGINE_STORE_TIMESTAMPS_COMMAND_H_
+
+#include "chrome/browser/sync/engine/syncer_command.h"
+#include "chrome/browser/sync/engine/syncer_types.h"
+
+namespace browser_sync {
+
+// A syncer command that extracts the changelog timestamp information from
+// a GetUpdatesResponse (fetched in DownloadUpdatesCommand) and stores
+// it in the directory. This is meant to run immediately after
+// ProcessUpdatesCommand.
+//
+// Preconditions - all updates in the SyncerSesssion have been stored in the
+// database, meaning it is safe to update the persisted
+// timestamps.
+//
+// Postconditions - The next_timestamp returned by the server will be
+// saved into the directory (where it will be used
+// the next time that DownloadUpdatesCommand runs).
+class StoreTimestampsCommand : public SyncerCommand {
+ public:
+ StoreTimestampsCommand();
+ virtual ~StoreTimestampsCommand();
+
+ // SyncerCommand implementation.
+ virtual void ExecuteImpl(sessions::SyncSession* session);
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(StoreTimestampsCommand);
+};
+
+} // namespace browser_sync
+
+#endif // CHROME_BROWSER_SYNC_ENGINE_STORE_TIMESTAMPS_COMMAND_H_
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index 64b7e41..d9f48bb4 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -1828,10 +1828,6 @@
if (lookup->initial_sync_ended())
MarkAndNotifyInitializationComplete();
-
- // If we are transitioning from having bad to good credentials, that
- // could mean that the syncer can now make forward progress. Wake it.
- syncer_thread_->NudgeSyncer(0, SyncerThread::kLocal);
}
return;
// Authentication failures translate to GoogleServiceAuthError events.
diff --git a/chrome/browser/sync/engine/syncer.cc b/chrome/browser/sync/engine/syncer.cc
index d6656b5..26b98b3f 100644
--- a/chrome/browser/sync/engine/syncer.cc
+++ b/chrome/browser/sync/engine/syncer.cc
@@ -18,6 +18,7 @@
#include "chrome/browser/sync/engine/process_commit_response_command.h"
#include "chrome/browser/sync/engine/process_updates_command.h"
#include "chrome/browser/sync/engine/resolve_conflicts_command.h"
+#include "chrome/browser/sync/engine/store_timestamps_command.h"
#include "chrome/browser/sync/engine/syncer_end_command.h"
#include "chrome/browser/sync/engine/syncer_types.h"
#include "chrome/browser/sync/engine/syncer_util.h"
@@ -139,9 +140,18 @@
LOG(INFO) << "Processing Updates";
ProcessUpdatesCommand process_updates;
process_updates.Execute(session);
+ next_step = STORE_TIMESTAMPS;
+ break;
+ }
+ case STORE_TIMESTAMPS: {
+ LOG(INFO) << "Storing timestamps";
+ StoreTimestampsCommand store_timestamps;
+ store_timestamps.Execute(session);
// We should download all of the updates before attempting to process
// them.
- if (session->status_controller()->CountUpdates() == 0) {
+ if (session->status_controller()->
+ server_says_nothing_more_to_download() ||
+ !session->status_controller()->download_updates_succeeded()) {
next_step = APPLY_UPDATES;
} else {
next_step = DOWNLOAD_UPDATES;
diff --git a/chrome/browser/sync/engine/syncer.h b/chrome/browser/sync/engine/syncer.h
index 7daed364..e6be6d2 100644
--- a/chrome/browser/sync/engine/syncer.h
+++ b/chrome/browser/sync/engine/syncer.h
@@ -47,6 +47,7 @@
PROCESS_CLIENT_COMMAND,
VERIFY_UPDATES,
PROCESS_UPDATES,
+ STORE_TIMESTAMPS,
APPLY_UPDATES,
BUILD_COMMIT_REQUEST,
POST_COMMIT_MESSAGE,
diff --git a/chrome/browser/sync/engine/syncer_end_command.cc b/chrome/browser/sync/engine/syncer_end_command.cc
index 3b57618b..e594542e 100644
--- a/chrome/browser/sync/engine/syncer_end_command.cc
+++ b/chrome/browser/sync/engine/syncer_end_command.cc
@@ -18,10 +18,9 @@
sessions::StatusController* status(session->status_controller());
status->set_syncing(false);
- if (!session->HasMoreToSync()) {
- // This might be the first time we've fully completed a sync cycle.
- DCHECK(status->got_zero_updates());
-
+ // This might be the first time we've fully completed a sync cycle.
+ if (!session->HasMoreToSync() &&
+ status->server_says_nothing_more_to_download()) {
syncable::ScopedDirLookup dir(session->context()->directory_manager(),
session->context()->account_name());
if (!dir.good()) {
diff --git a/chrome/browser/sync/engine/verify_updates_command.cc b/chrome/browser/sync/engine/verify_updates_command.cc
index 40fe8d6..1cf168e 100644
--- a/chrome/browser/sync/engine/verify_updates_command.cc
+++ b/chrome/browser/sync/engine/verify_updates_command.cc
@@ -42,7 +42,7 @@
LOG(INFO) << update_count << " entries to verify";
for (int i = 0; i < update_count; i++) {
- const SyncEntity entry =
+ const SyncEntity& entry =
*reinterpret_cast<const SyncEntity *>(&(updates.entries(i)));
ModelSafeGroup g = GetGroupForModelType(entry.GetModelType(),
session->routing_info());
diff --git a/chrome/browser/sync/notifier/base/linux/async_network_alive_linux.cc b/chrome/browser/sync/notifier/base/linux/async_network_alive_linux.cc
index 28e89a9d..9c096dad 100644
--- a/chrome/browser/sync/notifier/base/linux/async_network_alive_linux.cc
+++ b/chrome/browser/sync/notifier/base/linux/async_network_alive_linux.cc
@@ -69,7 +69,7 @@
return;
}
- // Since we recieved a change from the socket, read the change in.
+ // Since we received a change from the socket, read the change in.
if (FD_ISSET(fd, &rdfs)) {
char buf[4096];
struct iovec iov = { buf, sizeof(buf) };
diff --git a/chrome/browser/sync/sessions/session_state.h b/chrome/browser/sync/sessions/session_state.h
index c03fe4d..eb59f94 100644
--- a/chrome/browser/sync/sessions/session_state.h
+++ b/chrome/browser/sync/sessions/session_state.h
@@ -174,7 +174,7 @@
bool HasConflictingUpdates() const;
private:
- // Some container for updates that failed verification.
+ // Container for updates that passed verification.
std::vector<VerifiedUpdate> verified_updates_;
// Stores the result of the various ApplyUpdate attempts we've made.
@@ -185,8 +185,7 @@
struct SyncCycleControlParameters {
SyncCycleControlParameters() : conflict_sets_built(false),
conflicts_resolved(false),
- items_committed(false),
- got_new_timestamp(false) {}
+ items_committed(false) {}
// Set to true by BuildAndProcessConflictSetsCommand if the RESOLVE_CONFLICTS
// step is needed.
bool conflict_sets_built;
@@ -196,9 +195,6 @@
// Set to true by PostCommitMessageCommand if any commits were successful.
bool items_committed;
-
- // The server sent us updates and a newer timestamp as part of the session.
- bool got_new_timestamp;
};
// DirtyOnWrite wraps a value such that any write operation will update a
diff --git a/chrome/browser/sync/sessions/status_controller.cc b/chrome/browser/sync/sessions/status_controller.cc
index 0d112b3..cfd6fa84 100644
--- a/chrome/browser/sync/sessions/status_controller.cc
+++ b/chrome/browser/sync/sessions/status_controller.cc
@@ -81,10 +81,6 @@
shared_.error_counters.mutate()->consecutive_errors = value;
}
-void StatusController::set_got_new_timestamp() {
- shared_.control_params.got_new_timestamp = true;
-}
-
void StatusController::set_current_sync_timestamp(syncable::ModelType model,
int64 current_timestamp) {
PerModelTypeState* state = GetOrCreateModelTypeState(false, model);
diff --git a/chrome/browser/sync/sessions/status_controller.h b/chrome/browser/sync/sessions/status_controller.h
index 93287cf..87d30df3 100644
--- a/chrome/browser/sync/sessions/status_controller.h
+++ b/chrome/browser/sync/sessions/status_controller.h
@@ -82,7 +82,7 @@
ClientToServerResponse* mutable_commit_response() {
return &shared_.commit_response;
}
- const ClientToServerResponse& updates_response() {
+ const ClientToServerResponse& updates_response() const {
return shared_.updates_response;
}
ClientToServerResponse* mutable_updates_response() {
@@ -135,9 +135,6 @@
bool conflicts_resolved() const {
return shared_.control_params.conflicts_resolved;
}
- bool got_new_timestamp() const {
- return shared_.control_params.got_new_timestamp;
- }
bool did_commit_items() const {
return shared_.control_params.items_committed;
}
@@ -159,7 +156,23 @@
return shared_.commit_set.HasBookmarkCommitId();
}
- bool got_zero_updates() const { return CountUpdates() == 0; }
+ // Returns true if the last download_updates_command received a valid
+ // server response.
+ bool download_updates_succeeded() const {
+ return updates_response().has_get_updates();
+ }
+
+ // Returns true if the last updates response indicated that we were fully
+ // up to date. This is subtle: if it's false, it could either mean that
+ // the server said there WAS more to download, or it could mean that we
+ // were unable to reach the server.
+ bool server_says_nothing_more_to_download() const {
+ if (!download_updates_succeeded())
+ return false;
+ // The server indicates "you're up to date" by not sending a new
+ // timestamp.
+ return !updates_response().get_updates().has_new_timestamp();
+ }
ModelSafeGroup group_restriction() const {
return group_restriction_;
@@ -173,7 +186,6 @@
void set_num_consecutive_errors(int value);
void increment_num_consecutive_errors();
void increment_num_consecutive_errors_by(int value);
- void set_got_new_timestamp();
void set_current_sync_timestamp(syncable::ModelType model,
int64 current_timestamp);
void set_num_server_changes_remaining(int64 changes_remaining);
diff --git a/chrome/browser/sync/sessions/status_controller_unittest.cc b/chrome/browser/sync/sessions/status_controller_unittest.cc
index 24c4025d..d1ee710a 100644
--- a/chrome/browser/sync/sessions/status_controller_unittest.cc
+++ b/chrome/browser/sync/sessions/status_controller_unittest.cc
@@ -103,8 +103,6 @@
EXPECT_FALSE(status.TestAndClearIsDirty());
status.update_conflicts_resolved(true);
EXPECT_FALSE(status.TestAndClearIsDirty());
- status.set_got_new_timestamp();
- EXPECT_FALSE(status.TestAndClearIsDirty());
status.set_items_committed();
EXPECT_FALSE(status.TestAndClearIsDirty());
@@ -196,13 +194,11 @@
TEST_F(StatusControllerTest, CountUpdates) {
StatusController status(routes_);
EXPECT_EQ(0, status.CountUpdates());
- EXPECT_TRUE(status.got_zero_updates());
ClientToServerResponse* response(status.mutable_updates_response());
sync_pb::SyncEntity* entity1 = response->mutable_get_updates()->add_entries();
sync_pb::SyncEntity* entity2 = response->mutable_get_updates()->add_entries();
ASSERT_TRUE(entity1 != NULL && entity2 != NULL);
EXPECT_EQ(2, status.CountUpdates());
- EXPECT_FALSE(status.got_zero_updates());
}
// Test TotalNumConflictingItems
@@ -242,7 +238,8 @@
status.ComputeMaxLocalTimestamp();
status.commit_ids();
status.HasBookmarkCommitActivity();
- status.got_zero_updates();
+ status.download_updates_succeeded();
+ status.server_says_nothing_more_to_download();
status.group_restriction();
}
diff --git a/chrome/browser/sync/sessions/sync_session.cc b/chrome/browser/sync/sessions/sync_session.cc
index 6453a4c..7e04d71 100644
--- a/chrome/browser/sync/sessions/sync_session.cc
+++ b/chrome/browser/sync/sessions/sync_session.cc
@@ -53,12 +53,10 @@
return ((status->commit_ids().size() < status->unsynced_handles().size()) &&
status->syncer_status().num_successful_commits > 0) ||
status->conflict_sets_built() ||
- status->conflicts_resolved() ||
+ status->conflicts_resolved();
// Or, we have conflicting updates, but we're making progress on
// resolving them...
- !status->got_zero_updates() ||
- status->got_new_timestamp();
-}
+ }
} // namespace sessions
} // namespace browser_sync
diff --git a/chrome/browser/sync/sessions/sync_session_unittest.cc b/chrome/browser/sync/sessions/sync_session_unittest.cc
index 85c19cd..43f9769d 100644
--- a/chrome/browser/sync/sessions/sync_session_unittest.cc
+++ b/chrome/browser/sync/sessions/sync_session_unittest.cc
@@ -133,18 +133,55 @@
EXPECT_TRUE(session_->HasMoreToSync());
}
-TEST_F(SyncSessionTest, MoreToSyncIfDidNotGetZeroUpdates) {
- // We're not done getting updates until we get an empty response.
- ClientToServerResponse response;
- response.mutable_get_updates()->add_entries();
- status()->mutable_updates_response()->CopyFrom(response);
- EXPECT_TRUE(session_->HasMoreToSync());
- status()->mutable_updates_response()->Clear();
+TEST_F(SyncSessionTest, MoreToDownloadIfDownloadFailed) {
+ // When DownloadUpdatesCommand fails, these should be false.
+ EXPECT_FALSE(status()->server_says_nothing_more_to_download());
+ EXPECT_FALSE(status()->download_updates_succeeded());
+
+ // Download updates has its own loop in the syncer; it shouldn't factor
+ // into HasMoreToSync.
EXPECT_FALSE(session_->HasMoreToSync());
- status()->mutable_updates_response()->CopyFrom(response);
- EXPECT_TRUE(session_->HasMoreToSync());
}
+TEST_F(SyncSessionTest, MoreToDownloadIfGotTimestamp) {
+ // When the server returns a timestamp, that means there's more to download.
+ status()->mutable_updates_response()->mutable_get_updates()
+ ->set_new_timestamp(1000000L);
+ EXPECT_FALSE(status()->server_says_nothing_more_to_download());
+ EXPECT_TRUE(status()->download_updates_succeeded());
+
+ // Download updates has its own loop in the syncer; it shouldn't factor
+ // into HasMoreToSync.
+ EXPECT_FALSE(session_->HasMoreToSync());
+}
+
+TEST_F(SyncSessionTest, MoreToDownloadIfGotNoTimestamp) {
+ // When the server returns a timestamp, that means we're up to date.
+ status()->mutable_updates_response()->mutable_get_updates()
+ ->clear_new_timestamp();
+ EXPECT_TRUE(status()->server_says_nothing_more_to_download());
+ EXPECT_TRUE(status()->download_updates_succeeded());
+
+ // Download updates has its own loop in the syncer; it shouldn't factor
+ // into HasMoreToSync.
+ EXPECT_FALSE(session_->HasMoreToSync());
+}
+
+TEST_F(SyncSessionTest, MoreToDownloadIfGotTimestampAndEntries) {
+ // The actual entry count should not factor into the HasMoreToSync
+ // determination.
+ status()->mutable_updates_response()->mutable_get_updates()->add_entries();
+ status()->mutable_updates_response()->mutable_get_updates()
+ ->set_new_timestamp(1000000L);;
+ EXPECT_FALSE(status()->server_says_nothing_more_to_download());
+ EXPECT_TRUE(status()->download_updates_succeeded());
+
+ // Download updates has its own loop in the syncer; it shouldn't factor
+ // into HasMoreToSync.
+ EXPECT_FALSE(session_->HasMoreToSync());
+}
+
+
TEST_F(SyncSessionTest, MoreToSyncIfConflictsResolved) {
// Conflict resolution happens after get updates and commit,
// so we need to loop back and get updates / commit again now
@@ -153,16 +190,6 @@
EXPECT_TRUE(session_->HasMoreToSync());
}
-TEST_F(SyncSessionTest, MoreToSyncIfTimestampDirty) {
- // If there are more changes on the server that weren't processed during this
- // GetUpdates request, the client should send another GetUpdates request and
- // use new_timestamp as the from_timestamp value within GetUpdatesMessage.
- status()->set_got_new_timestamp();
- status()->update_conflicts_resolved(true);
- EXPECT_TRUE(session_->HasMoreToSync());
-}
-
-
} // namespace
} // namespace sessions
} // namespace browser_sync
diff --git a/chrome/browser/sync/syncable/directory_backing_store.cc b/chrome/browser/sync/syncable/directory_backing_store.cc
index 20c6581..a1cb986 100644
--- a/chrome/browser/sync/syncable/directory_backing_store.cc
+++ b/chrome/browser/sync/syncable/directory_backing_store.cc
@@ -280,7 +280,7 @@
"SET last_sync_timestamp = ?, initial_sync_ended = ?, "
"store_birthday = ?, "
"next_id = ?");
- update.bind_int64(0, info.last_sync_timestamp);
+ update.bind_int64(0, info.last_download_timestamp);
update.bind_bool(1, info.initial_sync_ended);
update.bind_string(2, info.store_birthday);
update.bind_int64(3, info.next_id);
@@ -444,7 +444,7 @@
"store_birthday, next_id, cache_guid "
"FROM share_info");
CHECK(SQLITE_ROW == query.step());
- info->kernel_info.last_sync_timestamp = query.column_int64(0);
+ info->kernel_info.last_download_timestamp = query.column_int64(0);
info->kernel_info.initial_sync_ended = query.column_bool(1);
info->kernel_info.store_birthday = query.column_string(2);
info->kernel_info.next_id = query.column_int64(3);
diff --git a/chrome/browser/sync/syncable/syncable.cc b/chrome/browser/sync/syncable/syncable.cc
index 038f5c83..08a7bf5 100644
--- a/chrome/browser/sync/syncable/syncable.cc
+++ b/chrome/browser/sync/syncable/syncable.cc
@@ -630,16 +630,16 @@
}
}
-int64 Directory::last_sync_timestamp() const {
+int64 Directory::last_download_timestamp() const {
ScopedKernelLock lock(this);
- return kernel_->persisted_info.last_sync_timestamp;
+ return kernel_->persisted_info.last_download_timestamp;
}
-void Directory::set_last_sync_timestamp(int64 timestamp) {
+void Directory::set_last_download_timestamp(int64 timestamp) {
ScopedKernelLock lock(this);
- if (kernel_->persisted_info.last_sync_timestamp == timestamp)
+ if (kernel_->persisted_info.last_download_timestamp == timestamp)
return;
- kernel_->persisted_info.last_sync_timestamp = timestamp;
+ kernel_->persisted_info.last_download_timestamp = timestamp;
kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
}
diff --git a/chrome/browser/sync/syncable/syncable.h b/chrome/browser/sync/syncable/syncable.h
index 7389873..21cd866 100644
--- a/chrome/browser/sync/syncable/syncable.h
+++ b/chrome/browser/sync/syncable/syncable.h
@@ -658,7 +658,7 @@
// for) needs saved across runs of the application.
struct PersistedKernelInfo {
// Last sync timestamp fetched from the server.
- int64 last_sync_timestamp;
+ int64 last_download_timestamp;
// true iff we ever reached the end of the changelog.
bool initial_sync_ended;
// The store birthday we were given by the server. Contents are opaque to
@@ -666,7 +666,7 @@
std::string store_birthday;
// The next local ID that has not been used with this cache-GUID.
int64 next_id;
- PersistedKernelInfo() : last_sync_timestamp(0),
+ PersistedKernelInfo() : last_download_timestamp(0),
initial_sync_ended(false),
next_id(0) {
}
@@ -720,8 +720,8 @@
// The sync timestamp is an index into the list of changes for an account.
// It doesn't actually map to any time scale, it's name is an historical
// anomaly.
- int64 last_sync_timestamp() const;
- void set_last_sync_timestamp(int64 timestamp);
+ int64 last_download_timestamp() const;
+ void set_last_download_timestamp(int64 timestamp);
bool initial_sync_ended() const;
void set_initial_sync_ended(bool value);
diff --git a/chrome/browser/sync/syncable/syncable_unittest.cc b/chrome/browser/sync/syncable/syncable_unittest.cc
index 3d514e0e..dd555c6 100644
--- a/chrome/browser/sync/syncable/syncable_unittest.cc
+++ b/chrome/browser/sync/syncable/syncable_unittest.cc
@@ -833,19 +833,19 @@
}
TEST_F(SyncableDirectoryTest, TestShareInfo) {
- dir_->set_last_sync_timestamp(100);
+ dir_->set_last_download_timestamp(100);
dir_->set_store_birthday("Jan 31st");
{
ReadTransaction trans(dir_.get(), __FILE__, __LINE__);
- EXPECT_EQ(100, dir_->last_sync_timestamp());
+ EXPECT_EQ(100, dir_->last_download_timestamp());
EXPECT_EQ("Jan 31st", dir_->store_birthday());
}
- dir_->set_last_sync_timestamp(200);
+ dir_->set_last_download_timestamp(200);
dir_->set_store_birthday("April 10th");
dir_->SaveChanges();
{
ReadTransaction trans(dir_.get(), __FILE__, __LINE__);
- EXPECT_EQ(200, dir_->last_sync_timestamp());
+ EXPECT_EQ(200, dir_->last_download_timestamp());
EXPECT_EQ("April 10th", dir_->store_birthday());
}
}
diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp
index 124ccb5..c604cdf 100644
--- a/chrome/chrome.gyp
+++ b/chrome/chrome.gyp
@@ -895,6 +895,8 @@
'browser/sync/engine/process_updates_command.h',
'browser/sync/engine/resolve_conflicts_command.cc',
'browser/sync/engine/resolve_conflicts_command.h',
+ 'browser/sync/engine/store_timestamps_command.cc',
+ 'browser/sync/engine/store_timestamps_command.h',
'browser/sync/engine/syncapi.h',
'browser/sync/engine/syncer.cc',
'browser/sync/engine/syncer.h',
diff --git a/chrome/common/ipc_test_sink.h b/chrome/common/ipc_test_sink.h
index ec1e70c..973fc71 100644
--- a/chrome/common/ipc_test_sink.h
+++ b/chrome/common/ipc_test_sink.h
@@ -39,7 +39,7 @@
// test_sink.ClearMessages();
//
// To hook up the sink, all you need to do is call OnMessageReceived when a
-// message is recieved.
+// message is received.
class TestSink {
public:
TestSink();
diff --git a/chrome/common/resource_dispatcher.cc b/chrome/common/resource_dispatcher.cc
index d5fcd897..9df8eaf 100644
--- a/chrome/common/resource_dispatcher.cc
+++ b/chrome/common/resource_dispatcher.cc
@@ -315,7 +315,7 @@
request_info.peer->GetURLForDebugging().possibly_invalid_spec());
request_info.peer->OnUploadProgress(position, size);
- // Acknowlegde reciept
+ // Acknowledge receipt
message_sender()->Send(
new ViewHostMsg_UploadProgress_ACK(message.routing_id(), request_id));
}
diff --git a/chrome/common/webmessageportchannel_impl.cc b/chrome/common/webmessageportchannel_impl.cc
index 86e9a5d..4ff8b55 100644
--- a/chrome/common/webmessageportchannel_impl.cc
+++ b/chrome/common/webmessageportchannel_impl.cc
@@ -152,7 +152,7 @@
void WebMessagePortChannelImpl::QueueMessages() {
// This message port is being sent elsewhere (perhaps to another process).
- // The new endpoint needs to recieve the queued messages, including ones that
+ // The new endpoint needs to receive the queued messages, including ones that
// could still be in-flight. So we tell the browser to queue messages, and it
// sends us an ack, whose receipt we know means that no more messages are
// in-flight. We then send the queued messages to the browser, which prepends
diff --git a/chrome/test/live_sync/profile_sync_service_test_harness.cc b/chrome/test/live_sync/profile_sync_service_test_harness.cc
index 7c4ce652..4bcc273 100644
--- a/chrome/test/live_sync/profile_sync_service_test_harness.cc
+++ b/chrome/test/live_sync/profile_sync_service_test_harness.cc
@@ -16,9 +16,9 @@
using browser_sync::sessions::SyncSessionSnapshot;
-// The default value for min_updates_needed_ when we're not in a call to
-// WaitForUpdatesRecievedAtLeast.
-static const int kMinUpdatesNeededNone = -1;
+// The default value for min_timestamp_needed_ when we're not in the
+// WAITING_FOR_UPDATES state.
+static const int kMinTimestampNeededNone = -1;
static const ProfileSyncService::Status kInvalidStatus = {};
@@ -87,7 +87,7 @@
: wait_state_(WAITING_FOR_INITIAL_CALLBACK), profile_(p), service_(NULL),
last_status_(kInvalidStatus),
last_timestamp_(0),
- min_timestamp_needed_(kMinUpdatesNeededNone),
+ min_timestamp_needed_(kMinTimestampNeededNone),
username_(username), password_(password) {
// Ensure the profile has enough prefs registered for use by sync.
if (!p->GetPrefs()->FindPreference(prefs::kAcceptLanguages))
