sync: Inject sync/'s dependency on invalidations

Defines an InvalidationInterface class and uses it to break any direct
dependencies from sync code on syncer::Invalidation.  Despite its name,
syncer::Invalidation should belong solely to the invalidations
component, which code in the sync/ directory should not depend on.

Changes the interface in the sync engine from copying
syncer::Invalidation to managing scoped_ptr<InvalidationInterface>.
This change in memory management was required to support the use of an
abstract interface.

Removes the DroppedInvaldiationTracker.  This class was previously only
used by sync.  The small benefit provided by this class is outweighed by
the amount of glue code it would take to maintain it.

Changes tests to conform to the new interface.  Adds some test-only
implementations of InvalidationInterface and some associated classes.

BUG=259559

Review URL: https://codereview.chromium.org/322333004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@281884 0039d316-1c4b-4281-b951-d872f2087c98

13 files changed, 121 insertions, 57 deletions

diff --git a/sync/internal_api/public/base/invalidation.cc b/sync/internal_api/public/base/invalidation.cc
index ff7a5a7..e6a64be 100644
--- a/sync/internal_api/public/base/invalidation.cc
+++ b/sync/internal_api/public/base/invalidation.cc
@@ -11,7 +11,6 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/values.h"
 #include "sync/notifier/ack_handler.h"
-#include "sync/notifier/dropped_invalidation_tracker.h"
 #include "sync/notifier/invalidation_util.h"
 
 namespace syncer {
@@ -43,6 +42,15 @@ Invalidation Invalidation::InitFromDroppedInvalidation(
                       std::string(), dropped.ack_handle_);
 }
 
+Invalidation::Invalidation(const Invalidation& other)
+  : id_(other.id_),
+    is_unknown_version_(other.is_unknown_version_),
+    version_(other.version_),
+    payload_(other.payload_),
+    ack_handle_(other.ack_handle_),
+    ack_handler_(other.ack_handler_) {
+}
+
 scoped_ptr<Invalidation> Invalidation::InitFromValue(
     const base::DictionaryValue& value) {
   invalidation::ObjectId id;
@@ -128,9 +136,7 @@ void Invalidation::Acknowledge() const {
   }
 }
 
