[Sync] Maintain a global_id mapping to update UserEvents that references navigations

components/sync_sessions/sessions_sync_manager.cc

Index: components/sync_sessions/sessions_sync_manager.cc
diff --git a/components/sync_sessions/sessions_sync_manager.cc b/components/sync_sessions/sessions_sync_manager.cc
index 5810f8fc937e2677a29fbc6320a2f727e861c95f..f7bdf27b1a4c4d16f5e220003d5c9974b9f45c3e 100644
--- a/components/sync_sessions/sessions_sync_manager.cc
+++ b/components/sync_sessions/sessions_sync_manager.cc
@@ -51,6 +51,12 @@ const char kNTPOpenTabSyncURL[] = "chrome://newtab/#open_tabs";
 // stale and becomes a candidate for garbage collection.
 const int kDefaultStaleSessionThresholdDays = 14;  // 2 weeks.
 
+// Clean up navigation tracking when we have over this many global_ids.
+const size_t kNavigationTrackingCleanupThreshold = 100;
+
+// When we clean up navigation tracking, delete this many global_ids.
+const int kNavigationTrackingCleanupAmount = 10;
+
 // Comparator function for use with std::sort that will sort tabs by
 // descending timestamp (i.e., most recent first).
 bool TabsRecencyComparator(const sessions::SessionTab* t1,
@@ -578,6 +584,11 @@ void SessionsSyncManager::OnLocalTabModified(SyncedTabDelegate* modified_tab) {
     }
   }
 
+  sessions::SerializedNavigationEntry current;
+  modified_tab->GetSerializedNavigationAtIndex(
+      modified_tab->GetCurrentEntryIndex(), &current);
+  TrackNavigationIds(current);
+
   if (local_tab_pool_out_of_sync_) {
     // If our tab pool is corrupt, pay the price of a full re-association to
     // fix things up.  This takes care of the new tab modification as well.
@@ -1285,6 +1296,22 @@ void SessionsSyncManager::DoGarbageCollection() {
     sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
 }
 
+void SessionsSyncManager::AddGlobalIdChangeObserver(
+    syncer::GlobalIdChange callback) {
+  global_id_change_observers_.push_back(std::move(callback));
+}
+
+int64_t SessionsSyncManager::GetLatestGlobalId(int64_t global_id) {
+  auto g2u_iter = global_to_unique_.find(global_id);
+  if (g2u_iter != global_to_unique_.end()) {

[Patrick Noland, 2017/06/28 22:44:37]

This might look better as if (auto g2u_iter = ... != end()), but decide for
yourself how you feel about that idiom

[skym, 2017/07/05 19:14:28]

Acknowledged.

+    auto u2g_iter = unique_to_current_global_.find(g2u_iter->second);
+    if (u2g_iter != unique_to_current_global_.end()) {
+      return u2g_iter->second;
+    }
+  }
+  return global_id;
+}
+
 // static
 std::string SessionsSyncManager::TagHashFromSpecifics(
     const sync_pb::SessionSpecifics& specifics) {
@@ -1292,4 +1319,75 @@ std::string SessionsSyncManager::TagHashFromSpecifics(
                                       TagFromSpecifics(specifics));
 }
 
+void SessionsSyncManager::TrackNavigationIds(
+    const sessions::SerializedNavigationEntry& current) {
+  // The expectation is that global_id will update for a given unique_id, which
+  // should accurately and uniquely represent a single navigation. It is
+  // theoretically possible for two unique_ids to map to the same global_id, but
+  // hopefully rare enough that it doesn't cause much harm. Lets record metrics
+  // verify this theory.
+  int64_t global_id = current.timestamp().ToInternalValue();
+  // It is possible that the global_id has not been set yet for this navigation.
+  // In this case there's nothing here for us to track yet.
+  if (global_id == 0) {
+    return;
+  }
+
+  int unique_id = current.unique_id();
+  DCHECK_NE(0, unique_id);
+
+  auto g2u_iter = global_to_unique_.find(global_id);
+  if (g2u_iter == global_to_unique_.end()) {
+    global_to_unique_.insert(g2u_iter, std::make_pair(global_id, unique_id));
+  } else if (g2u_iter->second != unique_id) {
+    UMA_HISTOGRAM_COUNTS("Sync.GlobalIdConflict", 1);

[Steven Holte, 2017/06/28 18:59:45]

UMA_HISTOGRAM_COUNTS is an alias for UMA_HISTOGRAM_COUNTS_1M (Using 50 buckets
for values 1 -1M).  If you are always recording a value of 1, you should
probably instead just use an enum histogram with a single bucket.

[skym, 2017/07/05 19:14:28]

Done, switched to UMA_HISTOGRAM_ENUMERATION. Wasn't sure if I should emit 0 or 1
and what the units should be, so switched to a real enum and emitting all cases
here instead.

+  }
+
+  auto u2g_iter = unique_to_current_global_.find(unique_id);
+  if (u2g_iter == unique_to_current_global_.end()) {
+    unique_to_current_global_.insert(u2g_iter,

[Patrick Noland, 2017/06/28 22:44:37]

if you use base::flat_map (see
https://cs.chromium.org/chromium/src/base/containers/README.md?type=cs) you can
use insert_or_assign, and your max size is small enough that it's a wash at
worst performance-wise

[skym, 2017/07/05 19:14:28]

I think it'd be a bit confusing to have these two maps have different types.
global_to_unique_ is likely going to sit at ~max capacity for most users, I'm
not sure about switching.

+                                     std::make_pair(unique_id, global_id));
+  } else if (u2g_iter->second != global_id) {
+    // Remember the old_global_id before we insert and invalidate out iter.
+    int64_t old_global_id = u2g_iter->second;
+
+    // TODO(skym): Use insert_or_assign with hint once on C++17.
+    unique_to_current_global_[unique_id] = global_id;
+
+    // This should be done after updating unique_to_current_global_ in case one
+    // of our observers calls into GetLatestGlobalId().
+    for (auto& observer : global_id_change_observers_) {
+      observer.Run(old_global_id, global_id);
+    }
+  }
+
+  CleanupNavigationTracking();
+}
+
+void SessionsSyncManager::CleanupNavigationTracking() {
+  DCHECK(kNavigationTrackingCleanupThreshold >
+         kNavigationTrackingCleanupAmount);
+
+  // |global_to_unique_| is implicitly ordered by least recently created, which
+  //  means we can drop from the beginning.
+  if (global_to_unique_.size() > kNavigationTrackingCleanupThreshold) {

[Patrick Noland, 2017/06/28 22:44:37]

Can you use the range-based erase here? 

[skym, 2017/07/05 19:14:28]

Done.

+    for (int i = 0; i < kNavigationTrackingCleanupAmount; ++i) {
+      global_to_unique_.erase(global_to_unique_.begin());
+    }
+
+    // While |unique_id|s do get bigger for the most part, this isn't a great
+    // thing to make assumptions about, and an old tab may get refreshed often
+    // and still be very important. So instead just delete anything that's
+    // orphaned from |global_to_unique_|.
+    auto u2g_iter = unique_to_current_global_.begin();
+    while (u2g_iter != unique_to_current_global_.end()) {

[Patrick Noland, 2017/06/28 22:44:37]

I think you can use base::EraseIf here

[skym, 2017/07/05 19:14:28]

Done.

+      if (global_to_unique_.find(u2g_iter->second) == global_to_unique_.end()) {
+        u2g_iter = unique_to_current_global_.erase(u2g_iter);
+      } else {
+        ++u2g_iter;
+      }
+    }
+  }
+}
+
 };  // namespace sync_sessions