[Sync] Avoid unregistering object IDs on shutdown

sync/notifier/sync_notifier_helper.cc

Index: sync/notifier/sync_notifier_helper.cc
diff --git a/sync/notifier/sync_notifier_helper.cc b/sync/notifier/sync_notifier_helper.cc
index af3b2ea72d5587b1295b33bac027677b80ecdbe0..209fbf6784218d52835fc8ed6a8816fd201d8df9 100644
--- a/sync/notifier/sync_notifier_helper.cc
+++ b/sync/notifier/sync_notifier_helper.cc
@@ -4,70 +4,103 @@
 
 #include "sync/notifier/sync_notifier_helper.h"
 
+#include <cstddef>
+
 #include "base/logging.h"
 
 namespace syncer {
 
 SyncNotifierHelper::SyncNotifierHelper() {}
-SyncNotifierHelper::~SyncNotifierHelper() {}
 
-ObjectIdSet SyncNotifierHelper::UpdateRegisteredIds(
-    SyncNotifierObserver* handler, const ObjectIdSet& ids) {
-  if (ids.empty()) {
-    handlers_.RemoveObserver(handler);
-  } else if (!handlers_.HasObserver(handler)) {
+SyncNotifierHelper::~SyncNotifierHelper() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+void SyncNotifierHelper::SetHandler(const std::string& handler_name,
+                                    SyncNotifierObserver* handler) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  SyncNotifierObserver* const old_handler =
+      HandlerNameToHandler(handler_name);
+  if (old_handler) {
+    handlers_.RemoveObserver(old_handler);
+  }
+
+  if (handler) {
     handlers_.AddObserver(handler);
+    name_to_handler_map_[handler_name] = handler;
+  } else {
+    handlers_.RemoveObserver(old_handler);

[dcheng, 2012/08/03 06:05:30]

I don't think you need this line, since it's always removed at line 25.

[akalin, 2012/08/03 18:44:10]

Done.  In fact, the entire else block can be moved to the first block.

+    name_to_handler_map_.erase(handler_name);
   }
+}
 
-  ObjectIdSet registered_ids(ids);
-  // Remove all existing entries for |handler| and fill |registered_ids| with
-  // the rest.
-  for (ObjectIdHandlerMap::iterator it = id_to_handler_map_.begin();
-       it != id_to_handler_map_.end(); ) {
-    if (it->second == handler) {
-      ObjectIdHandlerMap::iterator erase_it = it;
+void SyncNotifierHelper::UpdateRegisteredIds(
+    const std::string& handler_name, const ObjectIdSet& ids) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Remove all existing entries for |handler_name|.
+  for (ObjectIdNameMap::iterator it = id_to_name_map_.begin();
+       it != id_to_name_map_.end(); ) {
+    if (it->second == handler_name) {
+      ObjectIdNameMap::iterator erase_it = it;
       ++it;
-      id_to_handler_map_.erase(erase_it);
+      id_to_name_map_.erase(erase_it);
     } else {
-      registered_ids.insert(it->first);
       ++it;
     }
   }
 
-  // Now add the entries for |handler|. We keep track of the last insertion
-  // point so we only traverse the map once to insert all the new entries.
-  ObjectIdHandlerMap::iterator insert_it = id_to_handler_map_.begin();
+  // Now add the entries for |handler_name|. We keep track of the last
+  // insertion point so we only traverse the map once to insert all
+  // the new entries.
+  ObjectIdNameMap::iterator insert_it = id_to_name_map_.begin();
   for (ObjectIdSet::const_iterator it = ids.begin(); it != ids.end(); ++it) {
-    insert_it = id_to_handler_map_.insert(insert_it,
-                                          std::make_pair(*it, handler));
-    CHECK_EQ(handler, insert_it->second) << "Duplicate registration for "
-                                         << ObjectIdToString(insert_it->first);
+    insert_it =
+        id_to_name_map_.insert(insert_it, std::make_pair(*it, handler_name));
+    CHECK_EQ(handler_name, insert_it->second)
+        << "Duplicate registration: trying to register "
+        << ObjectIdToString(insert_it->first) << " for "
+        << handler_name << " when it's already registered for "
+        << insert_it->second;
   }
+
   if (logging::DEBUG_MODE) {
     // The mapping shouldn't contain any handlers that aren't in |handlers_|.
-    for (ObjectIdHandlerMap::const_iterator it = id_to_handler_map_.begin();
-         it != id_to_handler_map_.end(); ++it) {
-      CHECK(handlers_.HasObserver(it->second));
+    for (ObjectIdNameMap::const_iterator it = id_to_name_map_.begin();
+         it != id_to_name_map_.end(); ++it) {
+      SyncNotifierObserver* const handler = HandlerNameToHandler(it->second);
+      if (handler) {
+        CHECK(handlers_.HasObserver(handler));
+      }
     }
   }
+}
+
+ObjectIdSet SyncNotifierHelper::GetAllRegisteredIds() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ObjectIdSet registered_ids;
+  for (ObjectIdNameMap::const_iterator it = id_to_name_map_.begin();
+       it != id_to_name_map_.end(); ++it) {
+    registered_ids.insert(it->first);
+  }
   return registered_ids;
 }
 
 void SyncNotifierHelper::DispatchInvalidationsToHandlers(
     const ObjectIdPayloadMap& id_payloads,
     IncomingNotificationSource source) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   typedef std::map<SyncNotifierObserver*, ObjectIdPayloadMap> DispatchMap;
   DispatchMap dispatch_map;
   for (ObjectIdPayloadMap::const_iterator it = id_payloads.begin();
        it != id_payloads.end(); ++it) {
-    ObjectIdHandlerMap::const_iterator find_it =
-        id_to_handler_map_.find(it->first);
-    // If we get an invalidation with a source type that we can't map to an
-    // observer, just drop it--the observer was unregistered while the
-    // invalidation was in flight.
-    if (find_it == id_to_handler_map_.end())
+    SyncNotifierObserver* const handler = ObjectIdToHandler(it->first);
+    // If we get an invalidation with a source type that we can't map
+    // to an handler, just drop it -- the handler was unregistered
+    // while the invalidation was in flight.
+    if (!handler) {
       continue;
-    dispatch_map[find_it->second].insert(*it);
+    }
+    dispatch_map[handler].insert(*it);
   }
 
   if (handlers_.might_have_observers()) {
@@ -83,13 +116,37 @@ void SyncNotifierHelper::DispatchInvalidationsToHandlers(
 }
 
 void SyncNotifierHelper::EmitOnNotificationsEnabled() {
-  FOR_EACH_OBSERVER(SyncNotifierObserver, handlers_, OnNotificationsEnabled());
+  DCHECK(thread_checker_.CalledOnValidThread());
+  FOR_EACH_OBSERVER(SyncNotifierObserver, handlers_,
+                    OnNotificationsEnabled());
 }
 
 void SyncNotifierHelper::EmitOnNotificationsDisabled(
     NotificationsDisabledReason reason) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   FOR_EACH_OBSERVER(SyncNotifierObserver, handlers_,
                     OnNotificationsDisabled(reason));
 }
 
+void SyncNotifierHelper::DetachFromThreadForTest() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  thread_checker_.DetachFromThread();
+}
+
+SyncNotifierObserver* SyncNotifierHelper::HandlerNameToHandler(
+    const std::string& handler_name) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  NameHandlerMap::const_iterator it =
+      name_to_handler_map_.find(handler_name);
+  return (it == name_to_handler_map_.end()) ? NULL : it->second;
+}
+
+SyncNotifierObserver* SyncNotifierHelper::ObjectIdToHandler(
+    const invalidation::ObjectId& id) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ObjectIdNameMap::const_iterator it = id_to_name_map_.find(id);
+  return
+      (it == id_to_name_map_.end()) ? NULL : HandlerNameToHandler(it->second);
+}
+
 }  // namespace syncer