Refactor sync-specific parts out of SyncNotifier/SyncNotifierObserver

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
new file mode 100644
index 0000000000000000000000000000000000000000..65174f4089175b666e4334cd805c7ea4c402ef0c
--- /dev/null
+++ b/sync/notifier/sync_notifier_helper.cc
@@ -0,0 +1,65 @@
+// Copyright (c) 2012 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/notifier/sync_notifier_helper.h"
+
+namespace syncer {
+
+SyncNotifierHelper::SyncNotifierHelper() {}
+SyncNotifierHelper::~SyncNotifierHelper() {}
+
+ObjectIdSet SyncNotifierHelper::UpdateRegisteredIds(
+    SyncNotifierObserver* handler, const ObjectIdSet& ids) {
+  if (ids.empty()) {

[akalin, 2012/07/19 00:42:07]

given that at most one handler can handle a given object ID, I think a better
data structure would be:

map<Observer*, ObjectIdSet> handler_to_ids_map_;
ObjectIdSet registered_ids_;

Then this function could just be:

ids_to_unregister = SetDiff(ids, handler_to_ids_map_[handler]);
if (ids.empty()) {
  handlers_to_ids_map_.erase(handler);
} else {
  handlers_to_ids_map_[handler] = ids;
}

registered_ids_ = SetDiff(Union(registered_ids_, ids), ids_to_unregister);
return registered_ids_;

And DispatchInvalidationsToHandlers can just be:

for ((handler, ids) in handlers_to_ids_map_) {
  <build id->payload map from id_payloads and ids>
  dispatch_map[handler] = id->payload map;
}

<dispatch>

[dcheng, 2012/07/19 18:31:05]

I'm not a fan of the proposed scheme. It's more logical to go the other way and
build a directory of lookups for dispatching, rather than the proposed scheme of
scanning through all registered handlers and filtering the entire payload map
against each object ID set. You could use a counter to break out early, but
that'd just make the Dispatch method even longer.

[akalin, 2012/07/19 23:07:26]

Ah, I see your point.  You're optimizing for the Dispatch method.  That makes
sense, but that does complicate UpdateRegisteredIds.  I guess that's okay,
although I'll try to think of a way to simplify this method.

+    handlers_.RemoveObserver(handler);
+  } else if (!handlers_.HasObserver(handler)) {
+    handlers_.AddObserver(handler);
+  }
+
+  ObjectIdSet registered_ids;
+  for (ObjectIdObserverMap::iterator it = id_to_handler_map_.begin();
+       it != id_to_handler_map_.end(); ) {
+    if (it->second == handler) {
+      ObjectIdObserverMap::iterator erase_it = it;
+      ++it;
+      id_to_handler_map_.erase(erase_it);
+    } else {
+      registered_ids.insert(it->first);
+      ++it;
+    }
+  }
+
+  // Small optimization so we only traverse the map once when inserting entries.
+  ObjectIdObserverMap::iterator insert_it = id_to_handler_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));
+  }
+  registered_ids.insert(ids.begin(), ids.end());
+  return registered_ids;
+}
+
+void SyncNotifierHelper::DispatchInvalidationsToHandlers(
+    const ObjectIdPayloadMap& id_payloads,
+    IncomingNotificationSource source) {
+  std::map<SyncNotifierObserver*, ObjectIdPayloadMap> dispatch_map;

[akalin, 2012/07/19 00:42:07]

typedef the map to something? (can be scoped to this function)

[dcheng, 2012/07/19 18:31:05]

Done.

+  for (ObjectIdPayloadMap::const_iterator it = id_payloads.begin();
+       it != id_payloads.end(); ++it) {
+    ObjectIdObserverMap::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())
+      continue;
+    dispatch_map[find_it->second].insert(*it);
+  }
+  for (std::map<SyncNotifierObserver*, ObjectIdPayloadMap>::const_iterator it =
+       dispatch_map.begin(); it != dispatch_map.end(); ++it) {
+    DCHECK(handlers_.HasObserver(it->first));

[akalin, 2012/07/19 00:42:07]

this doesn't necessarily hold does it?  OnIncomingNotification can do anything,
including removing all other handlers.

[dcheng, 2012/07/19 18:31:05]

That seems like an overreaching handler, and not a situation that we should
encourage. But yes, I guess it's possible =)

I'm happy to remove the DCHECK if you want though.

[akalin, 2012/07/19 23:07:26]

Maybe add a comment somewhere mandating that observers should only touch
themselves ,then?

[dcheng, 2012/07/20 00:57:04]

After more thought, I just removed the DCHECK. I also updated the comments for
SyncNotifier::UpdateRegisteredIds and moved the comment about one handler per ID
there, since it's the public interface that gets used.

+    it->first->OnIncomingNotification(it->second, source);
+  }
+}
+
+}  // namespace syncer