Introduce SessionStore for migrating sessions to USS

The class is a key piece in the design doc ([1], accessible for Google
employees only) and will be used by the future USS bridge, to be
introduced in follow-up patches.

Ideally, SessionStore is an abstraction on top of SyncedSessionTracker
that in addition supports [de]serialization to/from disk. In the current
form, in diverges slightly from the design doc because direct mutable
access to the underlying SyncedSessionTracker is exposed. TODOs have
been added to address this in the future.

[1] https://docs.google.com/document/d/1UGM1yhHznmXvfeGvckdzBcrE6GLpgk7LsLmgPhFcIdk/edit#

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

components/sync/base/sync_prefs.cc

@@ -16,6 +16,8 @@
 
 namespace syncer {
 
+SessionSyncPrefs::~SessionSyncPrefs() {}
+
 SyncPrefObserver::~SyncPrefObserver() {}
 
 SyncPrefs::SyncPrefs(PrefService* pref_service) : pref_service_(pref_service) {

components/sync/base/sync_prefs.h

@@ -40,6 +40,14 @@ class SyncPrefObserver {
   virtual ~SyncPrefObserver();
 };
 
+// Use this for the unique machine tag used for session sync.
+class SessionSyncPrefs {
+ public:
+  virtual ~SessionSyncPrefs();
+  virtual std::string GetSyncSessionsGUID() const = 0;
+  virtual void SetSyncSessionsGUID(const std::string& guid) = 0;
+};
+
 // SyncPrefs is a helper class that manages getting, setting, and
 // persisting global sync preferences.  It is not thread-safe, and
 // lives on the UI thread.
@@ -53,7 +61,8 @@ class SyncPrefObserver {
 //   sync_setup_wizard.cc
 //   sync_setup_wizard_unittest.cc
 //   two_client_preferences_sync_test.cc
-class SyncPrefs : public base::SupportsWeakPtr<SyncPrefs> {
+class SyncPrefs : public SessionSyncPrefs,
+                  public base::SupportsWeakPtr<SyncPrefs> {
  public:
   // |pref_service| may not be null.
   // Does not take ownership of |pref_service|.
@@ -62,7 +71,7 @@ class SyncPrefs : public base::SupportsWeakPtr<SyncPrefs> {
   // For testing.
   SyncPrefs();
 
-  virtual ~SyncPrefs();
+  ~SyncPrefs() override;
 
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
 
@@ -118,8 +127,8 @@ class SyncPrefs : public base::SupportsWeakPtr<SyncPrefs> {
   void SetKeystoreEncryptionBootstrapToken(const std::string& token);
 
   // Use this for the unique machine tag used for session sync.
-  std::string GetSyncSessionsGUID() const;
-  void SetSyncSessionsGUID(const std::string& guid);
+  std::string GetSyncSessionsGUID() const override;
+  void SetSyncSessionsGUID(const std::string& guid) override;
 
   // Maps |type| to its corresponding preference name.
   static const char* GetPrefNameForDataType(ModelType type);

components/sync/model/entity_data.cc

@@ -5,8 +5,10 @@
 #include "components/sync/model/entity_data.h"
 
 #include <algorithm>
+#include <ostream>
 #include <utility>
 
+#include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -28,22 +30,34 @@ std::string UniquePositionToString(
 }  // namespace
 
 EntityData::EntityData() = default;
-EntityData::~EntityData() = default;
-EntityData::EntityData(const EntityData& src) = default;
 
-void EntityData::Swap(EntityData* other) {
-  id.swap(other->id);
-  client_tag_hash.swap(other->client_tag_hash);
-  non_unique_name.swap(other->non_unique_name);
+EntityData::EntityData(EntityData&& other)
+    : id(std::move(other.id)),
+      client_tag_hash(std::move(other.client_tag_hash)),
+      non_unique_name(std::move(other.non_unique_name)),
+      creation_time(other.creation_time),
+      modification_time(other.modification_time),
+      parent_id(std::move(other.parent_id)),
+      is_folder(other.is_folder) {
+  specifics.Swap(&other.specifics);
+  unique_position.Swap(&other.unique_position);
+}
 
-  specifics.Swap(&other->specifics);
+EntityData::EntityData(const EntityData& src) = default;
 
-  std::swap(creation_time, other->creation_time);
-  std::swap(modification_time, other->modification_time);
+EntityData::~EntityData() = default;
 
-  parent_id.swap(other->parent_id);
-  std::swap(is_folder, other->is_folder);
-  unique_position.Swap(&other->unique_position);
+EntityData& EntityData::operator=(EntityData&& other) {
+  id = std::move(other.id);
+  client_tag_hash = std::move(other.client_tag_hash);
+  non_unique_name = std::move(other.non_unique_name);
+  creation_time = other.creation_time;
+  modification_time = other.modification_time;
+  parent_id = other.parent_id;
+  is_folder = other.is_folder;
+  specifics.Swap(&other.specifics);
+  unique_position.Swap(&other.unique_position);
+  return *this;
 }
 
 EntityDataPtr EntityData::PassToPtr() {
@@ -106,7 +120,7 @@ size_t EntityData::EstimateMemoryUsage() const {
 }
 
 void EntityDataTraits::SwapValue(EntityData* dest, EntityData* src) {
-  dest->Swap(src);
+  std::swap(*dest, *src);
 }
 
 bool EntityDataTraits::HasValue(const EntityData& value) {
@@ -118,4 +132,13 @@ const EntityData& EntityDataTraits::DefaultValue() {
   return default_instance;
 }
 
+void PrintTo(const EntityData& entity_data, std::ostream* os) {
+  std::string specifics;
+  base::JSONWriter::WriteWithOptions(
+      *syncer::EntitySpecificsToValue(entity_data.specifics),
+      base::JSONWriter::OPTIONS_PRETTY_PRINT, &specifics);
+  *os << "{ id: '" << entity_data.id << "', client_tag_hash: '"
+      << entity_data.client_tag_hash << "', specifics: " << specifics << "}";
+}
+
 }  // namespace syncer

components/sync/model/entity_data.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_SYNC_MODEL_ENTITY_DATA_H_
 #define COMPONENTS_SYNC_MODEL_ENTITY_DATA_H_
 
+#include <iosfwd>
 #include <map>
 #include <memory>
 #include <string>
@@ -36,8 +37,11 @@ using EntityDataList = std::vector<EntityDataPtr>;
 struct EntityData {
  public:
   EntityData();
+  EntityData(EntityData&&);
   ~EntityData();
 
+  EntityData& operator=(EntityData&&);
+
   // Typically this is a server assigned sync ID, although for a local change
   // that represents a new entity this field might be either empty or contain
   // a temporary client sync ID.
@@ -96,10 +100,6 @@ struct EntityData {
   size_t EstimateMemoryUsage() const;
 
  private:
-  friend struct EntityDataTraits;
-  // Used to transfer the data without copying.
-  void Swap(EntityData* other);
-
   // Allow copy ctor so that UpdateId and UpdateSpecifics can make a copy of
   // this EntityData.
   EntityData(const EntityData& src);
@@ -107,6 +107,9 @@ struct EntityData {
   DISALLOW_ASSIGN(EntityData);
 };
 
+// gMock printer helper.
+void PrintTo(const EntityData& entity_data, std::ostream* os);
+
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_MODEL_ENTITY_DATA_H_

components/sync/model/metadata_batch.cc

@@ -11,7 +11,11 @@ namespace syncer {
 MetadataBatch::MetadataBatch() {}
 MetadataBatch::~MetadataBatch() {}
 
-EntityMetadataMap&& MetadataBatch::TakeAllMetadata() {
+const EntityMetadataMap& MetadataBatch::GetAllMetadata() const {
+  return metadata_map_;
+}
+
+EntityMetadataMap MetadataBatch::TakeAllMetadata() {
   return std::move(metadata_map_);
 }
 

components/sync/model/metadata_batch.h

@@ -20,12 +20,15 @@ using EntityMetadataMap = std::map<std::string, sync_pb::EntityMetadata>;
 class MetadataBatch {
  public:
   MetadataBatch();
-  virtual ~MetadataBatch();
+  ~MetadataBatch();
+
+  // Read-only access to the entire metadata map.
+  const EntityMetadataMap& GetAllMetadata() const;
 
   // Allows the caller to take ownership of the entire metadata map. This is
   // done because the caller will probably swap out all the EntityMetadata
   // protos from the map for performance reasons.
-  EntityMetadataMap&& TakeAllMetadata();
+  EntityMetadataMap TakeAllMetadata();
 
   // Add |metadata| for |storage_key| to the batch.
   void AddMetadata(const std::string& storage_key,

components/sync/model/model_error.h

@@ -39,6 +39,8 @@ class ModelError {
 // Typedef for a simple error handler callback.
 using ModelErrorHandler = base::RepeatingCallback<void(const ModelError&)>;
 
+using OnceModelErrorHandler = base::OnceCallback<void(const ModelError&)>;
+
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_MODEL_MODEL_ERROR_H_

components/sync/model/model_type_store_test_util.cc

@@ -12,6 +12,45 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace syncer {
+namespace {
+
+// Implementation of ModelTypeStore that delegates all calls to another
+// instance, as injected in the constructor, useful for APIs that take ownership
+// of ModelTypeStore.
+class ForwardingModelTypeStore : public ModelTypeStore {
+ public:
+  explicit ForwardingModelTypeStore(ModelTypeStore* other) : other_(other) {}
+
+  void ReadData(const IdList& id_list, ReadDataCallback callback) override {
+    other_->ReadData(id_list, std::move(callback));
+  }
+
+  void ReadAllData(ReadAllDataCallback callback) override {
+    other_->ReadAllData(std::move(callback));
+  }
+
+  void ReadAllMetadata(ReadMetadataCallback callback) override {
+    other_->ReadAllMetadata(std::move(callback));
+  }
+
+  std::unique_ptr<WriteBatch> CreateWriteBatch() override {
+    return other_->CreateWriteBatch();
+  }
+
+  void CommitWriteBatch(std::unique_ptr<WriteBatch> write_batch,
+                        CallbackWithResult callback) override {
+    other_->CommitWriteBatch(std::move(write_batch), std::move(callback));
+  }
+
+  void DeleteAllDataAndMetadata(CallbackWithResult callback) override {
+    other_->DeleteAllDataAndMetadata(std::move(callback));
+  }
+
+ private:
+  ModelTypeStore* other_;
+};
+
+}  // namespace
 
 // static
 std::unique_ptr<ModelTypeStore>
@@ -57,4 +96,17 @@ void ModelTypeStoreTestUtil::MoveStoreToCallback(
   std::move(callback).Run(/*error=*/base::nullopt, std::move(store));
 }
 
+// static
+RepeatingModelTypeStoreFactory
+ModelTypeStoreTestUtil::FactoryForForwardingStore(ModelTypeStore* target) {
+  return base::BindRepeating(
+      [](ModelTypeStore* target, ModelType,
+         ModelTypeStore::InitCallback callback) {
+        std::move(callback).Run(
+            /*error=*/base::nullopt,
+            std::make_unique<ForwardingModelTypeStore>(target));
+      },
+      base::Unretained(target));
+}
+
 }  // namespace syncer

components/sync/model/model_type_store_test_util.h

@@ -28,6 +28,12 @@ class ModelTypeStoreTestUtil {
   static void MoveStoreToCallback(std::unique_ptr<ModelTypeStore> store,
                                   ModelType type,
                                   ModelTypeStore::InitCallback callback);
+
+  // Returns a callback that constructs a store that forwards all calls to
+  // |target|. |*target| must outlive the returned factory as well any store
+  // created by the factory.
+  static RepeatingModelTypeStoreFactory FactoryForForwardingStore(
+      ModelTypeStore* target);
 };
 
 }  // namespace syncer

components/sync_sessions/BUILD.gn

@@ -17,6 +17,8 @@ static_library("sync_sessions") {
     "open_tabs_ui_delegate_impl.h",
     "session_data_type_controller.cc",
     "session_data_type_controller.h",
+    "session_store.cc",
+    "session_store.h",
     "sessions_global_id_mapper.cc",
     "sessions_global_id_mapper.h",
     "sessions_sync_manager.cc",
@@ -90,6 +92,7 @@ source_set("unit_tests") {
     "lost_navigations_recorder_unittest.cc",
     "open_tabs_ui_delegate_impl_unittest.cc",
     "session_data_type_controller_unittest.cc",
+    "session_store_unittest.cc",
     "sessions_global_id_mapper_unittest.cc",
     "sessions_sync_manager_unittest.cc",
     "synced_session_tracker_unittest.cc",

components/sync_sessions/session_store.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_SYNC_SESSIONS_SESSION_STORE_H_
+#define COMPONENTS_SYNC_SESSIONS_SESSION_STORE_H_
+
+#include <map>
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "base/callback_forward.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/time/time.h"
+#include "components/sync/device_info/local_device_info_provider.h"
+#include "components/sync/model/data_batch.h"
+#include "components/sync/model/model_error.h"
+#include "components/sync/model/model_type_store.h"
+#include "components/sync_sessions/synced_session_tracker.h"
+
+namespace syncer {
+class SessionSyncPrefs;
+}  // namespace syncer
+
+namespace sync_sessions {
+
+// Class responsible for maintaining an in-memory representation of sync
+// sessions (by owning a SyncedSessionTracker) with the capability to persist
+// state to disk and restore (data and metadata). The API enforces a valid and
+// consistent state of the model, e.g. by making sure there is at most one sync
+// entity per client tag.
+class SessionStore {
+ public:
+  struct SessionInfo {
+    std::string session_tag;
+    std::string client_name;
+    sync_pb::SyncEnums::DeviceType device_type = sync_pb::SyncEnums::TYPE_UNSET;
+  };
+
+  // Creation factory. The instantiation process is quite complex because it
+  // loads state from disk in addition to other asynchronous dependencies like
+  // LocalDeviceInfoProvider.
+  using FactoryCompletionCallback = base::OnceCallback<void(
+      const base::Optional<syncer::ModelError>& error,
+      std::unique_ptr<SessionStore> store,
+      std::unique_ptr<syncer::MetadataBatch> metadata_batch)>;
+  using Factory =
+      base::RepeatingCallback<void(FactoryCompletionCallback callback)>;
+  // Mimics signature of FaviconCache::UpdateMappingsFromForeignTab().
+  using RestoredForeignTabCallback =
+      base::RepeatingCallback<void(const sync_pb::SessionTab&, base::Time)>;
+
+  // Creates a factory object that is capable of constructing instances of type
+  // |SessionStore| and handling the involved IO. All pointer arguments must not
+  // be null and must outlive the factory as well as the instantiated stores.
+  static Factory CreateFactory(
+      SyncSessionsClient* sessions_client,
+      syncer::SessionSyncPrefs* sync_prefs,
+      syncer::LocalDeviceInfoProvider* local_device_info_provider,
+      const syncer::RepeatingModelTypeStoreFactory& store_factory,
+      const RestoredForeignTabCallback& restored_foreign_tab_callback);
+
+  // Verifies whether a proto is malformed (e.g. required fields are missing).
+  static bool AreValidSpecifics(const sync_pb::SessionSpecifics& specifics);
+  // |specifics| must be valid, see AreValidSpecifics().
+  static std::string GetClientTag(const sync_pb::SessionSpecifics& specifics);
+  // |specifics| must be valid, see AreValidSpecifics().
+  static std::string GetStorageKey(const sync_pb::SessionSpecifics& specifics);
+
+  // Various overloads for testing.
+  static std::string GetHeaderStorageKeyForTest(const std::string& session_tag);
+  static std::string GetTabStorageKeyForTest(const std::string& session_tag,
+                                             int tab_node_id);
+
+  // Similar to ModelTypeStore::WriteBatch but enforces a consistent state. In
+  // the current implementation, some functions do *NOT* update the tracker, so
+  // callers are responsible for doing so.
+  // TODO(crbug.com/681921): Enforce consistency between in-memory and persisted
+  // data by always updating the tracker.
+  class WriteBatch {
+   public:
+    // Callback that mimics the signature of ModelTypeStore::CommitWriteBatch().
+    using CommitCallback = base::OnceCallback<void(
+        std::unique_ptr<syncer::ModelTypeStore::WriteBatch>,
+        syncer::ModelTypeStore::CallbackWithResult)>;
+
+    // Raw pointers must not be nullptr and must outlive this object.
+    WriteBatch(std::unique_ptr<syncer::ModelTypeStore::WriteBatch> batch,
+               CommitCallback commit_cb,
+               syncer::OnceModelErrorHandler error_handler,
+               SyncedSessionTracker* session_tracker);
+    ~WriteBatch();
+
+    // Mutations return a storage key.
+    std::string PutAndUpdateTracker(const sync_pb::SessionSpecifics& specifics,
+                                    base::Time modification_time);
+    std::string DeleteForeignEntityAndUpdateTracker(
+        const sync_pb::SessionSpecifics& specifics);
+    // The functions below do not update SyncedSessionTracker and hence it is
+    // the caller's responsibility to do so *before* calling these functions.
+    std::string PutWithoutUpdatingTracker(
+        const sync_pb::SessionSpecifics& specifics);
+    std::string DeleteLocalTabWithoutUpdatingTracker(int tab_node_id);
+
+    syncer::MetadataChangeList* GetMetadataChangeList();
+
+    static void Commit(std::unique_ptr<WriteBatch> batch);
+
+   private:
+    std::unique_ptr<syncer::ModelTypeStore::WriteBatch> batch_;
+    CommitCallback commit_cb_;
+    syncer::OnceModelErrorHandler error_handler_;
+    SyncedSessionTracker* const session_tracker_;
+
+    DISALLOW_COPY_AND_ASSIGN(WriteBatch);
+  };
+
+  // Construction once all data and metadata has been loaded from disk. Use
+  // the factory above to take care of the IO. |sessions_client| must not be
+  // null and must outlive this object.
+  SessionStore(SyncSessionsClient* sessions_client,
+               const SessionInfo& local_session_info,
+               std::unique_ptr<syncer::ModelTypeStore> store,
+               std::map<std::string, sync_pb::SessionSpecifics> initial_data,
+               const syncer::EntityMetadataMap& initial_metadata,
+               const RestoredForeignTabCallback& restored_foreign_tab_callback);
+  ~SessionStore();
+
+  const SessionInfo& local_session_info() const { return local_session_info_; }
+
+  // Converts the in-memory model (SyncedSessionTracker) of the local session to
+  // sync protos. |storage_keys| must correspond to valid local session
+  // entities.
+  std::unique_ptr<syncer::DataBatch> GetLocalSessionDataForKeys(
+      const std::vector<std::string>& storage_keys) const;
+
+  // Returns all known local session entities, generated from the in-memory
+  // model (SyncedSessionTracker).
+  // TODO(crbug.com/681921): Implement the retrieval of foreign sessions too.
+  // Right now, the issue is that SyncedSessionTracker does *NOT* maintain a
+  // mapping between tab IDs and tab node IDs for foreign sessions, and hence
+  // it is impossible to compute storage keys.
+  std::unique_ptr<syncer::DataBatch> GetAllLocalSessionData() const;
+
+  // Write API. WriteBatch instances must not outlive this store and must be
+  // committed prior to destruction. Besides, more than one uncommitted
+  // instance must not exist at any time.
+  std::unique_ptr<WriteBatch> CreateWriteBatch(
+      syncer::OnceModelErrorHandler error_handler);
+  void DeleteAllDataAndMetadata();
+
+  // TODO(crbug.com/681921): Avoid exposing a mutable tracker, because that
+  // bypasses the consistency-enforcing API.
+  SyncedSessionTracker* mutable_tracker() { return &session_tracker_; }
+  const SyncedSessionTracker* tracker() const { return &session_tracker_; }
+
+ private:
+  // In charge of actually persisting changes to disk.
+  std::unique_ptr<syncer::ModelTypeStore> store_;
+
+  const SessionInfo local_session_info_;
+
+  SyncedSessionTracker session_tracker_;
+
+  base::WeakPtrFactory<SessionStore> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(SessionStore);
+};
+
+}  // namespace sync_sessions
+
+#endif  // COMPONENTS_SYNC_SESSIONS_SESSION_STORE_H_

components/sync_sessions/synced_session_tracker.cc

@@ -153,17 +153,14 @@ const std::string& SyncedSessionTracker::GetLocalSessionTag() const {
   return local_session_tag_;
 }
 
+std::vector<const SyncedSession*> SyncedSessionTracker::LookupAllSessions(
+    SessionLookup lookup) const {
+  return LookupSessions(lookup, /*exclude_local_session=*/false);
+}
+
 std::vector<const SyncedSession*>
 SyncedSessionTracker::LookupAllForeignSessions(SessionLookup lookup) const {
-  std::vector<const SyncedSession*> sessions;
-  for (const auto& session_pair : session_map_) {
-    const SyncedSession& foreign_session = session_pair.second.synced_session;
-    if (session_pair.first != local_session_tag_ &&
-        (lookup == RAW || IsPresentable(sessions_client_, foreign_session))) {
-      sessions.push_back(&foreign_session);
-    }
-  }
-  return sessions;
+  return LookupSessions(lookup, /*exclude_local_session=*/true);
 }
 
 bool SyncedSessionTracker::LookupSessionWindows(
@@ -205,6 +202,18 @@ std::set<int> SyncedSessionTracker::LookupTabNodeIds(
   return session ? session->tab_node_ids : std::set<int>();
 }
 
+std::vector<const sessions::SessionTab*>
+SyncedSessionTracker::LookupUnmappedTabs(const std::string& session_tag) const {
+  const TrackedSession* session = LookupTrackedSession(session_tag);
+  std::vector<const sessions::SessionTab*> unmapped_tabs;
+  if (session) {
+    for (const auto& unmapped_tab_entry : session->unmapped_tabs) {
+      unmapped_tabs.push_back(unmapped_tab_entry.second.get());
+    }
+  }
+  return unmapped_tabs;
+}
+
 const SyncedSession* SyncedSessionTracker::LookupLocalSession() const {
   return LookupSession(local_session_tag_);
 }
@@ -295,6 +304,23 @@ SyncedSessionTracker::TrackedSession* SyncedSessionTracker::GetTrackedSession(
   return session;
 }
 
+std::vector<const SyncedSession*> SyncedSessionTracker::LookupSessions(
+    SessionLookup lookup,
+    bool exclude_local_session) const {
+  std::vector<const SyncedSession*> sessions;
+  for (const auto& session_pair : session_map_) {
+    const SyncedSession& session = session_pair.second.synced_session;
+    if (lookup == PRESENTABLE && !IsPresentable(sessions_client_, session)) {
+      continue;
+    }
+    if (exclude_local_session && session_pair.first == local_session_tag_) {
+      continue;
+    }
+    sessions.push_back(&session);
+  }
+  return sessions;
+}
+
 void SyncedSessionTracker::CleanupSessionImpl(const std::string& session_tag) {
   TrackedSession* session = LookupTrackedSession(session_tag);
   if (!session)
@@ -471,6 +497,10 @@ void SyncedSessionTracker::CleanupLocalTabs(std::set<int>* deleted_node_ids) {
   }
 }
 
+int SyncedSessionTracker::LookupTabNodeFromLocalTabId(SessionID tab_id) const {
+  return local_tab_pool_.GetTabNodeIdFromTabId(tab_id);
+}
+
 bool SyncedSessionTracker::GetTabNodeFromLocalTabId(SessionID tab_id,
                                                     int* tab_node_id) {
   DCHECK(!local_session_tag_.empty());
@@ -481,19 +511,33 @@ bool SyncedSessionTracker::GetTabNodeFromLocalTabId(SessionID tab_id,
   // kept in sync and as consistent as possible.
   GetTab(local_session_tag_, tab_id);  // Ignore result.
 
-  bool reused_existing_tab =
-      local_tab_pool_.GetTabNodeForTab(tab_id, tab_node_id);
+  *tab_node_id = local_tab_pool_.GetTabNodeIdFromTabId(tab_id);
+  if (*tab_node_id != TabNodePool::kInvalidTabNodeID) {
+    DCHECK_NE(0U, GetTrackedSession(local_session_tag_)
+                      ->tab_node_ids.count(*tab_node_id));
+    return true;  // Reused existing tab.
+  }
+
+  // Could not reuse an existing tab so create a new one.
+  *tab_node_id = local_tab_pool_.AssociateWithFreeTabNode(tab_id);
   DCHECK_NE(TabNodePool::kInvalidTabNodeID, *tab_node_id);
-  GetTrackedSession(local_session_tag_)->tab_node_ids.insert(*tab_node_id);
+  // AssociateWithFreeTabNode() might have created a new tab node if none could
+  // be reused so make sure we register it in |tab_node_ids|.
+  bool reused_existing_tab = !GetTrackedSession(local_session_tag_)
+                                  ->tab_node_ids.insert(*tab_node_id)
+                                  .second;
   return reused_existing_tab;
 }
 
-bool SyncedSessionTracker::IsLocalTabNodeAssociated(int tab_node_id) {
-  if (tab_node_id == TabNodePool::kInvalidTabNodeID)
-    return false;
+bool SyncedSessionTracker::IsLocalTabNodeAssociated(int tab_node_id) const {
   return local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id).is_valid();
 }
 
+SessionID SyncedSessionTracker::LookupLocalTabIdFromTabNodeId(
+    int tab_node_id) const {
+  return local_tab_pool_.GetTabIdFromTabNodeId(tab_node_id);
+}
+
 void SyncedSessionTracker::ReassociateLocalTab(int tab_node_id,
                                                SessionID new_tab_id) {
   DCHECK(!local_session_tag_.empty());

components/sync_sessions/synced_session_tracker.h

@@ -46,6 +46,12 @@ class SyncedSessionTracker {
 
   // **** Synced session/tab query methods. ****
 
+  // Returns vector with all sessions we're tracking. SyncedSession ownership
+  // remains within the SyncedSessionTracker. Lookup parameter is used to decide
+  // which tabs should be included.
+  std::vector<const SyncedSession*> LookupAllSessions(
+      SessionLookup lookup) const;
+
   // 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.
@@ -56,6 +62,10 @@ class SyncedSessionTracker {
   // session having tag |session_tag|.
   std::set<int> LookupTabNodeIds(const std::string& session_tag) const;
 
+  // Returns tabs that are unmapped for session with tag |session_tag|.
+  std::vector<const sessions::SessionTab*> LookupUnmappedTabs(
+      const std::string& session_tag) const;
+
   // Attempts to look up the session windows associatd with the session given
   // by |session_tag|. Ownership of SessionWindows stays within the
   // SyncedSessionTracker.
@@ -167,6 +177,10 @@ class SyncedSessionTracker {
   // free tab nodes to be deleted.
   void CleanupLocalTabs(std::set<int>* deleted_node_ids);
 
+  // Returns the tab node ID for |tab_id| if an existing tab node was found, or
+  // kInvalidTabNodeID otherwise.
+  int LookupTabNodeFromLocalTabId(SessionID tab_id) const;
+
   // Fills |tab_node_id| with a tab node for |tab_id|. Returns true if an
   // existing tab node was found, false if there was none and one had to be
   // created.
@@ -174,7 +188,11 @@ class SyncedSessionTracker {
 
   // Returns whether |tab_node_id| refers to a valid tab node that is associated
   // with a tab.
-  bool IsLocalTabNodeAssociated(int tab_node_id);
+  bool IsLocalTabNodeAssociated(int tab_node_id) const;
+
+  // Returns the local tab ID associated to |tab_node_id| or
+  // SessionID::InvalidValue() if not associated.
+  SessionID LookupLocalTabIdFromTabNodeId(int tab_node_id) const;
 
   // Reassociates the tab denoted by |tab_node_id| with a new tab id, preserving
   // any previous SessionTab object the node was associated with. This is useful
@@ -254,6 +272,10 @@ class SyncedSessionTracker {
   // Creates tracked session if it wasn't known previously. Never returns null.
   TrackedSession* GetTrackedSession(const std::string& session_tag);
 
+  std::vector<const SyncedSession*> LookupSessions(
+      SessionLookup lookup,
+      bool exclude_local_session) const;
+
   // Implementation of CleanupForeignSession/CleanupLocalTabs.
   void CleanupSessionImpl(const std::string& session_tag);
 

components/sync_sessions/synced_session_tracker_unittest.cc

@@ -168,6 +168,44 @@ TEST_F(SyncedSessionTrackerTest, PutTabInWindow) {
   // Should clean up memory on its own.
 }
 
+TEST_F(SyncedSessionTrackerTest, LookupAllSessions) {
+  EXPECT_THAT(
+      GetTracker()->LookupAllSessions(SyncedSessionTracker::PRESENTABLE),
+      IsEmpty());
+
+  GetTracker()->InitLocalSession(kTag, kSessionName, kDeviceType);
+  GetTracker()->PutWindowInSession(kTag, kWindow1);
+  GetTracker()->PutTabInWindow(kTag, kWindow1, kTab1);
+
+  EXPECT_THAT(GetTracker()->LookupAllSessions(SyncedSessionTracker::RAW),
+              ElementsAre(HasSessionTag(kTag)));
+  EXPECT_THAT(
+      GetTracker()->LookupAllSessions(SyncedSessionTracker::PRESENTABLE),
+      IsEmpty());
+
+  sessions::SessionTab* tab = GetTracker()->GetTab(kTag, kTab1);
+  ASSERT_TRUE(tab);
+  tab->navigations.push_back(
+      sessions::SerializedNavigationEntryTestHelper::CreateNavigation(kValidUrl,
+                                                                      kTitle));
+  EXPECT_THAT(
+      GetTracker()->LookupAllSessions(SyncedSessionTracker::PRESENTABLE),
+      ElementsAre(HasSessionTag(kTag)));
+
+  GetTracker()->GetSession(kTag2);
+  GetTracker()->PutWindowInSession(kTag2, kWindow1);
+  GetTracker()->PutTabInWindow(kTag2, kWindow1, kTab2);
+
+  sessions::SessionTab* tab2 = GetTracker()->GetTab(kTag2, kTab2);
+  ASSERT_TRUE(tab2);
+  tab2->navigations.push_back(
+      sessions::SerializedNavigationEntryTestHelper::CreateNavigation(kValidUrl,
+                                                                      kTitle));
+  EXPECT_THAT(
+      GetTracker()->LookupAllSessions(SyncedSessionTracker::PRESENTABLE),
+      ElementsAre(HasSessionTag(kTag), HasSessionTag(kTag2)));
+}
+
 TEST_F(SyncedSessionTrackerTest, LookupAllForeignSessions) {
   const char kInvalidUrl[] = "invalid.url";
   ON_CALL(*GetSyncSessionsClient(), ShouldSyncURL(GURL(kInvalidUrl)))
@@ -347,6 +385,20 @@ TEST_F(SyncedSessionTrackerTest, LookupTabNodeIds) {
   EXPECT_THAT(GetTracker()->LookupTabNodeIds(kTag2), IsEmpty());
 }
 
+TEST_F(SyncedSessionTrackerTest, LookupUnmappedTabs) {
+  EXPECT_THAT(GetTracker()->LookupUnmappedTabs(kTag), IsEmpty());
+
+  sessions::SessionTab* tab = GetTracker()->GetTab(kTag, kTab1);
+  ASSERT_THAT(tab, NotNull());
+
+  EXPECT_THAT(GetTracker()->LookupUnmappedTabs(kTag), ElementsAre(tab));
+  EXPECT_THAT(GetTracker()->LookupUnmappedTabs(kTag2), IsEmpty());
+
+  GetTracker()->PutWindowInSession(kTag, kWindow1);
+  GetTracker()->PutTabInWindow(kTag, kWindow1, kTab1);
+  EXPECT_THAT(GetTracker()->LookupUnmappedTabs(kTag), IsEmpty());
+}
+
 TEST_F(SyncedSessionTrackerTest, SessionTracking) {
   ASSERT_TRUE(GetTracker()->Empty());
 

components/sync_sessions/tab_node_pool.cc

@@ -50,25 +50,12 @@ void TabNodePool::AssociateTabNode(int tab_node_id, SessionID tab_id) {
   tabid_nodeid_map_[tab_id] = tab_node_id;
 }
 
-bool TabNodePool::GetTabNodeForTab(SessionID tab_id, int* tab_node_id) {
-  if (tabid_nodeid_map_.find(tab_id) != tabid_nodeid_map_.end()) {
-    *tab_node_id = tabid_nodeid_map_[tab_id];
-    return true;
-  }
-
-  if (free_nodes_pool_.empty()) {
-    // Tab pool has no free nodes, allocate new one.
-    *tab_node_id = ++max_used_tab_node_id_;
-    AddTabNode(*tab_node_id);
-
-    AssociateTabNode(*tab_node_id, tab_id);
-    return false;
-  } else {
-    // Return the next free node.
-    *tab_node_id = *free_nodes_pool_.begin();
-    AssociateTabNode(*tab_node_id, tab_id);
-    return true;
+int TabNodePool::GetTabNodeIdFromTabId(SessionID tab_id) const {
+  TabIDToTabNodeIDMap::const_iterator it = tabid_nodeid_map_.find(tab_id);
+  if (it != tabid_nodeid_map_.end()) {
+    return it->second;
   }
+  return kInvalidTabNodeID;
 }
 
 void TabNodePool::FreeTab(SessionID tab_id) {
@@ -85,6 +72,21 @@ void TabNodePool::FreeTab(SessionID tab_id) {
   free_nodes_pool_.insert(tab_node_id);
 }
 
+int TabNodePool::AssociateWithFreeTabNode(SessionID tab_id) {
+  int tab_node_id;
+  if (free_nodes_pool_.empty()) {
+    // Tab pool has no free nodes, allocate new one.
+    tab_node_id = ++max_used_tab_node_id_;
+    AddTabNode(tab_node_id);
+  } else {
+    // Return the next free node.
+    tab_node_id = *free_nodes_pool_.begin();
+  }
+
+  AssociateTabNode(tab_node_id, tab_id);
+  return tab_node_id;
+}
+
 void TabNodePool::ReassociateTabNode(int tab_node_id, SessionID tab_id) {
   DCHECK_GT(tab_node_id, kInvalidTabNodeID);
   DCHECK(tab_id.is_valid());

components/sync_sessions/tab_node_pool.h

@@ -42,19 +42,18 @@ class TabNodePool {
 
   static const int kInvalidTabNodeID;
 
-  // Fills |tab_node_id| with a tab node associated with |tab_id|.
-  // If tab_id is already associated with a tab_node_id, reuses the existing
-  // association. Otherwise attempts to get the next free tab node and
-  // associate it with |tab_id|. If none are available, will create a new tab
-  // node.
-  // Returns true if a pre-existing tab node could be reused, false if a new one
-  // had to be created.
-  bool GetTabNodeForTab(SessionID tab_id, int* tab_node_id);
+  // Returns the tab node associated with |tab_id| or kInvalidTabNodeID if
+  // no association existed.
+  int GetTabNodeIdFromTabId(SessionID tab_id) const;
 
   // Returns the tab_id for |tab_node_id| if it is associated else returns an
   // invalid ID.
   SessionID GetTabIdFromTabNodeId(int tab_node_id) const;
 
+  // Gets the next free tab node (or creates a new one if needed) and associates
+  // it to |tab_id|. Returns the tab node ID associated to |tab_id|.
+  int AssociateWithFreeTabNode(SessionID tab_id);
+
   // Reassociates |tab_node_id| with |tab_id|. If |tab_node_id| is not already
   // known, it is added to the tab node pool before being associated.
   void ReassociateTabNode(int tab_node_id, SessionID tab_id);

components/sync_sessions/tab_node_pool_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <vector>
 
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace sync_sessions {
@@ -16,19 +17,19 @@ class SyncTabNodePoolTest : public testing::Test {
 
   int GetMaxUsedTabNodeId() const { return pool_.max_used_tab_node_id_; }
 
-  void AddFreeTabNodes(size_t size, const int node_ids[]);
+  void AddFreeTabNodes(const std::vector<int>& node_ids) {
+    for (int node_id : node_ids) {
+      pool_.free_nodes_pool_.insert(node_id);
+    }
+  }
 
   TabNodePool pool_;
 };
 
-void SyncTabNodePoolTest::AddFreeTabNodes(size_t size, const int node_ids[]) {
-  for (size_t i = 0; i < size; ++i) {
-    pool_.free_nodes_pool_.insert(node_ids[i]);
-  }
-}
-
 namespace {
 
+using testing::UnorderedElementsAre;
+
 const int kTabNodeId1 = 10;
 const int kTabNodeId2 = 5;
 const int kTabNodeId3 = 1000;
@@ -57,9 +58,11 @@ TEST_F(SyncTabNodePoolTest, TabNodeIdIncreases) {
   pool_.CleanupTabNodes(&deleted_node_ids);
   EXPECT_TRUE(deleted_node_ids.empty());
   for (int i = 0; i < 3; ++i) {
-    int tab_node_id = -1;
-    EXPECT_TRUE(pool_.GetTabNodeForTab(SessionID::FromSerializedValue(i + 1),
-                                       &tab_node_id));
+    const SessionID tab_id = SessionID::FromSerializedValue(i + 1);
+    ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
+              pool_.GetTabNodeIdFromTabId(tab_id));
+    EXPECT_NE(TabNodePool::kInvalidTabNodeID,
+              pool_.AssociateWithFreeTabNode(tab_id));
     EXPECT_EQ(kTabNodeId3, GetMaxUsedTabNodeId());
   }
   pool_.CleanupTabNodes(&deleted_node_ids);
@@ -135,19 +138,19 @@ TEST_F(SyncTabNodePoolTest, ReassociateThenFree) {
   pool_.CleanupTabNodes(&deleted_node_ids);
   EXPECT_TRUE(deleted_node_ids.empty());
   EXPECT_TRUE(pool_.Full());
-  std::set<int> free_sync_ids;
+
+  // Reassociate tab nodes.
+  std::vector<int> sync_ids;
   for (int i = 1; i <= 3; ++i) {
-    int tab_node_id = -1;
-    EXPECT_TRUE(pool_.GetTabNodeForTab(SessionID::FromSerializedValue(i),
-                                       &tab_node_id));
-    free_sync_ids.insert(tab_node_id);
+    const SessionID tab_id = SessionID::FromSerializedValue(i);
+    EXPECT_EQ(TabNodePool::kInvalidTabNodeID,
+              pool_.GetTabNodeIdFromTabId(tab_id));
+    sync_ids.push_back(pool_.AssociateWithFreeTabNode(tab_id));
   }
 
   EXPECT_TRUE(pool_.Empty());
-  EXPECT_EQ(3u, free_sync_ids.size());
-  EXPECT_EQ(1u, free_sync_ids.count(kTabNodeId1));
-  EXPECT_EQ(1u, free_sync_ids.count(kTabNodeId2));
-  EXPECT_EQ(1u, free_sync_ids.count(kTabNodeId3));
+  EXPECT_THAT(sync_ids,
+              UnorderedElementsAre(kTabNodeId1, kTabNodeId2, kTabNodeId3));
 }
 
 TEST_F(SyncTabNodePoolTest, Init) {
@@ -156,25 +159,32 @@ TEST_F(SyncTabNodePoolTest, Init) {
 }
 
 TEST_F(SyncTabNodePoolTest, AddGet) {
-  int free_nodes[] = {5, 10};
-  AddFreeTabNodes(2, free_nodes);
+  AddFreeTabNodes({5, 10});
 
   EXPECT_EQ(2U, pool_.Capacity());
-  int tab_node_id = -1;
-  EXPECT_TRUE(pool_.GetTabNodeForTab(kTabId1, &tab_node_id));
-  EXPECT_EQ(5, tab_node_id);
+  ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
+            pool_.GetTabNodeIdFromTabId(kTabId1));
+  EXPECT_EQ(5, pool_.AssociateWithFreeTabNode(kTabId1));
   EXPECT_FALSE(pool_.Empty());
   EXPECT_FALSE(pool_.Full());
   EXPECT_EQ(2U, pool_.Capacity());
+  ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
+            pool_.GetTabNodeIdFromTabId(kTabId2));
   // 5 is now used, should return 10.
-  EXPECT_TRUE(pool_.GetTabNodeForTab(kTabId2, &tab_node_id));
-  EXPECT_EQ(10, tab_node_id);
+  EXPECT_EQ(10, pool_.AssociateWithFreeTabNode(kTabId2));
 }
 
-TEST_F(SyncTabNodePoolTest, GetTabNodeForTabCreate) {
-  int tab_node_id = -1;
-  EXPECT_FALSE(pool_.GetTabNodeForTab(kTabId1, &tab_node_id));
-  EXPECT_EQ(0, tab_node_id);
+TEST_F(SyncTabNodePoolTest, AssociateWithFreeTabNode) {
+  ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
+            pool_.GetTabNodeIdFromTabId(kTabId1));
+  ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
+            pool_.GetTabNodeIdFromTabId(kTabId2));
+  EXPECT_EQ(0, pool_.AssociateWithFreeTabNode(kTabId1));
+  EXPECT_EQ(0, pool_.GetTabNodeIdFromTabId(kTabId1));
+  ASSERT_EQ(TabNodePool::kInvalidTabNodeID,
+            pool_.GetTabNodeIdFromTabId(kTabId2));
+  EXPECT_EQ(1, pool_.AssociateWithFreeTabNode(kTabId2));
+  EXPECT_EQ(1, pool_.GetTabNodeIdFromTabId(kTabId2));
 }
 
 TEST_F(SyncTabNodePoolTest, TabPoolFreeNodeLimits) {
@@ -185,10 +195,8 @@ TEST_F(SyncTabNodePoolTest, TabPoolFreeNodeLimits) {
   // kFreeNodesLowWatermark.
   std::vector<int> used_sync_ids;
   for (size_t i = 1; i <= TabNodePool::kFreeNodesHighWatermark + 1; ++i) {
-    int sync_id = -1;
-    EXPECT_FALSE(
-        pool_.GetTabNodeForTab(SessionID::FromSerializedValue(i), &sync_id));
-    used_sync_ids.push_back(sync_id);
+    used_sync_ids.push_back(
+        pool_.AssociateWithFreeTabNode(SessionID::FromSerializedValue(i)));
   }
 
   // Free all except one node.