Convert InvalidationStateTracker to use invalidation::ObjectId instead of syncable::ModelType

BUG=124145,124149
TEST=unit tests

Review URL: https://chromiumcodereview.appspot.com/10545168

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

9 files changed, 66 insertions, 42 deletions

diff --git a/sync/notifier/chrome_invalidation_client.cc b/sync/notifier/chrome_invalidation_client.cc
index 0c6c1e0..3557494 100644
--- a/sync/notifier/chrome_invalidation_client.cc
+++ b/sync/notifier/chrome_invalidation_client.cc
@@ -74,7 +74,7 @@ void ChromeInvalidationClient::Start(
              max_invalidation_versions_.begin();
          it != max_invalidation_versions_.end(); ++it) {
       DVLOG(2) << "Initial max invalidation version for "
-               << syncable::ModelTypeToString(it->first) << " is "
+               << ObjectIdToString(it->first) << " is "
                << it->second;
     }
   }
@@ -124,38 +124,41 @@ void ChromeInvalidationClient::Invalidate(
     const invalidation::AckHandle& ack_handle) {
   DCHECK(CalledOnValidThread());
   DVLOG(1) << "Invalidate: " << InvalidationToString(invalidation);
-  syncable::ModelType model_type;
-  if (!ObjectIdToRealModelType(invalidation.object_id(), &model_type)) {
-    LOG(WARNING) << "Could not get invalidation model type; "
-                 << "invalidating everything";
-    EmitInvalidation(registered_types_, std::string());
-    client->Acknowledge(ack_handle);
-    return;
-  }
+
+  const invalidation::ObjectId& id = invalidation.object_id();
+
   // The invalidation API spec allows for the possibility of redundant
   // invalidations, so keep track of the max versions and drop
   // invalidations with old versions.
   //
-  // TODO(akalin): Now that we keep track of registered types, we
-  // should drop invalidations for unregistered types.  We may also
+  // TODO(akalin): Now that we keep track of registered ids, we
+  // should drop invalidations for unregistered ids.  We may also
   // have to filter it at a higher level, as invalidations for
-  // newly-unregistered types may already be in flight.
+  // newly-unregistered ids may already be in flight.
   InvalidationVersionMap::const_iterator it =
-      max_invalidation_versions_.find(model_type);
+      max_invalidation_versions_.find(id);
   if ((it != max_invalidation_versions_.end()) &&
       (invalidation.version() <= it->second)) {
     // Drop redundant invalidations.
     client->Acknowledge(ack_handle);
     return;
   }
-  DVLOG(2) << "Setting max invalidation version for "
-           << syncable::ModelTypeToString(model_type) << " to "
-           << invalidation.version();
-  max_invalidation_versions_[model_type] = invalidation.version();
+  DVLOG(2) << "Setting max invalidation version for " << ObjectIdToString(id)
+           << " to " << invalidation.version();
+  max_invalidation_versions_[id] = invalidation.version();
   invalidation_state_tracker_.Call(
       FROM_HERE,
       &InvalidationStateTracker::SetMaxVersion,
-      model_type, invalidation.version());
+      id, invalidation.version());
+
+  syncable::ModelType model_type;
+  if (!ObjectIdToRealModelType(id, &model_type)) {
+    LOG(WARNING) << "Could not get invalidation model type; "
+                 << "invalidating everything";
+    EmitInvalidation(registered_types_, std::string());
+    client->Acknowledge(ack_handle);
+    return;
+  }
 
   std::string payload;
   // payload() CHECK()'s has_payload(), so we must check it ourselves first.
diff --git a/sync/notifier/chrome_invalidation_client_unittest.cc b/sync/notifier/chrome_invalidation_client_unittest.cc
index 40ddbdd..8cfe490 100644
--- a/sync/notifier/chrome_invalidation_client_unittest.cc
+++ b/sync/notifier/chrome_invalidation_client_unittest.cc
@@ -31,6 +31,8 @@ const char kClientInfo[] = "client_info";
 const char kState[] = "state";
 const char kNewState[] = "new_state";
 
