[Sync] Fake server now updates each model type's progress markers independently.

components/sync/test/fake_server/fake_server.cc

 // Copyright 2014 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 "components/sync/test/fake_server/fake_server.h"
 
 #include <algorithm>
 #include <limits>
 #include <set>
 #include <utility>
 
 #include "base/guid.h"
 #include "base/logging.h"
+#include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/lock.h"
 #include "components/sync/test/fake_server/bookmark_entity.h"
 #include "components/sync/test/fake_server/permanent_entity.h"
 #include "components/sync/test/fake_server/tombstone_entity.h"
 #include "components/sync/test/fake_server/unique_client_entity.h"
 #include "net/base/net_errors.h"
@@ skipping 21 matching lines @@
 
 // Properties of the other bookmarks permanent folder.
 static const char kOtherBookmarksFolderServerTag[] = "other_bookmarks";
 static const char kOtherBookmarksFolderName[] = "Other Bookmarks";
 
 // Properties of the synced bookmarks permanent folder.
 static const char kSyncedBookmarksFolderServerTag[] = "synced_bookmarks";
 static const char kSyncedBookmarksFolderName[] = "Synced Bookmarks";
 
 // A filter used during GetUpdates calls to determine what information to
-// send back to the client. There is a 1:1 correspondence between any given
-// GetUpdates call and an UpdateSieve instance.
+// send back to the client, filtering out old entities and tracking versions to
+// use in response progress markers. Note that only the GetUpdatesMessage's
+// from_progress_marker is used to determine this, legacy fields are ignored.

[maxbogue, 2016/12/09 21:10:15]

the nit of all nits: s/,/;/

[skym, 2016/12/09 21:51:50]