-void Invalidation::Drop(DroppedInvalidationTracker* tracker) const {
-  DCHECK(tracker->object_id() == object_id());
-  tracker->RecordDropEvent(ack_handler_, ack_handle_);
+void Invalidation::Drop() {
   if (SupportsAcknowledgement()) {
     ack_handler_.Call(FROM_HERE,
                       &AckHandler::Drop,
diff --git a/sync/internal_api/public/base/invalidation.h b/sync/internal_api/public/base/invalidation.h
index cf26112..309eb7d 100644
--- a/sync/internal_api/public/base/invalidation.h
+++ b/sync/internal_api/public/base/invalidation.h
@@ -35,6 +35,7 @@ class SYNC_EXPORT Invalidation {
   static scoped_ptr<Invalidation> InitFromValue(
       const base::DictionaryValue& value);
 
+  Invalidation(const Invalidation& other);
   ~Invalidation();
 
   // Compares two invalidations.  The comparison ignores ack-tracking state.
@@ -84,12 +85,9 @@ class SYNC_EXPORT Invalidation {
   // invalidations in order to allow the ack tracker to drop the invalidation,
   // too.
   //
-  // The drop record will be tracked by the specified
-  // DroppedInvalidationTracker.  The caller should hang on to this tracker.  It
-  // will need to use it when it recovers from this drop event, or if it needs
-  // to record another drop event for the same ObjectID.  Refer to the
-  // documentation of DroppedInvalidationTracker for more details.
-  void Drop(DroppedInvalidationTracker* tracker) const;
+  // To indicate recovery from a drop event, the client should call
+  // Acknowledge() on the most recently dropped inavlidation.
+  void Drop();
 
   scoped_ptr<base::DictionaryValue> ToValue() const;
   std::string ToString() const;
diff --git a/sync/internal_api/public/base/invalidation_interface.cc b/sync/internal_api/public/base/invalidation_interface.cc
new file mode 100644
index 0000000..0284170
--- /dev/null
+++ b/sync/internal_api/public/base/invalidation_interface.cc
@@ -0,0 +1,29 @@
+// 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 "sync/internal_api/public/base/invalidation_interface.h"
+
+namespace syncer {
+
+bool InvalidationInterface::LessThanByVersion(const InvalidationInterface& a,
+                                              const InvalidationInterface& b) {
+  if (a.IsUnknownVersion() && !b.IsUnknownVersion())
+    return true;
+
+  if (!a.IsUnknownVersion() && b.IsUnknownVersion())
+    return false;
+
+  if (a.IsUnknownVersion() && b.IsUnknownVersion())
+    return false;
+
+  return a.GetVersion() < b.GetVersion();
+}
+
+InvalidationInterface::InvalidationInterface() {
+}
+
+InvalidationInterface::~InvalidationInterface() {
+}
+
+}  // namespace syncer
diff --git a/sync/internal_api/public/base/invalidation_interface.h b/sync/internal_api/public/base/invalidation_interface.h
new file mode 100644
index 0000000..d7dbbe8
--- /dev/null
+++ b/sync/internal_api/public/base/invalidation_interface.h
@@ -0,0 +1,55 @@
+// 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.
+
+#ifndef SYNC_INTERNAL_API_PUBLIC_BASE_INVALIDATION_INTERFACE_H_
+#define SYNC_INTERNAL_API_PUBLIC_BASE_INVALIDATION_INTERFACE_H_
+
+#include <string>
+
+#include "base/basictypes.h"
+#include "sync/base/sync_export.h"
+
+namespace syncer {
+
+// An interface that wraps sync's interactions with the component that provides
+// it with invalidations.
+class SYNC_EXPORT InvalidationInterface {
+ public:
+  // Orders invalidations based on version number and IsUnknownVersion().
+  static bool LessThanByVersion(const InvalidationInterface& a,
+                                const InvalidationInterface& b);
+
+  InvalidationInterface();
+  virtual ~InvalidationInterface();
+
+  // Returns true if this is an 'unknown version' invalidation.
+  // Such invalidations have no valid payload or version number.
+  virtual bool IsUnknownVersion() const = 0;
+
+  // Returns the payload of this item.
+  // DCHECKs if this is an unknown version invalidation.
+  virtual const std::string& GetPayload() const = 0;
+
+  // Retursn the version of this item.
+  // DCHECKs if this is an unknown version invalidation.
+  //
+  // It is preferable to use the LessThan() function, which handles unknown
+  // versions properly, rather than this function.
+  virtual int64 GetVersion() const = 0;
+
+  // This function will be called when the invalidation has been handled
+  // successfully.
+  virtual void Acknowledge() = 0;
+
+  // This function should be called if a lack of buffer space required that we
+  // drop this invalidation.
+  //
+  // To indicate recovery from a drop event, the receiver of this invalidation
+  // will call Acknowledge() on the most recently dropped invalidation.
+  virtual void Drop() = 0;
+};
+
+}  // namespace syncer
+
+#endif
diff --git a/sync/internal_api/public/base/model_type_test_util.cc b/sync/internal_api/public/base/model_type_test_util.cc
index 4876962..5893922 100644
--- a/sync/internal_api/public/base/model_type_test_util.cc
+++ b/sync/internal_api/public/base/model_type_test_util.cc
@@ -3,7 +3,6 @@
 // found in the LICENSE file.
 
 #include "sync/internal_api/public/base/model_type_test_util.h"
-#include "sync/internal_api/public/base/ack_handle.h"
 
 namespace syncer {
 
diff --git a/sync/internal_api/public/sync_manager.h b/sync/internal_api/public/sync_manager.h
index 073b8ed..b8cc50d 100644
--- a/sync/internal_api/public/sync_manager.h
+++ b/sync/internal_api/public/sync_manager.h
@@ -12,11 +12,13 @@
 #include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/scoped_vector.h"
 #include "base/task_runner.h"
 #include "base/threading/thread_checker.h"
 #include "google_apis/gaia/oauth2_token_service.h"
 #include "sync/base/sync_export.h"
+#include "sync/internal_api/public/base/invalidation_interface.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/change_record.h"
 #include "sync/internal_api/public/configure_reason.h"
@@ -89,7 +91,7 @@ struct SYNC_EXPORT SyncCredentials {
 //
 // Unless stated otherwise, all methods of SyncManager should be called on the
 // same thread.
-class SYNC_EXPORT SyncManager : public syncer::InvalidationHandler {
+class SYNC_EXPORT SyncManager {
  public:
   // An interface the embedding application implements to be notified
   // on change events.  Note that these methods may be called on *any*
@@ -314,7 +316,8 @@ class SYNC_EXPORT SyncManager : public syncer::InvalidationHandler {
 
   // Inform the syncer that its cached information about a type is obsolete.
   virtual void OnIncomingInvalidation(
-      const ObjectIdInvalidationMap& invalidation_map) = 0;
+      syncer::ModelType type,
+      scoped_ptr<syncer::InvalidationInterface> invalidation) = 0;
 
   // Adds a listener to be notified of sync events.
   // NOTE: It is OK (in fact, it's probably a good idea) to call this before
diff --git a/sync/internal_api/public/test/fake_sync_manager.h b/sync/internal_api/public/test/fake_sync_manager.h
index df14a52..f4ea19a 100644
--- a/sync/internal_api/public/test/fake_sync_manager.h
+++ b/sync/internal_api/public/test/fake_sync_manager.h
@@ -63,14 +63,12 @@ class FakeSyncManager : public SyncManager {
 
   // Posts a method to invalidate the given IDs on the sync thread.
   virtual void OnIncomingInvalidation(
-      const ObjectIdInvalidationMap& invalidation_map) OVERRIDE;
+      syncer::ModelType type,
+      scoped_ptr<InvalidationInterface> interface) OVERRIDE;
 
   // Posts a method to update the invalidator state on the sync thread.
   virtual void OnInvalidatorStateChange(InvalidatorState state) OVERRIDE;
 
-  // Returns this class name for logging purposes.
-  virtual std::string GetOwnerName() const OVERRIDE;
-
   // Block until the sync thread has finished processing any pending messages.
   void WaitForSyncThread();
 
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index 0e3caf3..d0ae309 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -20,6 +20,7 @@
 #include "sync/engine/syncer_types.h"
 #include "sync/internal_api/change_reorder_buffer.h"
 #include "sync/internal_api/public/base/cancelation_signal.h"
+#include "sync/internal_api/public/base/invalidation_interface.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/base_node.h"
 #include "sync/internal_api/public/configure_reason.h"
@@ -979,30 +980,17 @@ void SyncManagerImpl::OnInvalidatorStateChange(InvalidatorState state) {
 }
 
 void SyncManagerImpl::OnIncomingInvalidation(
-    const ObjectIdInvalidationMap& invalidation_map) {
+    syncer::ModelType type,
+    scoped_ptr<InvalidationInterface> invalidation) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // We should never receive IDs from non-sync objects.
-  ObjectIdSet ids = invalidation_map.GetObjectIds();
-  for (ObjectIdSet::const_iterator it = ids.begin(); it != ids.end(); ++it) {
-    ModelType type;
-    if (!ObjectIdToRealModelType(*it, &type)) {
-      DLOG(WARNING) << "Notification has invalid id: " << ObjectIdToString(*it);
-    }
-  }
-
-  if (invalidation_map.Empty()) {
-    LOG(WARNING) << "Sync received invalidation without any type information.";
-  } else {
-    scheduler_->ScheduleInvalidationNudge(
-        TimeDelta::FromMilliseconds(kSyncSchedulerDelayMsec),
-        invalidation_map, FROM_HERE);
-    debug_info_event_listener_.OnIncomingNotification(invalidation_map);
-  }
+  scheduler_->ScheduleInvalidationNudge(
+      TimeDelta::FromMilliseconds(kSyncSchedulerDelayMsec),
+      type,
+      invalidation.Pass(),
+      FROM_HERE);
 }
 
-std::string SyncManagerImpl::GetOwnerName() const { return "SyncManagerImpl"; }
-
 void SyncManagerImpl::RefreshTypes(ModelTypeSet types) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (types.Empty()) {
diff --git a/sync/internal_api/sync_manager_impl.h b/sync/internal_api/sync_manager_impl.h
index 2ca5fd6..7efde2d 100644
--- a/sync/internal_api/sync_manager_impl.h
+++ b/sync/internal_api/sync_manager_impl.h
@@ -104,8 +104,8 @@ class SYNC_EXPORT_PRIVATE SyncManagerImpl :
       const base::Closure& retry_task) OVERRIDE;
   virtual void OnInvalidatorStateChange(InvalidatorState state) OVERRIDE;
   virtual void OnIncomingInvalidation(
-      const ObjectIdInvalidationMap& invalidation_map) OVERRIDE;
-  virtual std::string GetOwnerName() const OVERRIDE;
+      syncer::ModelType type,
+      scoped_ptr<InvalidationInterface> invalidation) OVERRIDE;
   virtual void AddObserver(SyncManager::Observer* observer) OVERRIDE;
   virtual void RemoveObserver(SyncManager::Observer* observer) OVERRIDE;
   virtual SyncStatus GetDetailedStatus() const OVERRIDE;
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index 3fd4c5d..b4b5f87 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -983,14 +983,6 @@ class SyncManagerTest : public testing::Test,
     sync_manager_.OnInvalidatorStateChange(state);
   }
 
-  void TriggerOnIncomingNotificationForTest(ModelTypeSet model_types) {
-    DCHECK(sync_manager_.thread_checker_.CalledOnValidThread());
-    ObjectIdSet id_set = ModelTypeSetToObjectIdSet(model_types);
-    ObjectIdInvalidationMap invalidation_map =
-        ObjectIdInvalidationMap::InvalidateAll(id_set);
-    sync_manager_.OnIncomingInvalidation(invalidation_map);
-  }
-
   void SetProgressMarkerForType(ModelType type, bool set) {
     if (set) {
       sync_pb::DataTypeProgressMarker marker;
diff --git a/sync/internal_api/sync_rollback_manager_base.cc b/sync/internal_api/sync_rollback_manager_base.cc
index 28afb8b..b7d1fef 100644
--- a/sync/internal_api/sync_rollback_manager_base.cc
+++ b/sync/internal_api/sync_rollback_manager_base.cc
@@ -133,7 +133,8 @@ void SyncRollbackManagerBase::OnInvalidatorStateChange(InvalidatorState state) {
 }
 
 void SyncRollbackManagerBase::OnIncomingInvalidation(
-      const ObjectIdInvalidationMap& invalidation_map) {
+    syncer::ModelType type,
+    scoped_ptr<InvalidationInterface> invalidation) {
   NOTREACHED();
 }
 
@@ -233,10 +234,6 @@ void SyncRollbackManagerBase::NotifyInitializationFailure() {
           false, ModelTypeSet()));
 }
 
-std::string SyncRollbackManagerBase::GetOwnerName() const {
-  return "";
-}
-
 syncer::SyncContextProxy* SyncRollbackManagerBase::GetSyncContextProxy() {
   return NULL;
 }
diff --git a/sync/internal_api/sync_rollback_manager_base.h b/sync/internal_api/sync_rollback_manager_base.h
index 8d32d7d..e18535f 100644
--- a/sync/internal_api/sync_rollback_manager_base.h
+++ b/sync/internal_api/sync_rollback_manager_base.h
@@ -73,7 +73,8 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase :
       const base::Closure& retry_task) OVERRIDE;
   virtual void OnInvalidatorStateChange(InvalidatorState state) OVERRIDE;
   virtual void OnIncomingInvalidation(
-      const ObjectIdInvalidationMap& invalidation_map) OVERRIDE;
+      syncer::ModelType type,
+      scoped_ptr<InvalidationInterface> invalidation) OVERRIDE;
   virtual void AddObserver(SyncManager::Observer* observer) OVERRIDE;
   virtual void RemoveObserver(SyncManager::Observer* observer) OVERRIDE;
   virtual SyncStatus GetDetailedStatus() const OVERRIDE;
@@ -85,7 +86,6 @@ class SYNC_EXPORT_PRIVATE SyncRollbackManagerBase :
   virtual bool HasUnsyncedItems() OVERRIDE;
   virtual SyncEncryptionHandler* GetEncryptionHandler() OVERRIDE;
   virtual void RefreshTypes(ModelTypeSet types) OVERRIDE;
-  virtual std::string GetOwnerName() const OVERRIDE;
   virtual SyncContextProxy* GetSyncContextProxy() OVERRIDE;
   virtual ScopedVector<ProtocolEvent> GetBufferedProtocolEvents()
       OVERRIDE;
diff --git a/sync/internal_api/test/fake_sync_manager.cc b/sync/internal_api/test/fake_sync_manager.cc
index 7b5e32f..5bb4f10 100644
--- a/sync/internal_api/test/fake_sync_manager.cc
+++ b/sync/internal_api/test/fake_sync_manager.cc
@@ -263,7 +263,8 @@ bool FakeSyncManager::HasDirectoryTypeDebugInfoObserver(
 void FakeSyncManager::RequestEmitDebugInfo() {}
 
 void FakeSyncManager::OnIncomingInvalidation(
-      const ObjectIdInvalidationMap& invalidation_map) {
+    syncer::ModelType type,
+    scoped_ptr<InvalidationInterface> invalidation) {
   // Do nothing.
 }
 
@@ -275,6 +276,4 @@ void FakeSyncManager::OnInvalidatorStateChange(InvalidatorState state) {
   // Do nothing.
 }
 
-std::string FakeSyncManager::GetOwnerName() const { return "FakeSyncManager"; }
-
 }  // namespace syncer