+const int kChromeSyncSourceId = 1004;
+
 class MockInvalidationClient : public invalidation::InvalidationClient {
  public:
   MOCK_METHOD0(Start, void());
@@ -55,7 +57,11 @@ class ChromeInvalidationClientTest : public testing::Test {
  protected:
   ChromeInvalidationClientTest()
       : fake_push_client_(new notifier::FakePushClient()),
-        client_(scoped_ptr<notifier::PushClient>(fake_push_client_)) {}
+        client_(scoped_ptr<notifier::PushClient>(fake_push_client_)),
+        kBookmarksId_(kChromeSyncSourceId, "BOOKMARK"),
+        kPreferencesId_(kChromeSyncSourceId, "PREFERENCE"),
+        kExtensionsId_(kChromeSyncSourceId, "EXTENSION"),
+        kAppsId_(kChromeSyncSourceId, "APP") {}
 
   virtual void SetUp() {
     client_.Start(kClientId, kClientInfo, kState,
@@ -119,6 +125,11 @@ class ChromeInvalidationClientTest : public testing::Test {
   StrictMock<MockInvalidationClient> mock_invalidation_client_;
   notifier::FakePushClient* const fake_push_client_;
   ChromeInvalidationClient client_;
+
+  const invalidation::ObjectId kBookmarksId_;
+  const invalidation::ObjectId kPreferencesId_;
+  const invalidation::ObjectId kExtensionsId_;
+  const invalidation::ObjectId kAppsId_;
 };
 
 namespace {
@@ -141,15 +152,17 @@ TEST_F(ChromeInvalidationClientTest, InvalidateBadObjectId) {
   syncable::ModelTypeSet types(syncable::BOOKMARKS, syncable::APPS);
   client_.RegisterTypes(types);
   EXPECT_CALL(mock_listener_, OnInvalidate(MakeMapFromSet(types, "")));
+  EXPECT_CALL(mock_invalidation_state_tracker_,
+              SetMaxVersion(invalidation::ObjectId(kChromeSyncSourceId, "bad"),
+                            1));
   FireInvalidate("bad", 1, NULL);
-  message_loop_.RunAllPending();
 }
 
 TEST_F(ChromeInvalidationClientTest, InvalidateNoPayload) {
   EXPECT_CALL(mock_listener_,
               OnInvalidate(MakeMap(syncable::BOOKMARKS, "")));
   EXPECT_CALL(mock_invalidation_state_tracker_,
-              SetMaxVersion(syncable::BOOKMARKS, 1));
+              SetMaxVersion(kBookmarksId_, 1));
   FireInvalidate("BOOKMARK", 1, NULL);
 }
 
@@ -157,7 +170,7 @@ TEST_F(ChromeInvalidationClientTest, InvalidateWithPayload) {
   EXPECT_CALL(mock_listener_,
               OnInvalidate(MakeMap(syncable::PREFERENCES, "payload")));
   EXPECT_CALL(mock_invalidation_state_tracker_,
-              SetMaxVersion(syncable::PREFERENCES, 1));
+              SetMaxVersion(kPreferencesId_, 1));
   FireInvalidate("PREFERENCE", 1, "payload");
 }
 
@@ -167,7 +180,7 @@ TEST_F(ChromeInvalidationClientTest, InvalidateVersion) {
   EXPECT_CALL(mock_listener_,
               OnInvalidate(MakeMap(syncable::APPS, "")));
   EXPECT_CALL(mock_invalidation_state_tracker_,
-              SetMaxVersion(syncable::APPS, 1));
+              SetMaxVersion(kAppsId_, 1));
 
   // Should trigger.
   FireInvalidate("APP", 1, NULL);
@@ -200,9 +213,9 @@ TEST_F(ChromeInvalidationClientTest, InvalidateVersionMultipleTypes) {
               OnInvalidate(MakeMap(syncable::EXTENSIONS, "")));
 
   EXPECT_CALL(mock_invalidation_state_tracker_,
-              SetMaxVersion(syncable::APPS, 3));
+              SetMaxVersion(kAppsId_, 3));
   EXPECT_CALL(mock_invalidation_state_tracker_,
-              SetMaxVersion(syncable::EXTENSIONS, 2));
+              SetMaxVersion(kExtensionsId_, 2));
 
   // Should trigger both.
   FireInvalidate("APP", 3, NULL);
@@ -233,11 +246,11 @@ TEST_F(ChromeInvalidationClientTest, InvalidateVersionMultipleTypes) {
               OnInvalidate(MakeMap(syncable::APPS, "")));
 
   EXPECT_CALL(mock_invalidation_state_tracker_,
-              SetMaxVersion(syncable::PREFERENCES, 5));
+              SetMaxVersion(kPreferencesId_, 5));
   EXPECT_CALL(mock_invalidation_state_tracker_,
-              SetMaxVersion(syncable::EXTENSIONS, 3));
+              SetMaxVersion(kExtensionsId_, 3));
   EXPECT_CALL(mock_invalidation_state_tracker_,
-              SetMaxVersion(syncable::APPS, 4));
+              SetMaxVersion(kAppsId_, 4));
 
   // Should trigger all three.
   FireInvalidate("PREFERENCE", 5, NULL);
