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..3c7a026b1f749bc1e321664fef7ca24305c6e51d
--- /dev/null
+++ b/sync/notifier/sync_notifier_helper.cc
@@ -0,0 +1,74 @@
+// 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() {}
+
+ObjectIdSet SyncNotifierHelper::UpdateRegisteredIds(
+    SyncNotifierObserver* handler, const ObjectIdSet& ids) {
+  if (ids.empty()) {
+    handlers_.RemoveObserver(handler);
+  } else if (!handlers_.HasObserver(handler)) {
+    handlers_.AddObserver(handler);
+  }
+
+  ObjectIdSet registered_ids;

[akalin, 2012/07/20 19:01:40]

why not just initialize registered_ids with ids?

[akalin, 2012/07/20 19:01:40]

Add a comment here like:

// Remove any existing entries for |handler| and fill |registered_ids| with the
rest.

[dcheng, 2012/07/21 00:09:54]

Done.

[dcheng, 2012/07/21 00:09:54]

Done.

+  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.

[akalin, 2012/07/20 19:01:40]

Then add a comment like:

// Now add the new entries for |handler|.

[akalin, 2012/07/20 19:01:40]

it's a bit unclear what the small optimization is.  Perhaps:

// Keep track of the last insertion so we only ...

[dcheng, 2012/07/21 00:09:54]

Done.

[dcheng, 2012/07/21 00:09:54]

Done.

+  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,

[akalin, 2012/07/20 19:01:40]

this is dangerous since if an entry for the id already exists (in violation of
the API contract) this insertion will silently no-op.

How about adding:

// Make sure there's no existing handler for an ID.
DCHECK_EQ(insert_it->second, handler);

?

Although ideally, we'd clobber instead of no-oping in release mode.  Yeah, let's
do that.  I think it suffices to do:

if (insert_id->second != handler) {
  NOTREACHED() << <some message>;
  insert_it->second = handler;
}

[akalin, 2012/07/20 19:34:38]

Actually, on second thought, I think we should just CHECK_EQ().

[dcheng, 2012/07/21 00:09:54]

Is there a way to make CHECKS non-fatal for unit tests? Otherwise, I don't see
how I can add this without breaking the unti test.

[dcheng, 2012/07/21 00:09:54]

Done with a variation.

+                                          std::make_pair(*it, handler));
+  }
+  registered_ids.insert(ids.begin(), ids.end());
+  return registered_ids;

[akalin, 2012/07/20 19:01:40]

add a DCHECK somewhere (or a test in an "if (DEBUG_MODE)" block) that makes sure
handlers_ is consistent with id_to_handler_map_.

[dcheng, 2012/07/21 00:09:54]

Done.

+}
+
+void SyncNotifierHelper::DispatchInvalidationsToHandlers(
+    const ObjectIdPayloadMap& id_payloads,
+    IncomingNotificationSource source) {
+  typedef std::map<SyncNotifierObserver*, ObjectIdPayloadMap> DispatchMap;
+  DispatchMap dispatch_map;
+  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 =

[akalin, 2012/07/20 19:01:40]

i think it's worth it to do a FOR_EACH_OBSERVER-style loop here, actually, and
do a lookup on dispatch_map for each observer.  It's not as efficient, but I
think the consistency of behavior is worth it.

[akalin, 2012/07/20 19:01:40]

use the typedef here

[dcheng, 2012/07/21 00:09:54]

Done.

[dcheng, 2012/07/21 00:09:54]

I'd prefer not to dispatch invalidations with no associated ObjectIdSet though.

+       dispatch_map.begin(); it != dispatch_map.end(); ++it) {
+    it->first->OnIncomingNotification(it->second, source);
+  }
+}
+
+void SyncNotifierHelper::OnNotificationsEnabled() {
+  FOR_EACH_OBSERVER(SyncNotifierObserver, handlers_, OnNotificationsEnabled());
+}
+
+void SyncNotifierHelper::OnNotificationsDisabled(
+    NotificationsDisabledReason reason) {
+  FOR_EACH_OBSERVER(SyncNotifierObserver, handlers_,
+                    OnNotificationsDisabled(reason));
+}
+
+}  // namespace syncer