Avoid parallel maps in SyncedSessionTracker

No behavioral differences: a new struct (TrackedSession) is introduced
to represent what the tracker knows about a session, which is an
extended version of SyncedSession.

The set |tab_node_ids| is moved away from SyncedSession to the new
struct, because there's nothing that depends on it except the tracker
itself.

Somewhat unrelated, minor changes are introduced in the API as have
been suggested in code reviews.

Bug: 681921
Change-Id: Ife4c215b65dfbeb97d5c70fd1f984986b9938a8f
Reviewed-on: https://chromium-review.googlesource.com/983773
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546755}

components/sync_sessions/local_session_event_handler_impl.cc

@@ -245,9 +245,9 @@ void LocalSessionEventHandlerImpl::AssociateWindows(ReloadTabsOption option,
       // the OnLocalTabModified method invoking AssociateTab directly.
       // Therefore, we can key whether this window has valid tabs based on
       // the tab's presence in the tracker.
-      const sessions::SessionTab* tab = nullptr;
-      if (session_tracker_->LookupSessionTab(current_session_tag_, tab_id,
-                                             &tab)) {
+      const sessions::SessionTab* tab =
+          session_tracker_->LookupSessionTab(current_session_tag_, tab_id);
+      if (tab) {
         found_tabs = true;
 
         // Update this window's representation in the synced session tracker.

components/sync_sessions/open_tabs_ui_delegate_impl.cc

@@ -50,11 +50,10 @@ bool OpenTabsUIDelegateImpl::GetSyncedFaviconForPageURL(
 
 bool OpenTabsUIDelegateImpl::GetAllForeignSessions(
     std::vector<const SyncedSession*>* sessions) {
-  if (!session_tracker_->LookupAllForeignSessions(
-          sessions, SyncedSessionTracker::PRESENTABLE))
-    return false;
+  *sessions = session_tracker_->LookupAllForeignSessions(
+      SyncedSessionTracker::PRESENTABLE);
   std::sort(sessions->begin(), sessions->end(), SessionsRecencyComparator);
-  return true;
+  return !sessions->empty();
 }
 
 bool OpenTabsUIDelegateImpl::GetForeignSession(
@@ -66,11 +65,8 @@ bool OpenTabsUIDelegateImpl::GetForeignSession(
 bool OpenTabsUIDelegateImpl::GetForeignTab(const std::string& tag,
                                            const SessionID tab_id,
                                            const sessions::SessionTab** tab) {
-  const sessions::SessionTab* synced_tab = nullptr;
-  bool success = session_tracker_->LookupSessionTab(tag, tab_id, &synced_tab);
-  if (success)
-    *tab = synced_tab;
-  return success;
+  *tab = session_tracker_->LookupSessionTab(tag, tab_id);
+  return *tab != nullptr;
 }
 
 bool OpenTabsUIDelegateImpl::GetForeignSessionTabs(
@@ -106,7 +102,8 @@ void OpenTabsUIDelegateImpl::DeleteForeignSession(const std::string& tag) {
 
 bool OpenTabsUIDelegateImpl::GetLocalSession(
     const SyncedSession** local_session) {
-  return session_tracker_->LookupLocalSession(local_session);
+  *local_session = session_tracker_->LookupLocalSession();
+  return *local_session != nullptr;
 }
 
 }  // namespace sync_sessions

components/sync_sessions/sessions_sync_manager.cc

@@ -277,8 +277,8 @@ void SessionsSyncManager::StopSyncing(syncer::ModelType type) {
 syncer::SyncDataList SessionsSyncManager::GetAllSyncData(
     syncer::ModelType type) const {
   syncer::SyncDataList list;
-  const SyncedSession* session = nullptr;
-  if (!session_tracker_.LookupLocalSession(&session))
+  const SyncedSession* session = session_tracker_.LookupLocalSession();
+  if (!session)
     return syncer::SyncDataList();
 
   // First construct the header node.
@@ -524,10 +524,8 @@ bool SessionsSyncManager::InitFromSyncModel(
 
   // Cleanup all foreign sessions, since orphaned tabs may have been added after
   // the header.
-  std::vector<const SyncedSession*> sessions;
-  session_tracker_.LookupAllForeignSessions(&sessions,
-                                            SyncedSessionTracker::RAW);
-  for (const auto* session : sessions) {
+  for (const auto* session :
+       session_tracker_.LookupAllForeignSessions(SyncedSessionTracker::RAW)) {
     session_tracker_.CleanupSession(session->session_tag);
   }
 
@@ -624,15 +622,11 @@ OpenTabsUIDelegate* SessionsSyncManager::GetOpenTabsUIDelegate() {
 }
 
 void SessionsSyncManager::DoGarbageCollection() {
-  std::vector<const SyncedSession*> sessions;
-  if (!session_tracker_.LookupAllForeignSessions(&sessions,
-                                                 SyncedSessionTracker::RAW))
-    return;  // No foreign sessions.
-
   // Iterate through all the sessions and delete any with age older than
   // |stale_session_threshold_days_|.
   syncer::SyncChangeList changes;
-  for (const auto* session : sessions) {
+  for (const auto* session :
+       session_tracker_.LookupAllForeignSessions(SyncedSessionTracker::RAW)) {
     int session_age_in_days =
         (base::Time::Now() - session->modified_time).InDays();
     if (session_age_in_days > stale_session_threshold_days_) {

components/sync_sessions/sessions_sync_manager_unittest.cc

@@ -30,6 +30,7 @@ using syncer::SyncDataList;
 using syncer::SyncDataLocal;
 using syncer::SyncError;
 using testing::Eq;
+using testing::IsNull;
 
 namespace sync_sessions {
 
@@ -1132,11 +1133,12 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithShadowing) {
   output.clear();
 
   // Verify that cleanup post-merge cleanup correctly removes all tabs objects.
-  const sessions::SessionTab* tab;
-  ASSERT_FALSE(manager()->session_tracker_.LookupSessionTab(session_tag,
-                                                            kTabIds1[0], &tab));
-  ASSERT_FALSE(manager()->session_tracker_.LookupSessionTab(session_tag,
-                                                            kTabIds1[1], &tab));
+  ASSERT_THAT(
+      manager()->session_tracker_.LookupSessionTab(session_tag, kTabIds1[0]),
+      IsNull());
+  ASSERT_THAT(
+      manager()->session_tracker_.LookupSessionTab(session_tag, kTabIds1[1]),
+      IsNull());
 
   std::set<int> tab_node_ids;
   manager()->session_tracker_.LookupForeignTabNodeIds(session_tag,
@@ -1333,9 +1335,9 @@ TEST_F(SessionsSyncManagerTest, MergeDeletesBadHash) {
   EXPECT_EQ(1U, CountIfTagMatches(output, bad_header_tag));
   EXPECT_EQ(1U, CountIfTagMatches(output, bad_tab_tag));
 
-  std::vector<const SyncedSession*> sessions;
+  const std::vector<const SyncedSession*> sessions =
       manager()->session_tracker_.LookupAllForeignSessions(
-      &sessions, SyncedSessionTracker::RAW);
+          SyncedSessionTracker::RAW);
   ASSERT_EQ(2U, sessions.size());
   EXPECT_EQ(1U, CountIfTagMatches(sessions, good_header_tag));
   EXPECT_EQ(1U, CountIfTagMatches(sessions, good_tag_tab));

components/sync_sessions/synced_session.h

@@ -59,17 +59,6 @@ struct SyncedSession {
   // Map of windows that make up this session.
   std::map<SessionID, std::unique_ptr<SyncedSessionWindow>> windows;
 
-  // A tab node id is part of the identifier for the sync tab objects. Tab node
-  // ids are not used for interacting with the model/browser tabs. However, when
-  // when we want to delete a foreign session, we use these values to inform
-  // sync which tabs to delete. We are extracting these tab node ids from
-  // individual session (tab, not header) specifics, but store them here in the
-  // SyncedSession during runtime. We do this because tab node ids may be reused
-  // for different tabs, and tracking which tab id is currently associated with
-  // each tab node id is both difficult and unnecessary. See comments at
-  // SyncedSessionTracker::GetTabImpl for a concrete example of id reuse.
-  std::set<int> tab_node_ids;
-
   // Convert this object to its protocol buffer equivalent. Shallow conversion,
   // does not create SessionTab protobufs.
   sync_pb::SessionHeader ToSessionHeaderProto() const;

components/sync_sessions/synced_session_tracker.h

@@ -46,12 +46,10 @@ class SyncedSessionTracker {
 
   // **** Synced session/tab query methods. ****
 
-  // Fill a preallocated vector with all foreign sessions we're tracking (skips
-  // the local session object). SyncedSession ownership remains within the
-  // SyncedSessionTracker. Lookup parameter is used to decide which foreign tabs
-  // should be include.
-  // Returns true if we had foreign sessions to fill it with, false otherwise.
-  bool LookupAllForeignSessions(std::vector<const SyncedSession*>* sessions,
+  // Returns all foreign sessions we're tracking (skips the local session
+  // object). SyncedSession ownership remains within the SyncedSessionTracker.
+  // Lookup parameter is used to decide which foreign tabs should be include.
+  std::vector<const SyncedSession*> LookupAllForeignSessions(
       SessionLookup lookup) const;
 
   // Fills |tab_node_ids| with the tab node ids (see GetTab) for all the tabs*
@@ -72,17 +70,13 @@ class SyncedSessionTracker {
 
   // Attempts to look up the tab associated with the given tag and tab id.
   // Ownership of the SessionTab remains within the SyncedSessionTracker.
-  // If lookup succeeds:
-  // - Sets tab to point to the SessionTab, and returns true.
-  // Else
-  // - Returns false, tab is set to null.
-  bool LookupSessionTab(const std::string& session_tag,
-                        SessionID tab_id,
-                        const sessions::SessionTab** tab) const;
+  // Returns null if lookup fails.
+  const sessions::SessionTab* LookupSessionTab(const std::string& session_tag,
+                                               SessionID tab_id) const;
 
   // Allows retrieval of existing data for the local session. Unlike GetSession
-  // this won't create-if-not-present.
-  bool LookupLocalSession(const SyncedSession** output) const;
+  // this won't create-if-not-present and will return null instead.
+  const SyncedSession* LookupLocalSession() const;
 
   // **** Methods for manipulating synced sessions and tabs. ****
 
@@ -193,18 +187,16 @@ class SyncedSessionTracker {
   // tracking structures.
   void Clear();
 
-  bool Empty() const {
-    return synced_tab_map_.empty() && synced_session_map_.empty();
-  }
+  bool Empty() const { return session_map_.empty(); }
 
   // Includes both foreign sessions and the local session.
-  size_t num_synced_sessions() const { return synced_session_map_.size(); }
+  size_t num_synced_sessions() const { return session_map_.size(); }
 
   // Returns the number of tabs associated with the specified session tag.
   size_t num_synced_tabs(const std::string& session_tag) const {
-    auto iter = synced_tab_map_.find(session_tag);
-    if (iter != synced_tab_map_.end()) {
-      return iter->second.size();
+    auto iter = session_map_.find(session_tag);
+    if (iter != session_map_.end()) {
+      return iter->second.synced_tab_map.size();
     } else {
       return 0;
     }
@@ -213,45 +205,61 @@ class SyncedSessionTracker {
   // Returns whether a tab is unmapped or not.
   bool IsTabUnmappedForTesting(SessionID tab_id);
 
+  std::set<int> GetTabNodeIdsForTesting(const std::string& session_tag) const;
+
  private:
   friend class SessionsSyncManagerTest;
   friend class SyncedSessionTrackerTest;
 
+  struct TrackedSession {
+    TrackedSession();
+    ~TrackedSession();
+
+    // Owns the SyncedSessions, and transitively, all of the windows and tabs
+    // they contain.
+    SyncedSession synced_session;
+
+    // The mapping of tab/window to their SessionTab/SessionWindow objects.
+    // The SessionTab/SessionWindow objects referred to may be owned either by
+    // the session in the |synced_session| or be temporarily unmapped and live
+    // in the |unmapped_tabs|/|unmapped_windows| collections.
+    std::map<SessionID, sessions::SessionTab*> synced_tab_map;
+    std::map<SessionID, SyncedSessionWindow*> synced_window_map;
+
+    // The collection of tabs/windows not owned by SyncedSession. This is the
+    // case either because 1. (in the case of tabs) they were newly created by
+    // GetTab() and not yet added to a session, or 2. they were removed from
+    // their owning session by a call to ResetSessionTracking() and not yet
+    // added back.
+    std::map<SessionID, std::unique_ptr<sessions::SessionTab>> unmapped_tabs;
+    std::map<SessionID, std::unique_ptr<SyncedSessionWindow>> unmapped_windows;
+
+    // A tab node id is part of the identifier for the sync tab objects. Tab
+    // node ids are not used for interacting with the model/browser tabs.
+    // However, when when we want to delete a foreign session, we use these
+    // values to inform sync which tabs to delete. We are extracting these tab
+    // node ids from individual session (tab, not header) specifics, but store
+    // them here during runtime. We do this because tab node ids may be reused
+    // for different tabs, and tracking which tab id is currently associated
+    // with each tab node id is both difficult and unnecessary.
+    std::set<int> tab_node_ids;
+  };
+
+  // LookupTrackedSession() returns null if the session tag is unknown.
+  const TrackedSession* LookupTrackedSession(
+      const std::string& session_tag) const;
+  TrackedSession* LookupTrackedSession(const std::string& session_tag);
+  // Creates tracked session if it wasn't known previously. Never returns null.
+  TrackedSession* GetTrackedSession(const std::string& session_tag);
+
   // Implementation of CleanupForeignSession/CleanupLocalTabs.
   void CleanupSessionImpl(const std::string& session_tag);
 
   // The client of the sync sessions datatype.
   SyncSessionsClient* const sessions_client_;
 
-  // The mapping of tab/window to their SessionTab/SessionWindow objects.
-  // The SessionTab/SessionWindow objects referred to may be owned either by the
-  // session in the |synced_session_map_| or be temporarily unmapped and live in
-  // the |unmapped_tabs_|/|unmapped_windows_| collections.
-  //
-  // Map: session tag -> (tab/window -> SessionTab*/SessionWindow*)
-  std::map<std::string, std::map<SessionID, sessions::SessionTab*>>
-      synced_tab_map_;
-  std::map<std::string, std::map<SessionID, SyncedSessionWindow*>>
-      synced_window_map_;
-
-  // The collection that owns the SyncedSessions, and transitively, all of the
-  // windows and tabs they contain.
-  //
-  // Map: session tag -> owned SyncedSession
-  std::map<std::string, std::unique_ptr<SyncedSession>> synced_session_map_;
-
-  // The collection of tabs/windows not owned by a SyncedSession. This is the
-  // case either because 1. (in the case of tabs) they were newly created by
-  // GetTab() and not yet added to a session, or 2. they were removed from their
-  // owning session by a call to ResetSessionTracking() and not yet added back.
-  //
-  // Map: session tag -> (tab/window id -> owned SessionTab/SessionWindow)
-  std::map<std::string,
-           std::map<SessionID, std::unique_ptr<sessions::SessionTab>>>
-      unmapped_tabs_;
-  std::map<std::string,
-           std::map<SessionID, std::unique_ptr<SyncedSessionWindow>>>
-      unmapped_windows_;
+  // Map: session tag -> TrackedSession.
+  std::map<std::string, TrackedSession> session_map_;
 
   // The tag for this machine's local session, so we can distinguish the foreign
   // sessions.