diff --git a/sync/notifier/chrome_system_resources.cc b/sync/notifier/chrome_system_resources.cc
index 62a44ac..702078b 100644
--- a/sync/notifier/chrome_system_resources.cc
+++ b/sync/notifier/chrome_system_resources.cc
@@ -14,6 +14,7 @@
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
+#include "google/cacheinvalidation/deps/callback.h"
 #include "google/cacheinvalidation/include/types.h"
 #include "jingle/notifier/listener/push_client.h"
 #include "sync/notifier/invalidation_util.h"
@@ -123,8 +124,9 @@ void ChromeScheduler::SetSystemResources(
 
 void ChromeScheduler::RunPostedTask(invalidation::Closure* task) {
   CHECK_EQ(created_on_loop_, MessageLoop::current());
-  RunAndDeleteClosure(task);
+  task->Run();
   posted_tasks_.erase(task);
+  delete task;
 }
 
 ChromeStorage::ChromeStorage(StateWriter* state_writer,
diff --git a/sync/notifier/invalidation_state_tracker.h b/sync/notifier/invalidation_state_tracker.h
index b09f72e..9dbc352 100644
--- a/sync/notifier/invalidation_state_tracker.h
+++ b/sync/notifier/invalidation_state_tracker.h
@@ -11,11 +11,13 @@
 #include <map>
 
 #include "base/basictypes.h"
-#include "sync/internal_api/public/syncable/model_type.h"
+#include "google/cacheinvalidation/include/types.h"
+#include "sync/notifier/invalidation_util.h"
 
 namespace csync {
 
-typedef std::map<syncable::ModelType, int64> InvalidationVersionMap;
+typedef std::map<invalidation::ObjectId, int64, ObjectIdLessThan>
+    InvalidationVersionMap;
 
 class InvalidationStateTracker {
  public:
@@ -25,7 +27,7 @@ class InvalidationStateTracker {
 
   // |max_version| should be strictly greater than any existing max
   // version for |model_type|.
-  virtual void SetMaxVersion(syncable::ModelType model_type,
+  virtual void SetMaxVersion(const invalidation::ObjectId& id,
                              int64 max_version) = 0;
 
   // Used by InvalidationClient for persistence. |state| is opaque data we can
diff --git a/sync/notifier/invalidation_util.cc b/sync/notifier/invalidation_util.cc
index 27bb851..3e680ec 100644
--- a/sync/notifier/invalidation_util.cc
+++ b/sync/notifier/invalidation_util.cc
@@ -11,9 +11,10 @@
 
 namespace csync {
 
-void RunAndDeleteClosure(invalidation::Closure* task) {
-  task->Run();
-  delete task;
+bool ObjectIdLessThan::operator()(const invalidation::ObjectId& lhs,
+                                  const invalidation::ObjectId& rhs) const {
+  return (lhs.source() < rhs.source()) ||
+         (lhs.source() == rhs.source() && lhs.name() < rhs.name());
 }
 
 bool RealModelTypeToObjectId(syncable::ModelType model_type,
diff --git a/sync/notifier/invalidation_util.h b/sync/notifier/invalidation_util.h
index 8d7de66..628ffed 100644
--- a/sync/notifier/invalidation_util.h
+++ b/sync/notifier/invalidation_util.h
@@ -10,7 +10,6 @@
 
 #include <string>
 
-#include "google/cacheinvalidation/deps/callback.h"
 #include "sync/internal_api/public/syncable/model_type.h"
 
 namespace invalidation {
@@ -22,7 +21,10 @@ class ObjectId;
 
 namespace csync {
 
-void RunAndDeleteClosure(invalidation::Closure* task);
+struct ObjectIdLessThan {
+  bool operator()(const invalidation::ObjectId& lhs,
+                  const invalidation::ObjectId& rhs) const;
+};
 
 bool RealModelTypeToObjectId(syncable::ModelType model_type,
                              invalidation::ObjectId* object_id);
diff --git a/sync/notifier/mock_invalidation_state_tracker.h b/sync/notifier/mock_invalidation_state_tracker.h
index f5a84da..ac884f5 100644
--- a/sync/notifier/mock_invalidation_state_tracker.h
+++ b/sync/notifier/mock_invalidation_state_tracker.h
@@ -19,7 +19,7 @@ class MockInvalidationStateTracker
   virtual ~MockInvalidationStateTracker();
 
   MOCK_CONST_METHOD0(GetAllMaxVersions, InvalidationVersionMap());
-  MOCK_METHOD2(SetMaxVersion, void(syncable::ModelType, int64));
+  MOCK_METHOD2(SetMaxVersion, void(const invalidation::ObjectId&, int64));
   MOCK_CONST_METHOD0(GetInvalidationState, std::string());
   MOCK_METHOD1(SetInvalidationState, void(const std::string&));
 };
diff --git a/sync/sync.gyp b/sync/sync.gyp
index 8202ea2..7fca6b9 100644
--- a/sync/sync.gyp
+++ b/sync/sync.gyp
@@ -386,8 +386,6 @@
         'internal_api/public/syncable/model_type_test_util.h',
         'js/js_test_util.cc',
         'js/js_test_util.h',
-        'notifier/mock_invalidation_state_tracker.cc',
-        'notifier/mock_invalidation_state_tracker.h',
         'sessions/test_util.cc',
         'sessions/test_util.h',
         'syncable/syncable_mock.cc',
@@ -436,6 +434,8 @@
         'sync_notifier',
       ],
       'sources': [
+        'notifier/mock_invalidation_state_tracker.cc',
+        'notifier/mock_invalidation_state_tracker.h',
         'notifier/mock_sync_notifier_observer.cc',
         'notifier/mock_sync_notifier_observer.h',
       ],
diff --git a/sync/tools/sync_listen_notifications.cc b/sync/tools/sync_listen_notifications.cc
index 58e116f..e13f3e3 100644
--- a/sync/tools/sync_listen_notifications.cc
+++ b/sync/tools/sync_listen_notifications.cc
@@ -23,6 +23,7 @@
 #include "sync/internal_api/public/syncable/model_type.h"
 #include "sync/internal_api/public/syncable/model_type_payload_map.h"
 #include "sync/notifier/invalidation_state_tracker.h"
+#include "sync/notifier/invalidation_util.h"
 #include "sync/notifier/sync_notifier.h"
 #include "sync/notifier/sync_notifier_factory.h"
 #include "sync/notifier/sync_notifier_observer.h"
@@ -78,10 +79,10 @@ class NullInvalidationStateTracker
   }
 
   virtual void SetMaxVersion(
-      syncable::ModelType model_type,
+      const invalidation::ObjectId& id,
       int64 max_invalidation_version) OVERRIDE {
     LOG(INFO) << "Setting max invalidation version for "
-              << syncable::ModelTypeToString(model_type) << " to "
+              << csync::ObjectIdToString(id) << " to "
               << max_invalidation_version;
   }