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

components/sync_sessions/sessions_sync_manager.cc

 // Copyright 2014 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 "components/sync_sessions/sessions_sync_manager.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/format_macros.h"
@@ skipping 33 matching lines @@
 const int kMaxSyncNavigationCount = 6;
 
 // The URL at which the set of synced tabs is displayed. We treat it differently
 // from all other URL's as accessing it triggers a sync refresh of Sessions.
 const char kNTPOpenTabSyncURL[] = "chrome://newtab/#open_tabs";
 
 // Default number of days without activity after which a session is considered
 // 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,
                            const sessions::SessionTab* t2) {
   return t1->timestamp > t2->timestamp;
 }
 
 // Comparator function for use with std::sort that will sort sessions by
 // descending modified_time (i.e., most recent first).
 bool SessionsRecencyComparator(const SyncedSession* s1,
@@ skipping 507 matching lines @@
     GURL virtual_url =
       modified_tab->GetVirtualURLAtIndex(modified_tab->GetCurrentEntryIndex());
     if (virtual_url.is_valid() &&
         virtual_url.spec() == kNTPOpenTabSyncURL) {
       DVLOG(1) << "Triggering sync refresh for sessions datatype.";
       if (!datatype_refresh_callback_.is_null())
         datatype_refresh_callback_.Run();
     }
   }
 
+  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.
     bool rebuild_association_succeeded = RebuildAssociations();
     DCHECK(!rebuild_association_succeeded || !local_tab_pool_out_of_sync_);
     return;
   }
 
   syncer::SyncChangeList changes;
   AssociateTab(modified_tab, &changes);
@@ skipping 687 matching lines @@
       DVLOG(1) << "Found stale session " << session_tag << " with age "
                << session_age_in_days << ", deleting.";
       DeleteForeignSessionInternal(session_tag, &changes);
     }
   }
 
   if (!changes.empty())
     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) {
   return syncer::GenerateSyncableHash(syncer::SESSIONS,
                                       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