diff options
author | nick@chromium.org <nick@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-03-24 00:27:18 +0000 |
---|---|---|
committer | nick@chromium.org <nick@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-03-24 00:27:18 +0000 |
commit | ad76da7f13a8314f24e9acf9f43aae4fb6dfa6db (patch) | |
tree | 1f66cd8d92a30d9da23d6e23231b74d75892d252 | |
parent | cec1b8d4524bd0ce53a7b08b4263c788a5b6d72b (diff) | |
download | chromium_src-ad76da7f13a8314f24e9acf9f43aae4fb6dfa6db.zip chromium_src-ad76da7f13a8314f24e9acf9f43aae4fb6dfa6db.tar.gz chromium_src-ad76da7f13a8314f24e9acf9f43aae4fb6dfa6db.tar.bz2 |
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
29 files changed, 222 insertions, 140 deletions
diff --git a/chrome/browser/cocoa/preferences_window_controller.h b/chrome/browser/cocoa/preferences_window_controller.h index 33fce3a..03a8ca5 100644 --- a/chrome/browser/cocoa/preferences_window_controller.h +++ b/chrome/browser/cocoa/preferences_window_controller.h @@ -24,7 +24,7 @@ class ProfileSyncService; // 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..45b5304 100644 --- a/chrome/browser/extensions/extension_message_service.cc +++ b/chrome/browser/extensions/extension_message_service.cc @@ -328,7 +328,7 @@ bool ExtensionMessageService::OpenChannelOnUIThreadImpl( 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 @@ class FFUnitTestDecryptorProxy { #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 @@ void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) { 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 @@ void DownloadUpdatesCommand::ExecuteImpl(SyncSession* session) { 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 03af303..77a1272 100644 --- a/chrome/browser/sync/engine/process_updates_command.cc +++ b/chrome/browser/sync/engine/process_updates_command.cc @@ -25,6 +25,15 @@ using sessions::StatusController; 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 @@ void ProcessUpdatesCommand::ModelChangingExecuteImpl(SyncSession* session) { 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 @@ void ProcessUpdatesCommand::ModelChangingExecuteImpl(SyncSession* session) { ++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 @@ class ProcessUpdatesCommand : public ModelChangingSyncerCommand { 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..d9f48bb 100644 --- a/chrome/browser/sync/engine/syncapi.cc +++ b/chrome/browser/sync/engine/syncapi.cc @@ -1828,10 +1828,6 @@ void SyncManager::SyncInternal::HandleAuthWatcherEvent( 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..26b98b3 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 @@ void Syncer::SyncShare(sessions::SyncSession* session, 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 7daed36..e6be6d2 100644 --- a/chrome/browser/sync/engine/syncer.h +++ b/chrome/browser/sync/engine/syncer.h @@ -47,6 +47,7 @@ enum SyncerStep { 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 3b57618..e594542 100644 --- a/chrome/browser/sync/engine/syncer_end_command.cc +++ b/chrome/browser/sync/engine/syncer_end_command.cc @@ -18,10 +18,9 @@ void SyncerEndCommand::ExecuteImpl(sessions::SyncSession* session) { 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 @@ void VerifyUpdatesCommand::ModelChangingExecuteImpl( 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 28e89a9..9c096da 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 @@ class AsyncNetworkAliveLinux : public AsyncNetworkAlive { 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 @@ class UpdateProgress { 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 @@ class UpdateProgress { 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 @@ struct SyncCycleControlParameters { // 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..cfd6fa8 100644 --- a/chrome/browser/sync/sessions/status_controller.cc +++ b/chrome/browser/sync/sessions/status_controller.cc @@ -81,10 +81,6 @@ void StatusController::set_num_consecutive_errors(int value) { 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..87d30df 100644 --- a/chrome/browser/sync/sessions/status_controller.h +++ b/chrome/browser/sync/sessions/status_controller.h @@ -82,7 +82,7 @@ class StatusController { 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 @@ class StatusController { 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 @@ class StatusController { 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 @@ class StatusController { 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 24c4025..d1ee710 100644 --- a/chrome/browser/sync/sessions/status_controller_unittest.cc +++ b/chrome/browser/sync/sessions/status_controller_unittest.cc @@ -103,8 +103,6 @@ TEST_F(StatusControllerTest, StaysClean) { 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, HasConflictingUpdates) { 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 @@ TEST_F(StatusControllerTest, Unrestricted) { 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 @@ bool SyncSession::HasMoreToSync() const { 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..43f9769 100644 --- a/chrome/browser/sync/sessions/sync_session_unittest.cc +++ b/chrome/browser/sync/sessions/sync_session_unittest.cc @@ -133,18 +133,55 @@ TEST_F(SyncSessionTest, MoreToSyncIfConflictSetsBuilt) { 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()); +} + +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()); - status()->mutable_updates_response()->CopyFrom(response); - EXPECT_TRUE(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 @@ TEST_F(SyncSessionTest, MoreToSyncIfConflictsResolved) { 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 @@ bool DirectoryBackingStore::SaveChanges( "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 @@ void DirectoryBackingStore::LoadInfo(Directory::KernelLoadInfo* info) { "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 @@ void Directory::HandleSaveChangesFailure(const SaveChangesSnapshot& snapshot) { } } -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 @@ class Directory { // 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 @@ class Directory { 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 @@ class Directory { // 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, TestCaseChangeRename) { } 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 @@ namespace IPC { // 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 d5fcd89..9df8eaf 100644 --- a/chrome/common/resource_dispatcher.cc +++ b/chrome/common/resource_dispatcher.cc @@ -315,7 +315,7 @@ void ResourceDispatcher::OnUploadProgress( 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::Entangle( 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 7c4ce65..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 @@ ProfileSyncServiceTestHarness::ProfileSyncServiceTestHarness( : 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)) |