Done.

 class UpdateSieve {
  public:
+  explicit UpdateSieve(const sync_pb::GetUpdatesMessage& message)
+      : UpdateSieve(MessageToVersionMap(message)) {}
   ~UpdateSieve() {}
 
-  // Factory method for creating an UpdateSieve.
-  static std::unique_ptr<UpdateSieve> Create(
-      const sync_pb::GetUpdatesMessage& get_updates_message);
-
-  // Sets the progress markers in |get_updates_response| given the progress
-  // markers from the original GetUpdatesMessage and |new_version| (the latest
-  // version in the entries sent back).
-  void UpdateProgressMarkers(
-      int64_t new_version,
+  // Sets the progress markers in |get_updates_response| based on the highest
+  // version between request progress markers and response entities.
+  void SetProgressMarkers(
       sync_pb::GetUpdatesResponse* get_updates_response) const {
-    ModelTypeToVersionMap::const_iterator it;
-    for (it = request_from_version_.begin(); it != request_from_version_.end();
-         ++it) {
+    for (auto& kv : response_version_map_) {

[maxbogue, 2016/12/09 21:10:15]

const auto& perhaps?

[skym, 2016/12/09 21:51:50]

Done.

       sync_pb::DataTypeProgressMarker* new_marker =
           get_updates_response->add_new_progress_marker();
       new_marker->set_data_type_id(
-          GetSpecificsFieldNumberFromModelType(it->first));
-
-      int64_t version = std::max(new_version, it->second);
-      new_marker->set_token(base::Int64ToString(version));
+          GetSpecificsFieldNumberFromModelType(kv.first));
+      new_marker->set_token(base::Int64ToString(kv.second));
     }
   }
 
   // Determines whether the server should send an |entity| to the client as
-  // part of a GetUpdatesResponse.
-  bool ClientWantsItem(const FakeServerEntity& entity) const {
+  // part of a GetUpdatesResponse. Update internal tracking of max versions as a
+  // side effect which will later be used to set response progress markers.
+  bool ClientWantsItem(const FakeServerEntity& entity) {
     int64_t version = entity.GetVersion();
-    if (version <= min_version_) {
-      return false;
-    } else if (entity.IsDeleted()) {
-      return true;
-    }
-
-    ModelTypeToVersionMap::const_iterator it =
-        request_from_version_.find(entity.model_type());
-
-    return it == request_from_version_.end() ? false : it->second < version;
+    ModelType type = entity.model_type();
+    response_version_map_[type] =
+        std::max(response_version_map_[type], version);
+    auto it = request_version_map_.find(type);
+    return it == request_version_map_.end() ? false : it->second < version;
   }
 
-  // Returns the minimum version seen across all types.
-  int64_t GetMinVersion() const { return min_version_; }
-
  private:
   typedef std::map<ModelType, int64_t> ModelTypeToVersionMap;
 
-  // Creates an UpdateSieve.
-  UpdateSieve(const ModelTypeToVersionMap request_from_version,
-              const int64_t min_version)
-      : request_from_version_(request_from_version),
-        min_version_(min_version) {}
+  static UpdateSieve::ModelTypeToVersionMap MessageToVersionMap(

[maxbogue, 2016/12/09 21:10:15]

you shouldn't need the UpdateSieve:: bit right?

[skym, 2016/12/09 21:51:50]

Does not compile w/o it. I think it has to do with the function being static.

+      const sync_pb::GetUpdatesMessage& get_updates_message) {
+    CHECK_GT(get_updates_message.from_progress_marker_size(), 0)
+        << "A GetUpdates request must have at least one progress marker.";
+    ModelTypeToVersionMap request_version_map;
 
-  // Maps data type IDs to the latest version seen for that type.
-  const ModelTypeToVersionMap request_from_version_;
+    for (int i = 0; i < get_updates_message.from_progress_marker_size(); i++) {
+      sync_pb::DataTypeProgressMarker marker =
+          get_updates_message.from_progress_marker(i);
 
-  // The minimum version seen among all data types.
-  const int min_version_;
-};
+      int64_t version = 0;
+      // Let the version remain zero if there is no token or an empty token (the
+      // first request for this type).
+      if (marker.has_token() && !marker.token().empty()) {
+        bool parsed = base::StringToInt64(marker.token(), &version);
+        CHECK(parsed) << "Unable to parse progress marker token.";
+      }
 
-std::unique_ptr<UpdateSieve> UpdateSieve::Create(
-    const sync_pb::GetUpdatesMessage& get_updates_message) {
-  CHECK_GT(get_updates_message.from_progress_marker_size(), 0)
-      << "A GetUpdates request must have at least one progress marker.";
-
-  UpdateSieve::ModelTypeToVersionMap request_from_version;
-  int64_t min_version = std::numeric_limits<int64_t>::max();
-  for (int i = 0; i < get_updates_message.from_progress_marker_size(); i++) {
-    sync_pb::DataTypeProgressMarker marker =
-        get_updates_message.from_progress_marker(i);
-
-    int64_t version = 0;
-    // Let the version remain zero if there is no token or an empty token (the
-    // first request for this type).
-    if (marker.has_token() && !marker.token().empty()) {
-      bool parsed = base::StringToInt64(marker.token(), &version);
-      CHECK(parsed) << "Unable to parse progress marker token.";
+      ModelType model_type =
+          syncer::GetModelTypeFromSpecificsFieldNumber(marker.data_type_id());
+      request_version_map[model_type] = version;

[maxbogue, 2016/12/09 21:10:15]

Consider adding a DCHECK above this that model_type doesn't already exist in the
map.

[skym, 2016/12/09 21:51:50]

Done.

     }
-
-    ModelType model_type =
-        syncer::GetModelTypeFromSpecificsFieldNumber(marker.data_type_id());
-    request_from_version[model_type] = version;
-
-    if (version < min_version)
-      min_version = version;
+    return request_version_map;
   }
 
-  return std::unique_ptr<UpdateSieve>(
-      new UpdateSieve(request_from_version, min_version));
-}
+  explicit UpdateSieve(const ModelTypeToVersionMap request_version_map)
+      : request_version_map_(request_version_map),
+        response_version_map_(request_version_map) {}
+
+  // The largest versions the client has seen before this request, and is used
+  // to filter entities to send back to clients. The values in this map are not
+  // updated after being initially set. The presence of a type in this map is a
+  // proxy for the desire to receive results about this type.
+  const ModelTypeToVersionMap request_version_map_;
+
+  // The largest versions seen between client and server, ultimately used to
+  // send progress markers back to the client.
+  ModelTypeToVersionMap response_version_map_;
+};
 
 // Returns whether |entity| is deleted or permanent.
 bool IsDeletedOrPermanent(const FakeServerEntity& entity) {
   return entity.IsDeleted() || entity.IsPermanent();
 }
 
 }  // namespace
 
 FakeServer::FakeServer()
     : version_(0),
@@ skipping 172 matching lines @@
   return true;
 }
 
 bool FakeServer::HandleGetUpdatesRequest(
     const sync_pb::GetUpdatesMessage& get_updates,
     sync_pb::GetUpdatesResponse* response) {
   // TODO(pvalenzuela): Implement batching instead of sending all information
   // at once.
   response->set_changes_remaining(0);
 
-  std::unique_ptr<UpdateSieve> sieve = UpdateSieve::Create(get_updates);
+  auto sieve = base::MakeUnique<UpdateSieve>(get_updates);
 
   // This folder is called "Synced Bookmarks" by sync and is renamed
   // "Mobile Bookmarks" by the mobile client UIs.
   if (get_updates.create_mobile_bookmarks_folder() &&
       !CreatePermanentBookmarkFolder(kSyncedBookmarksFolderServerTag,
                                      kSyncedBookmarksFolderName)) {
     return false;
   }
 
   bool send_encryption_keys_based_on_nigori = false;
-  int64_t max_response_version = 0;
   for (EntityMap::const_iterator it = entities_.begin(); it != entities_.end();
        ++it) {
     const FakeServerEntity& entity = *it->second;
     if (sieve->ClientWantsItem(entity)) {
       sync_pb::SyncEntity* response_entity = response->add_entries();
       entity.SerializeAsProto(response_entity);
 
-      max_response_version =
-          std::max(max_response_version, response_entity->version());
-
       if (entity.model_type() == syncer::NIGORI) {
         send_encryption_keys_based_on_nigori =
             response_entity->specifics().nigori().passphrase_type() ==
             sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE;
       }
     }
   }
 
   if (send_encryption_keys_based_on_nigori ||
       get_updates.need_encryption_key()) {
     for (vector<string>::iterator it = keystore_keys_.begin();
          it != keystore_keys_.end(); ++it) {
       response->add_encryption_keys(*it);
     }
   }
 
-  sieve->UpdateProgressMarkers(max_response_version, response);
+  sieve->SetProgressMarkers(response);
   return true;
 }
 
 string FakeServer::CommitEntity(
     const sync_pb::SyncEntity& client_entity,
     sync_pb::CommitResponse_EntryResponse* entry_response,
     const string& client_guid,
     const string& parent_id) {
   if (client_entity.version() == 0 && client_entity.deleted()) {
     return string();
@@ skipping 330 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   return weak_ptr_factory_.GetWeakPtr();
 }
 
 std::string FakeServer::GetStoreBirthday() const {
   DCHECK(thread_checker_.CalledOnValidThread());
   return base::Int64ToString(store_birthday_);
 }
 
 }  // namespace fake_server