Fix sync cycle snapshot progress markers for USS

Instead of reading progress markers from the directory, use the
UpdateHandler interface, which is supported in USS as well.

Some optional but somewhat related test changes are bundled in this
patch, although they are not required to pass tests.

Bug: 836289
Change-Id: I5a62f27a92aa089a2aa19d4a50aa680c38387c3f
Reviewed-on: https://chromium-review.googlesource.com/1033752
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554730}

chrome/browser/sync/test/integration/updated_progress_marker_checker.cc

@@ -26,7 +26,17 @@ bool UpdatedProgressMarkerChecker::IsExitConditionSatisfied() {
   }
 
   const syncer::SyncCycleSnapshot& snap = service()->GetLastCycleSnapshot();
-  return snap.model_neutral_state().num_successful_commits == 0 &&
+  // Assuming the lack of ongoing remote changes, the progress marker can be
+  // considered updated when:
+  // 1. Progress markers are non-empty (which discards the default value for
+  //    GetLastCycleSnapshot() prior to the first sync cycle).
+  // 2. Our last sync cycle committed no changes (because commits are followed
+  //    by the test-only 'self-notify' cycle).
+  // 3. Sync is still active (e.g. no failures).
+  // 4. No pending local changes (which will ultimately generate new progress
+  //    markers once submitted to the server).
+  return !snap.download_progress_markers().empty() &&
+         snap.model_neutral_state().num_successful_commits == 0 &&
          service()->IsSyncActive() && !has_unsynced_items_.value();
 }
 

components/sync/engine_impl/cycle/sync_cycle.cc

@@ -8,6 +8,7 @@
 #include <iterator>
 
 #include "base/logging.h"
+#include "components/sync/engine_impl/update_handler.h"
 #include "components/sync/syncable/directory.h"
 
 namespace syncer {
@@ -25,14 +26,23 @@ SyncCycleSnapshot SyncCycle::TakeSnapshot() const {
 
 SyncCycleSnapshot SyncCycle::TakeSnapshotWithOrigin(
     sync_pb::SyncEnums::GetUpdatesOrigin get_updates_origin) const {
-  syncable::Directory* dir = context_->directory();
-
   ProgressMarkerMap download_progress_markers;
   for (int i = FIRST_REAL_MODEL_TYPE; i < MODEL_TYPE_COUNT; ++i) {
     ModelType type(ModelTypeFromInt(i));
-    dir->GetDownloadProgressAsString(type, &download_progress_markers[type]);
+    const UpdateHandler* update_handler =
+        context_->model_type_registry()->GetUpdateHandler(type);
+    if (update_handler == nullptr) {
+      continue;
+    }
+    sync_pb::DataTypeProgressMarker progress_marker;
+    update_handler->GetDownloadProgress(&progress_marker);
+    download_progress_markers[type] = progress_marker.SerializeAsString();
   }
 
+  // TODO(mastiz): Remove dependency to directory, since it most likely hides
+  // an issue with USS types.
+  syncable::Directory* dir = context_->directory();
+
   std::vector<int> num_entries_by_type(MODEL_TYPE_COUNT, 0);
   std::vector<int> num_to_delete_entries_by_type(MODEL_TYPE_COUNT, 0);
   dir->CollectMetaHandleCounts(&num_entries_by_type,

components/sync/engine_impl/model_type_registry.cc

@@ -253,6 +253,11 @@ ModelTypeSet ModelTypeRegistry::GetInitialSyncDoneNonBlockingTypes() const {
   return types;
 }
 
+const UpdateHandler* ModelTypeRegistry::GetUpdateHandler(ModelType type) const {
+  UpdateHandlerMap::const_iterator it = update_handler_map_.find(type);
+  return it == update_handler_map_.end() ? nullptr : it->second;
+}
+
 UpdateHandlerMap* ModelTypeRegistry::update_handler_map() {
   return &update_handler_map_;
 }

components/sync/engine_impl/model_type_registry.h

@@ -94,6 +94,9 @@ class ModelTypeRegistry : public ModelTypeConnector,
   // Returns the set of non-blocking types with initial sync done.
   ModelTypeSet GetInitialSyncDoneNonBlockingTypes() const;
 
+  // Returns the update handler for |type|.
+  const UpdateHandler* GetUpdateHandler(ModelType type) const;
+
   // Simple getters.
   UpdateHandlerMap* update_handler_map();
   CommitContributorMap* commit_contributor_map();

components/sync/syncable/directory.cc

@@ -820,13 +820,6 @@ void Directory::GetDownloadProgress(
       kernel_->persisted_info.download_progress[model_type]);
 }
 
-void Directory::GetDownloadProgressAsString(ModelType model_type,
-                                            std::string* value_out) const {
-  ScopedKernelLock lock(this);
-  kernel_->persisted_info.download_progress[model_type].SerializeToString(
-      value_out);
-}
-
 size_t Directory::GetEntriesCount() const {
   ScopedKernelLock lock(this);
   return kernel_->metahandles_map.size();

components/sync/syncable/directory.h

@@ -265,8 +265,6 @@ class Directory {
   // to indicate the continuation state of the next GetUpdates operation.
   void GetDownloadProgress(ModelType type,
                            sync_pb::DataTypeProgressMarker* value_out) const;
-  void GetDownloadProgressAsString(ModelType type,
-                                   std::string* value_out) const;
   void SetDownloadProgress(ModelType type,
                            const sync_pb::DataTypeProgressMarker& value);
   bool HasEmptyDownloadProgress(ModelType type) const;