Reland v3 of Session refactor

chrome/browser/sync/sessions/sessions_sync_manager_unittest.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 <stdint.h>
 
 #include <utility>
 
@@ skipping 30 matching lines @@
 using syncer::DeviceInfo;
 using syncer::LocalDeviceInfoProvider;
 using syncer::LocalDeviceInfoProviderMock;
 using syncer::SyncChange;
 using syncer::SyncData;
 
 namespace sync_sessions {
 
 namespace {
 
+std::string TabNodeIdToTag(const std::string& machine_tag, int tab_node_id) {
+  return base::StringPrintf("%s %d", machine_tag.c_str(), tab_node_id);
+}
+
 class SessionNotificationObserver {
  public:
   SessionNotificationObserver()
       : notified_of_update_(false), notified_of_refresh_(false) {}
   void NotifyOfUpdate() { notified_of_update_ = true; }
   void NotifyOfRefresh() { notified_of_refresh_ = true; }
 
   bool notified_of_update() const { return notified_of_update_; }
   bool notified_of_refresh() const { return notified_of_refresh_; }
 
@@ skipping 60 matching lines @@
   bool IsSessionRestoreInProgress() const override {
     return wrapped_->IsSessionRestoreInProgress();
   }
 
   bool ShouldSync() const override { return wrapped_->ShouldSync(); }
 
  private:
   std::map<int, SyncedTabDelegate*> tab_overrides_;
   std::map<int, SessionID::id_type> tab_id_overrides_;
   const SyncedWindowDelegate* const wrapped_;
-  SessionID::id_type session_id_override_ = -1;
+  SessionID::id_type session_id_override_ = TabNodePool::kInvalidTabID;
 };
 
 class TestSyncedWindowDelegatesGetter : public SyncedWindowDelegatesGetter {
  public:
   TestSyncedWindowDelegatesGetter(
       const std::set<const SyncedWindowDelegate*>& delegates)
       : delegates_(delegates) {}
 
   std::set<const SyncedWindowDelegate*> GetSyncedWindowDelegates() override {
     return delegates_;
@@ skipping 250 matching lines @@
       }
     }
     EXPECT_TRUE(found);
     return list;
   }
 
   SyncSessionsClient* GetSyncSessionsClient() {
     return sessions_client_shim_.get();
   }
 
+  TabNodePool* GetTabPool() {
+    return &manager()->session_tracker_.local_tab_pool_;
+  }
+
   syncer::SyncPrefs* sync_prefs() { return sync_prefs_.get(); }
 
   SyncedWindowDelegatesGetter* get_synced_window_getter() {
     return manager()->synced_window_delegates_getter();
   }
 
   void set_synced_window_getter(
       SyncedWindowDelegatesGetter* synced_window_getter) {
     sessions_client_shim_->set_synced_window_getter(synced_window_getter);
   }
@@ skipping 83 matching lines @@
   ASSERT_EQ(1U, session->windows[0]->tabs.size());
   ASSERT_EQ(1, session->windows[0]->selected_tab_index);
   ASSERT_EQ(sessions::SessionWindow::TYPE_TABBED, session->windows[0]->type);
   ASSERT_EQ(1U, manager()->session_tracker_.num_synced_sessions());
   ASSERT_EQ(1U,
             manager()->session_tracker_.num_synced_tabs(std::string("tag")));
 }
 
 namespace {
 
+// A SyncedTabDelegate fake for testing. It simulates a normal
+// SyncedTabDelegate with a proper WebContents. For a SyncedTabDelegate without
+// a WebContents, see PlaceholderTabDelegate below.
 class SyncedTabDelegateFake : public SyncedTabDelegate {
  public:
-  SyncedTabDelegateFake()
-      : current_entry_index_(0), is_supervised_(false), sync_id_(-1) {}
+  SyncedTabDelegateFake() {}
   ~SyncedTabDelegateFake() override {}
 
+  // SyncedTabDelegate overrides.
   bool IsInitialBlankNavigation() const override {
     // This differs from NavigationControllerImpl, which has an initial blank
     // NavigationEntry.
     return GetEntryCount() == 0;
   }
   int GetCurrentEntryIndex() const override { return current_entry_index_; }
-  void set_current_entry_index(int i) {
-    current_entry_index_ = i;
-  }
-
-  void AppendEntry(std::unique_ptr<content::NavigationEntry> entry) {
-    entries_.push_back(std::move(entry));
-  }
-
   GURL GetVirtualURLAtIndex(int i) const override {
     if (static_cast<size_t>(i) >= entries_.size())
       return GURL();
     return entries_[i]->GetVirtualURL();
   }
-
   GURL GetFaviconURLAtIndex(int i) const override { return GURL(); }
-
   ui::PageTransition GetTransitionAtIndex(int i) const override {
     if (static_cast<size_t>(i) >= entries_.size())
       return ui::PAGE_TRANSITION_LINK;
     return entries_[i]->GetTransitionType();
   }
-
   void GetSerializedNavigationAtIndex(
       int i,
       sessions::SerializedNavigationEntry* serialized_entry) const override {
     if (static_cast<size_t>(i) >= entries_.size())
       return;
     *serialized_entry =
         sessions::ContentSerializedNavigationBuilder::FromNavigationEntry(
             i, *entries_[i]);
   }
-
   int GetEntryCount() const override { return entries_.size(); }
-
-  SessionID::id_type GetWindowId() const override {
-    return SessionID::id_type();
-  }
-
-  SessionID::id_type GetSessionId() const override {
-    return SessionID::id_type();
-  }
-
+  SessionID::id_type GetWindowId() const override { return window_id_; }
+  SessionID::id_type GetSessionId() const override { return tab_id_; }
   bool IsBeingDestroyed() const override { return false; }
   std::string GetExtensionAppId() const override { return std::string(); }
   bool ProfileIsSupervised() const override { return is_supervised_; }
   void set_is_supervised(bool is_supervised) { is_supervised_ = is_supervised; }
   const std::vector<std::unique_ptr<const sessions::SerializedNavigationEntry>>*
   GetBlockedNavigations() const override {
     return &blocked_navigations_;
   }
+  bool IsPlaceholderTab() const override { return false; }
+  int GetSyncId() const override { return sync_id_; }
+  void SetSyncId(int sync_id) override { sync_id_ = sync_id; }
+  bool ShouldSync(SyncSessionsClient* sessions_client) override {
+    return false;
+  }
+
+  void AppendEntry(std::unique_ptr<content::NavigationEntry> entry) {
+    entries_.push_back(std::move(entry));
+  }
+
+  void set_current_entry_index(int i) { current_entry_index_ = i; }
+
+  void SetWindowId(SessionID::id_type window_id) { window_id_ = window_id; }
+
+  void SetSessionId(SessionID::id_type id) { tab_id_ = id; }
+
   void set_blocked_navigations(
       std::vector<const content::NavigationEntry*>* navs) {
     for (auto* entry : *navs) {
       auto serialized_entry =
           base::MakeUnique<sessions::SerializedNavigationEntry>(
               sessions::ContentSerializedNavigationBuilder::FromNavigationEntry(
                   blocked_navigations_.size(), *entry));
       blocked_navigations_.push_back(std::move(serialized_entry));
     }
   }
-  bool IsPlaceholderTab() const override { return true; }
-
-  // Session sync related methods.
-  int GetSyncId() const override { return sync_id_; }
-  void SetSyncId(int sync_id) override { sync_id_ = sync_id; }
-
-  bool ShouldSync(SyncSessionsClient* sessions_client) override {
-    return false;
-  }
 
   void reset() {
     current_entry_index_ = 0;
-    sync_id_ = -1;
+    sync_id_ = TabNodePool::kInvalidTabNodeID;
     entries_.clear();
   }
 
  private:
-  int current_entry_index_;
-  bool is_supervised_;
-  int sync_id_;
+  int current_entry_index_ = 0;
+  bool is_supervised_ = false;
+  int sync_id_ = -1;
+  SessionID::id_type tab_id_ = 0;
+  SessionID::id_type window_id_ = 0;
   std::vector<std::unique_ptr<const sessions::SerializedNavigationEntry>>
       blocked_navigations_;
   std::vector<std::unique_ptr<content::NavigationEntry>> entries_;
 };
 
+// A placeholder delegate. These delegates have no WebContents, simulating a tab
+// that has been restored without bringing its state fully into memory (for
+// example on Android), or where the tab's contents have been evicted from
+// memory. See SyncedTabDelegate::IsPlaceHolderTab for more info.
+class PlaceholderTabDelegate : public SyncedTabDelegate {
+ public:
+  PlaceholderTabDelegate(SessionID::id_type session_id, int sync_id)
+      : session_id_(session_id), sync_id_(sync_id) {}
+  ~PlaceholderTabDelegate() override {}
+
+  // SyncedTabDelegate overrides.
+  SessionID::id_type GetSessionId() const override { return session_id_; }
+  int GetSyncId() const override { return sync_id_; }
+  void SetSyncId(int sync_id) override { sync_id_ = sync_id; }
+  bool IsPlaceholderTab() const override { return true; }
+
+  // Everything else is invalid to invoke as it depends on a valid WebContents.
+  SessionID::id_type GetWindowId() const override {
+    NOTREACHED();
+    return 0;
+  }
+  bool IsBeingDestroyed() const override {
+    NOTREACHED();
+    return false;
+  }
+  std::string GetExtensionAppId() const override {
+    NOTREACHED();
+    return "";
+  }
+  bool IsInitialBlankNavigation() const override {
+    NOTREACHED();
+    return false;
+  }
+  int GetCurrentEntryIndex() const override {
+    NOTREACHED();
+    return 0;
+  }
+  int GetEntryCount() const override {
+    NOTREACHED();
+    return 0;
+  }
+  GURL GetVirtualURLAtIndex(int i) const override {
+    NOTREACHED();
+    return GURL();
+  }
+  GURL GetFaviconURLAtIndex(int i) const override {
+    NOTREACHED();
+    return GURL();
+  }
+  ui::PageTransition GetTransitionAtIndex(int i) const override {
+    NOTREACHED();
+    return ui::PageTransition();
+  }
+  void GetSerializedNavigationAtIndex(
+      int i,
+      sessions::SerializedNavigationEntry* serialized_entry) const override {
+    NOTREACHED();
+  }
+  bool ProfileIsSupervised() const override {
+    NOTREACHED();
+    return false;
+  }
+  const std::vector<std::unique_ptr<const sessions::SerializedNavigationEntry>>*
+  GetBlockedNavigations() const override {
+    NOTREACHED();
+    return nullptr;
+  }
+  bool ShouldSync(SyncSessionsClient* sessions_client) override {
+    NOTREACHED();
+    return false;
+  }
+
+ private:
+  SessionID::id_type session_id_;
+  int sync_id_;
+};
+
 }  // namespace
 
 static const base::Time kTime0 = base::Time::FromInternalValue(100);
 static const base::Time kTime1 = base::Time::FromInternalValue(110);
 static const base::Time kTime2 = base::Time::FromInternalValue(120);
 static const base::Time kTime3 = base::Time::FromInternalValue(130);
 static const base::Time kTime4 = base::Time::FromInternalValue(140);
 static const base::Time kTime5 = base::Time::FromInternalValue(150);
 static const base::Time kTime6 = base::Time::FromInternalValue(160);
 static const base::Time kTime7 = base::Time::FromInternalValue(170);
@@ skipping 334 matching lines @@
           new syncer::SyncErrorFactoryMock()));
   ASSERT_FALSE(result.error().IsSet());
 
   EXPECT_EQ(1U, out.size());
   EXPECT_EQ(SyncChange::ACTION_UPDATE, out[0].change_type());
   EXPECT_TRUE(out[0].sync_data().GetSpecifics().session().has_header());
 }
 
 // Ensure model association associates the pre-existing tabs.
 TEST_F(SessionsSyncManagerTest, SwappedOutOnRestore) {
-  AddTab(browser(), GURL("http://foo1"));
-  NavigateAndCommitActiveTab(GURL("http://foo2"));
-  AddTab(browser(), GURL("http://bar1"));
-  NavigateAndCommitActiveTab(GURL("http://bar2"));
-  AddTab(browser(), GURL("http://baz1"));
-  NavigateAndCommitActiveTab(GURL("http://baz2"));
+  const char kFoo1[] = "http://foo1/";
+  const char kFoo2[] = "http://foo2/";
+  const char kBar1[] = "http://bar1/";
+  const char kBar2[] = "http://bar2/";
+  const char kBaz1[] = "http://baz1/";
+  const char kBaz2[] = "http://baz2/";
   const int kRestoredTabId = 1337;
   const int kNewTabId = 2468;
 
+  // AddTab inserts at index 0, so go in reverse order (tab 3 -> tab 1).
+  AddTab(browser(), GURL(kBaz1));
+  NavigateAndCommitActiveTab(GURL(kBaz2));
+  AddTab(browser(), GURL(kBar1));
+  NavigateAndCommitActiveTab(GURL(kBar2));
+  AddTab(browser(), GURL(kFoo1));
+  NavigateAndCommitActiveTab(GURL(kFoo2));
+
   syncer::SyncDataList in;
   syncer::SyncChangeList out;
   InitWithSyncDataTakeOutput(in, &out);
 
-  // Should be one header add, 3 tab add/update pairs, one header update.
-  ASSERT_EQ(8U, out.size());
+  // Should be one header add, 3 tab adds, one header update.
+  ASSERT_EQ(5U, out.size());
 
   // For input, we set up:
   // * one "normal" fully loaded tab
-  // * one "frozen" tab with no WebContents and a tab_id change
-  // * one "frozen" tab with no WebContents and no tab_id change
-  sync_pb::EntitySpecifics t0_entity = out[2].sync_data().GetSpecifics();
-  sync_pb::EntitySpecifics t1_entity = out[4].sync_data().GetSpecifics();
-  sync_pb::EntitySpecifics t2_entity = out[6].sync_data().GetSpecifics();
+  // * one placeholder tab with no WebContents and a tab_id change
+  // * one placeholder tab with no WebContents and no tab_id change
+  sync_pb::EntitySpecifics t0_entity = out[1].sync_data().GetSpecifics();
+  sync_pb::EntitySpecifics t1_entity = out[2].sync_data().GetSpecifics();
+  sync_pb::EntitySpecifics t2_entity = out[3].sync_data().GetSpecifics();
   t1_entity.mutable_session()->mutable_tab()->set_tab_id(kRestoredTabId);
   in.push_back(CreateRemoteData(t0_entity));
   in.push_back(CreateRemoteData(t1_entity));
   in.push_back(CreateRemoteData(t2_entity));
   out.clear();
   manager()->StopSyncing(syncer::SESSIONS);
 
   const std::set<const SyncedWindowDelegate*>& windows =
       manager()->synced_window_delegates_getter()->GetSyncedWindowDelegates();
   ASSERT_EQ(1U, windows.size());
-  SyncedTabDelegateFake t1_override, t2_override;
-  t1_override.SetSyncId(1);  // No WebContents by default.
-  t2_override.SetSyncId(2);  // No WebContents by default.
+  PlaceholderTabDelegate t1_override(kNewTabId, 1);
+  PlaceholderTabDelegate t2_override(t2_entity.session().tab().tab_id(), 2);
   SyncedWindowDelegateOverride window_override(*windows.begin());
   window_override.OverrideTabAt(1, &t1_override, kNewTabId);
   window_override.OverrideTabAt(2, &t2_override,
                                 t2_entity.session().tab().tab_id());
   std::set<const SyncedWindowDelegate*> delegates;
   delegates.insert(&window_override);
   std::unique_ptr<TestSyncedWindowDelegatesGetter> getter(
       new TestSyncedWindowDelegatesGetter(delegates));
   set_synced_window_getter(getter.get());
 
   syncer::SyncMergeResult result = manager()->MergeDataAndStartSyncing(
       syncer::SESSIONS, in, std::unique_ptr<syncer::SyncChangeProcessor>(
                                 new TestSyncProcessorStub(&out)),
       std::unique_ptr<syncer::SyncErrorFactory>(
           new syncer::SyncErrorFactoryMock()));
 
   // There should be two changes, one for the fully associated tab, and

[Patrick Noland, 2017/01/25 19:23:41]

Update to say "three changes"

[Nicolas Zea, 2017/01/25 23:55:04]

Done.

-  // one for the tab_id update to t1.  t2 shouldn't need to be updated.
-  ASSERT_EQ(2U, FilterOutLocalHeaderChanges(&out)->size());
+  // one each for the tab_id updates to t1 and t2.
+  ASSERT_EQ(3U, FilterOutLocalHeaderChanges(&out)->size());
   EXPECT_EQ(SyncChange::ACTION_UPDATE, out[0].change_type());
+  EXPECT_EQ(kFoo2, out[0]
+                       .sync_data()
+                       .GetSpecifics()
+                       .session()
+                       .tab()
+                       .navigation(1)
+                       .virtual_url());
+
   EXPECT_EQ(SyncChange::ACTION_UPDATE, out[1].change_type());
   EXPECT_EQ(kNewTabId,
             out[1].sync_data().GetSpecifics().session().tab().tab_id());
+  EXPECT_EQ(kBar2, out[1]
+                       .sync_data()
+                       .GetSpecifics()
+                       .session()
+                       .tab()
+                       .navigation(1)
+                       .virtual_url());
 
-  // Verify TabLinks.
-  SessionsSyncManager::TabLinksMap tab_map = manager()->local_tab_map_;
-  ASSERT_EQ(3U, tab_map.size());
-  int t2_tab_id = t2_entity.session().tab().tab_id();
-  EXPECT_EQ(2, tab_map.find(t2_tab_id)->second->tab_node_id());
-  EXPECT_EQ(1, tab_map.find(kNewTabId)->second->tab_node_id());
-  int t0_tab_id = out[0].sync_data().GetSpecifics().session().tab().tab_id();
-  EXPECT_EQ(0, tab_map.find(t0_tab_id)->second->tab_node_id());
-  // TODO(tim): Once bug 337057 is fixed, we can issue an OnLocalTabModified
-  // from here (using an override similar to above) to return a new tab id
-  // and verify that we don't see any node creations in the SyncChangeProcessor
-  // (similar to how SessionsSyncManagerTest.OnLocalTabModified works.)
+  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[2].change_type());
+  EXPECT_EQ(t2_entity.session().tab().tab_id(),
+            out[2].sync_data().GetSpecifics().session().tab().tab_id());
+  EXPECT_EQ(kBaz2, out[2]
+                       .sync_data()
+                       .GetSpecifics()
+                       .session()
+                       .tab()
+                       .navigation(1)
+                       .virtual_url());
 }
 
 // Ensure model association updates the window ID for tabs whose window's ID has
 // changed.
 TEST_F(SessionsSyncManagerTest, WindowIdUpdatedOnRestore) {
+  const char kFoo1[] = "http://foo1/";

[Patrick Noland, 2017/01/25 19:23:41]

You've got the same two constants in SwappedOutOnRestore, any reason not to make
them top level?

[Nicolas Zea, 2017/01/25 23:55:04]

Done.

+  const char kFoo2[] = "http://foo2/";
   const int kNewWindowId = 1337;
   syncer::SyncDataList in;
   syncer::SyncChangeList out;
 
   // Set up one tab and start sync with it.
-  AddTab(browser(), GURL("http://foo1"));
-  NavigateAndCommitActiveTab(GURL("http://foo2"));
+  AddTab(browser(), GURL(kFoo1));
+  NavigateAndCommitActiveTab(GURL(kFoo2));
   InitWithSyncDataTakeOutput(in, &out);
 
-  // Should be one header add, 1 tab add/update pair, and one header update.
-  ASSERT_EQ(4U, out.size());
-  const sync_pb::EntitySpecifics t0_entity = out[2].sync_data().GetSpecifics();
+  // Should be one header add, 1 tab add, and one header update.
+  ASSERT_EQ(3U, out.size());
+  const sync_pb::EntitySpecifics t0_entity = out[1].sync_data().GetSpecifics();
+  ASSERT_TRUE(t0_entity.session().has_tab());
 
   in.push_back(CreateRemoteData(t0_entity));
   out.clear();
   manager()->StopSyncing(syncer::SESSIONS);
 
-  // SyncedTabDelegateFake is a placeholder (no WebContents) by default.
-  SyncedTabDelegateFake t0_override;
-  t0_override.SetSyncId(t0_entity.session().tab_node_id());
+  // Override the tab with a placeholder tab delegate.
+  PlaceholderTabDelegate t0_override(t0_entity.session().tab().tab_id(),
+                                     t0_entity.session().tab_node_id());
 
   // Set up the window override with the new window ID and placeholder tab.
   const std::set<const SyncedWindowDelegate*>& windows =
       get_synced_window_getter()->GetSyncedWindowDelegates();
   ASSERT_EQ(1U, windows.size());
   SyncedWindowDelegateOverride window_override(*windows.begin());
   window_override.OverrideSessionId(kNewWindowId);
   window_override.OverrideTabAt(0, &t0_override,
                                 t0_entity.session().tab().tab_id());
 
   // Inject the window override.
   std::set<const SyncedWindowDelegate*> delegates;
   delegates.insert(&window_override);
   std::unique_ptr<TestSyncedWindowDelegatesGetter> getter(
       new TestSyncedWindowDelegatesGetter(delegates));
   set_synced_window_getter(getter.get());
 
   syncer::SyncMergeResult result = manager()->MergeDataAndStartSyncing(
       syncer::SESSIONS, in, std::unique_ptr<syncer::SyncChangeProcessor>(
                                 new TestSyncProcessorStub(&out)),
       std::unique_ptr<syncer::SyncErrorFactory>(
           new syncer::SyncErrorFactoryMock()));
 
   // There should be one change for t0's window ID update.
   ASSERT_EQ(1U, FilterOutLocalHeaderChanges(&out)->size());
   EXPECT_EQ(SyncChange::ACTION_UPDATE, out[0].change_type());
   EXPECT_EQ(kNewWindowId,
             out[0].sync_data().GetSpecifics().session().tab().window_id());
+  EXPECT_EQ(kFoo2, out[0]
+                       .sync_data()
+                       .GetSpecifics()
+                       .session()
+                       .tab()
+                       .navigation(1)
+                       .virtual_url());
+}
+
+// Ensure that the manager properly ignores a restored placeholder that refers
+// to a tab node that doesn't exist
+TEST_F(SessionsSyncManagerTest, RestoredPlacholderTabNodeDeleted) {
+  const char kFoo1[] = "http://foo1/";
+  const char kFoo2[] = "http://foo2/";
+  const int kNewWindowId = 1337;
+  syncer::SyncDataList in;
+  syncer::SyncChangeList out;
+
+  // Set up one tab and start sync with it.
+  AddTab(browser(), GURL(kFoo1));
+  NavigateAndCommitActiveTab(GURL(kFoo2));
+  InitWithSyncDataTakeOutput(in, &out);
+
+  // Should be one header add, 1 tab add, and one header update.
+  ASSERT_EQ(3U, out.size());
+  const sync_pb::EntitySpecifics t0_entity = out[1].sync_data().GetSpecifics();
+  ASSERT_TRUE(t0_entity.session().has_tab());
+
+  out.clear();
+  manager()->StopSyncing(syncer::SESSIONS);
+
+  // Override the tab with a placeholder tab delegate.
+  PlaceholderTabDelegate t0_override(t0_entity.session().tab().tab_id(),
+                                     t0_entity.session().tab_node_id());
+
+  // Set up the window override with the new window ID and placeholder tab.
+  const std::set<const SyncedWindowDelegate*>& windows =
+      get_synced_window_getter()->GetSyncedWindowDelegates();
+  ASSERT_EQ(1U, windows.size());
+  SyncedWindowDelegateOverride window_override(*windows.begin());
+  window_override.OverrideSessionId(kNewWindowId);
+  window_override.OverrideTabAt(0, &t0_override,
+                                t0_entity.session().tab().tab_id());
+
+  // Inject the window override.
+  std::set<const SyncedWindowDelegate*> delegates;
+  delegates.insert(&window_override);
+  std::unique_ptr<TestSyncedWindowDelegatesGetter> getter(
+      new TestSyncedWindowDelegatesGetter(delegates));
+  set_synced_window_getter(getter.get());
+
+  syncer::SyncMergeResult result = manager()->MergeDataAndStartSyncing(
+      syncer::SESSIONS, in, std::unique_ptr<syncer::SyncChangeProcessor>(
+                                new TestSyncProcessorStub(&out)),
+      std::unique_ptr<syncer::SyncErrorFactory>(
+          new syncer::SyncErrorFactoryMock()));
+
+  // Because no entities were passed in at associate time, there should be no
+  // tab changes.
+  ASSERT_EQ(0U, FilterOutLocalHeaderChanges(&out)->size());
 }
 
 // Tests MergeDataAndStartSyncing with sync data but no local data.
 TEST_F(SessionsSyncManagerTest, MergeWithInitialForeignSession) {
   std::string tag = "tag1";
 
   SessionID::id_type n1[] = {5, 10, 13, 17};
   std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
   std::vector<sync_pb::SessionSpecifics> tabs1;
   sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
@@ skipping 42 matching lines @@
   std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
   std::vector<sync_pb::SessionSpecifics> tabs1;
   sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
       tag, tab_list1, &tabs1));
   syncer::SyncDataList foreign_data;
   foreign_data.push_back(CreateRemoteData(meta));
   AddTabsToSyncDataList(tabs1, &foreign_data);
 
   syncer::SyncChangeList output;
   InitWithSyncDataTakeOutput(foreign_data, &output);
-  ASSERT_EQ(4U, output.size());
+  // Should be one header add, 1 tab add, and one header update.
+  ASSERT_EQ(3U, output.size());
 
   // Verify the local header.
   EXPECT_TRUE(output[0].IsValid());
   EXPECT_EQ(SyncChange::ACTION_ADD, output[0].change_type());
   const SyncData data(output[0].sync_data());
   EXPECT_EQ(manager()->current_machine_tag(),
             syncer::SyncDataLocal(data).GetTag());
   const sync_pb::SessionSpecifics& specifics(data.GetSpecifics().session());
   EXPECT_EQ(manager()->current_machine_tag(), specifics.session_tag());
   EXPECT_TRUE(specifics.has_header());
   const sync_pb::SessionHeader& header_s = specifics.header();
   EXPECT_TRUE(header_s.has_device_type());
   EXPECT_EQ(GetLocalDeviceInfo()->client_name(), header_s.client_name());
   EXPECT_EQ(0, header_s.window_size());
 
   // Verify the tab node creations and updates with content.
   for (int i = 1; i < 3; i++) {
     EXPECT_TRUE(output[i].IsValid());
     const SyncData data(output[i].sync_data());
     EXPECT_TRUE(base::StartsWith(syncer::SyncDataLocal(data).GetTag(),
                                  manager()->current_machine_tag(),
                                  base::CompareCase::SENSITIVE));
     const sync_pb::SessionSpecifics& specifics(data.GetSpecifics().session());
     EXPECT_EQ(manager()->current_machine_tag(), specifics.session_tag());
   }
   EXPECT_EQ(SyncChange::ACTION_ADD, output[1].change_type());
   EXPECT_EQ(SyncChange::ACTION_UPDATE, output[2].change_type());
-  EXPECT_TRUE(output[2].sync_data().GetSpecifics().session().has_tab());
+  EXPECT_TRUE(output[1].sync_data().GetSpecifics().session().has_tab());
 
   // Verify the header was updated to reflect window state.
-  EXPECT_TRUE(output[3].IsValid());
-  EXPECT_EQ(SyncChange::ACTION_UPDATE, output[3].change_type());
-  const SyncData data_2(output[3].sync_data());
+  EXPECT_TRUE(output[2].IsValid());
+  EXPECT_EQ(SyncChange::ACTION_UPDATE, output[2].change_type());
+  const SyncData data_2(output[2].sync_data());
   EXPECT_EQ(manager()->current_machine_tag(),
             syncer::SyncDataLocal(data_2).GetTag());
   const sync_pb::SessionSpecifics& specifics2(data_2.GetSpecifics().session());
   EXPECT_EQ(manager()->current_machine_tag(), specifics2.session_tag());
   EXPECT_TRUE(specifics2.has_header());
   const sync_pb::SessionHeader& header_s2 = specifics2.header();
   EXPECT_EQ(1, header_s2.window_size());
 
   // Verify foreign data.
   std::vector<const SyncedSession*> foreign_sessions;
@@ skipping 22 matching lines @@
   SessionID::id_type n1[] = {5, 10, 13, 17};
   std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
   meta1_reference.push_back(tab_list1);
   std::vector<sync_pb::SessionSpecifics> tabs1;
   meta1 = helper()->BuildForeignSession(tag1, tab_list1, &tabs1);
   foreign_data1.push_back(CreateRemoteData(meta1));
   AddTabsToSyncDataList(tabs1, &foreign_data1);
 
   syncer::SyncChangeList output1;
   InitWithSyncDataTakeOutput(foreign_data1, &output1);
-  ASSERT_EQ(4U, output1.size());
+
+  // 1 header add, one tab add, one header update.
+  ASSERT_EQ(3U, output1.size());
 
   // Add a second window to the foreign session.
   // TODO(tim): Bug 98892. Add local window too when observers are hooked up.
   SessionID::id_type tab_nums2[] = {7, 15, 18, 20};
   std::vector<SessionID::id_type> tab_list2(
       tab_nums2, tab_nums2 + arraysize(tab_nums2));
   meta1_reference.push_back(tab_list2);
   helper()->AddWindowSpecifics(1, tab_list2, &meta1);
   std::vector<sync_pb::SessionSpecifics> tabs2;
   tabs2.resize(tab_list2.size());
@@ skipping 70 matching lines @@
   EXPECT_TRUE(changes.empty());
 
   // Fill an instance of session specifics with a foreign session's data.
   std::vector<sync_pb::SessionSpecifics> tabs;
   SessionID::id_type n1[] = {5, 10, 13, 17};
   std::vector<SessionID::id_type> tab_nums1(n1, n1 + arraysize(n1));
   sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
       tag, tab_nums1, &tabs));
 
   // Update associator with the session's meta node, window, and tabs.
-  manager()->UpdateTrackerWithForeignSession(meta, base::Time());
+  manager()->UpdateTrackerWithSpecifics(meta, base::Time());
   for (std::vector<sync_pb::SessionSpecifics>::iterator iter = tabs.begin();
        iter != tabs.end(); ++iter) {
-    manager()->UpdateTrackerWithForeignSession(*iter, base::Time());
+    manager()->UpdateTrackerWithSpecifics(*iter, base::Time());
   }
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
   ASSERT_EQ(1U, foreign_sessions.size());
 
   // Now delete the foreign session.
   manager()->DeleteForeignSessionInternal(tag, &changes);
   EXPECT_FALSE(manager()->GetAllForeignSessions(&foreign_sessions));
 
   EXPECT_EQ(5U, changes.size());
   std::set<std::string> expected_tags(&tag, &tag + 1);
   for (int i = 0; i < 5; i++)
-    expected_tags.insert(TabNodePool::TabIdToTag(tag, i));
+    expected_tags.insert(TabNodeIdToTag(tag, i));
 
   for (int i = 0; i < 5; i++) {
     SCOPED_TRACE(changes[i].ToString());
     EXPECT_TRUE(changes[i].IsValid());
     EXPECT_EQ(SyncChange::ACTION_DELETE, changes[i].change_type());
     EXPECT_TRUE(changes[i].sync_data().IsValid());
     EXPECT_EQ(1U,
               expected_tags.erase(
                   syncer::SyncDataLocal(changes[i].sync_data()).GetTag()));
   }
@@ skipping 102 matching lines @@
 
   // This should trigger repair of the TabNodePool.
   const GURL foo1("http://foo/1");
   AddTab(browser(), foo1);
   EXPECT_FALSE(manager()->local_tab_pool_out_of_sync_);
 
   // AddTab triggers two notifications, one for the tab insertion and one for
   // committing the NavigationEntry. The first notification results in a tab
   // we don't associate although we do update the header node.  The second
   // notification triggers association of the tab, and the subsequent window
-  // update.  So we should see 4 changes at the SyncChangeProcessor.
-  ASSERT_EQ(4U, out.size());
+  // update.  So we should see 3 changes at the SyncChangeProcessor.
+  ASSERT_EQ(3U, out.size());

[Patrick Noland, 2017/01/25 19:23:41]

Is there a way to make the verification of the "shape" of the output more
generic/declarative? There are a lot of similar lines checking that an item in
index i is and ADD and that it contains a header or a tab. 

[Nicolas Zea, 2017/01/25 23:55:04]

I think that's a fair point, although something I'd like to postpone to a more
thorough cleanup patch. 

 
   EXPECT_EQ(SyncChange::ACTION_UPDATE, out[0].change_type());
   ASSERT_TRUE(out[0].sync_data().GetSpecifics().session().has_header());
   EXPECT_EQ(SyncChange::ACTION_ADD, out[1].change_type());
   int tab_node_id = out[1].sync_data().GetSpecifics().session().tab_node_id();
-  EXPECT_EQ(TabNodePool::TabIdToTag(
-                manager()->current_machine_tag(), tab_node_id),
+  EXPECT_EQ(TabNodeIdToTag(manager()->current_machine_tag(), tab_node_id),
             syncer::SyncDataLocal(out[1].sync_data()).GetTag());
   EXPECT_EQ(SyncChange::ACTION_UPDATE, out[2].change_type());
-  ASSERT_TRUE(out[2].sync_data().GetSpecifics().session().has_tab());
-  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[3].change_type());
-  ASSERT_TRUE(out[3].sync_data().GetSpecifics().session().has_header());
+  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[2].change_type());
+  ASSERT_TRUE(out[2].sync_data().GetSpecifics().session().has_header());
 
   // Verify the actual content.
   const sync_pb::SessionHeader& session_header =
-      out[3].sync_data().GetSpecifics().session().header();
+      out[2].sync_data().GetSpecifics().session().header();
   ASSERT_EQ(1, session_header.window_size());
   EXPECT_EQ(1, session_header.window(0).tab_size());
   const sync_pb::SessionTab& tab1 =
-      out[2].sync_data().GetSpecifics().session().tab();
+      out[1].sync_data().GetSpecifics().session().tab();
   ASSERT_EQ(1, tab1.navigation_size());
   EXPECT_EQ(foo1.spec(), tab1.navigation(0).virtual_url());
 
   // Verify TabNodePool integrity.
-  EXPECT_EQ(1U, manager()->local_tab_pool_.Capacity());
-  EXPECT_TRUE(manager()->local_tab_pool_.Empty());
-
-  // Verify TabLinks.
-  SessionsSyncManager::TabLinksMap tab_map = manager()->local_tab_map_;
-  ASSERT_EQ(1U, tab_map.size());
-  int tab_id = out[2].sync_data().GetSpecifics().session().tab().tab_id();
-  EXPECT_EQ(tab_node_id, tab_map.find(tab_id)->second->tab_node_id());
+  EXPECT_EQ(1U, GetTabPool()->Capacity());
+  EXPECT_TRUE(GetTabPool()->Empty());
 }
 
 // Test that receiving a session delete from sync removes the session
 // from tracking.
 TEST_F(SessionsSyncManagerTest, ProcessForeignDelete) {
   InitWithNoSyncData();
   SessionID::id_type n[] = {5};
   std::vector<sync_pb::SessionSpecifics> tabs1;
   std::vector<SessionID::id_type> tab_list(n, n + arraysize(n));
   sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
@@ skipping 123 matching lines @@
   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, 1, &tab));
   ASSERT_FALSE(
       manager()->session_tracker_.LookupSessionTab(session_tag, 2, &tab));
 
   std::set<int> tab_node_ids;
-  manager()->session_tracker_.LookupTabNodeIds(session_tag, &tab_node_ids);
+  manager()->session_tracker_.LookupForeignTabNodeIds(session_tag,
+                                                      &tab_node_ids);
   EXPECT_EQ(6U, tab_node_ids.size());
   EXPECT_TRUE(tab_node_ids.find(tab1A.tab_node_id()) != tab_node_ids.end());
   EXPECT_TRUE(tab_node_ids.find(tab1B.tab_node_id()) != tab_node_ids.end());
   EXPECT_TRUE(tab_node_ids.find(tab1C.tab_node_id()) != tab_node_ids.end());
   EXPECT_TRUE(tab_node_ids.find(tab2A.tab_node_id()) != tab_node_ids.end());
   EXPECT_TRUE(tab_node_ids.find(tab2B.tab_node_id()) != tab_node_ids.end());
   EXPECT_TRUE(tab_node_ids.find(tab2C.tab_node_id()) != tab_node_ids.end());
 
   syncer::SyncChangeList changes;
   changes.push_back(MakeRemoteChange(tab1A, SyncChange::ACTION_DELETE));
   changes.push_back(MakeRemoteChange(tab1B, SyncChange::ACTION_DELETE));
   changes.push_back(MakeRemoteChange(tab2C, SyncChange::ACTION_DELETE));
   manager()->ProcessSyncChanges(FROM_HERE, changes);
 
   tab_node_ids.clear();
-  manager()->session_tracker_.LookupTabNodeIds(session_tag, &tab_node_ids);
+  manager()->session_tracker_.LookupForeignTabNodeIds(session_tag,
+                                                      &tab_node_ids);
   EXPECT_EQ(3U, tab_node_ids.size());
   EXPECT_TRUE(tab_node_ids.find(tab1C.tab_node_id()) != tab_node_ids.end());
   EXPECT_TRUE(tab_node_ids.find(tab2A.tab_node_id()) != tab_node_ids.end());
   EXPECT_TRUE(tab_node_ids.find(tab2B.tab_node_id()) != tab_node_ids.end());
 
   manager()->DoGarbageCollection();
   ASSERT_EQ(3U, output.size());
 }
 
 TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithReusedNodeIds) {
@@ skipping 17 matching lines @@
   helper()->BuildTabSpecifics(session_tag, 0, 2, tab_node_id_shared, &tab2A);
   AddToSyncDataList(tab2A, &foreign_data,
                     stale_mtime + base::TimeDelta::FromMinutes(1));
 
   syncer::SyncChangeList output;
   InitWithSyncDataTakeOutput(foreign_data, &output);
   ASSERT_EQ(2U, output.size());
   output.clear();
 
   std::set<int> tab_node_ids;
-  manager()->session_tracker_.LookupTabNodeIds(session_tag, &tab_node_ids);
+  manager()->session_tracker_.LookupForeignTabNodeIds(session_tag,
+                                                      &tab_node_ids);
   EXPECT_EQ(2U, tab_node_ids.size());
   EXPECT_TRUE(tab_node_ids.find(tab_node_id_shared) != tab_node_ids.end());
   EXPECT_TRUE(tab_node_ids.find(tab_node_id_unique) != tab_node_ids.end());
 
   syncer::SyncChangeList changes;
   changes.push_back(MakeRemoteChange(tab1A, SyncChange::ACTION_DELETE));
   manager()->ProcessSyncChanges(FROM_HERE, changes);
 
   tab_node_ids.clear();
-  manager()->session_tracker_.LookupTabNodeIds(session_tag, &tab_node_ids);
+  manager()->session_tracker_.LookupForeignTabNodeIds(session_tag,
+                                                      &tab_node_ids);
   EXPECT_EQ(1U, tab_node_ids.size());
   EXPECT_TRUE(tab_node_ids.find(tab_node_id_unique) != tab_node_ids.end());
 
   manager()->DoGarbageCollection();
   EXPECT_EQ(1U, output.size());
 }
 
-// TODO(shashishekhar): "Move this to TabNodePool unittests."
-TEST_F(SessionsSyncManagerTest, SaveUnassociatedNodesForReassociation) {
+TEST_F(SessionsSyncManagerTest, AssociationReusesNodes) {
   syncer::SyncChangeList changes;
-  InitWithNoSyncData();
+  AddTab(browser(), GURL("http://foo1"));
+  InitWithSyncDataTakeOutput(syncer::SyncDataList(), &changes);
+  ASSERT_EQ(3U, changes.size());  // Header add, tab add, header update.
+  ASSERT_TRUE(changes[1].sync_data().GetSpecifics().session().has_tab());
+  int tab_node_id =
+      changes[1].sync_data().GetSpecifics().session().tab_node_id();
 
-  std::string local_tag = manager()->current_machine_tag();
-  // Create a free node and then dissassociate sessions so that it ends up
-  // unassociated.
-  manager()->local_tab_pool_.GetFreeTabNode(&changes);
+  // Pass back the previous tab and header nodes at association, along with a
+  // second tab node (with a rewritten tab node id).
+  syncer::SyncDataList in;
+  in.push_back(
+      CreateRemoteData(changes[2].sync_data().GetSpecifics()));  // Header node.
+  sync_pb::SessionSpecifics new_tab(
+      changes[1].sync_data().GetSpecifics().session());
+  new_tab.set_tab_node_id(tab_node_id + 1);
+  in.push_back(CreateRemoteData(new_tab));  // New tab node.
+  in.push_back(CreateRemoteData(
+      changes[1].sync_data().GetSpecifics()));  // Old tab node.
+  changes.clear();
 
-  // Update the tab_id of the node, so that it is considered a valid
-  // unassociated node otherwise it will be mistaken for a corrupted node and
-  // will be deleted before being added to the tab node pool.
-  sync_pb::EntitySpecifics entity(changes[0].sync_data().GetSpecifics());
-  entity.mutable_session()->mutable_tab()->set_tab_id(1);
-  SyncData d = CreateRemoteData(entity);
-  syncer::SyncDataList in(&d, &d + 1);
-  changes.clear();
-  SessionsSyncManager manager2(GetSyncSessionsClient(), sync_prefs(),
-                               local_device(), NewDummyRouter(),
-                               base::Closure(), base::Closure());
-  syncer::SyncMergeResult result = manager2.MergeDataAndStartSyncing(
+  // Reassociate (with the same single tab/window open).
+  manager()->StopSyncing(syncer::SESSIONS);
+  syncer::SyncMergeResult result = manager()->MergeDataAndStartSyncing(
       syncer::SESSIONS, in, std::unique_ptr<syncer::SyncChangeProcessor>(
                                 new TestSyncProcessorStub(&changes)),
       std::unique_ptr<syncer::SyncErrorFactory>(
           new syncer::SyncErrorFactoryMock()));
   ASSERT_FALSE(result.error().IsSet());
-  EXPECT_TRUE(FilterOutLocalHeaderChanges(&changes)->empty());
+
+  // No tab entities should be deleted. The original (lower) tab node id should
+  // be reused for association.
+  FilterOutLocalHeaderChanges(&changes);
+  ASSERT_EQ(1U, changes.size());
+  EXPECT_EQ(SyncChange::ACTION_UPDATE, changes[0].change_type());
+  EXPECT_TRUE(changes[0].sync_data().GetSpecifics().session().has_tab());
+  EXPECT_EQ(tab_node_id,
+            changes[0].sync_data().GetSpecifics().session().tab_node_id());
 }
 
 TEST_F(SessionsSyncManagerTest, MergeDeletesCorruptNode) {
   syncer::SyncChangeList changes;
   InitWithNoSyncData();
 
   std::string local_tag = manager()->current_machine_tag();
-  int tab_node_id = manager()->local_tab_pool_.GetFreeTabNode(&changes);
-  SyncData d = CreateRemoteData(changes[0].sync_data().GetSpecifics());
+  int tab_node_id = TabNodePool::kInvalidTabNodeID;
+  GetTabPool()->GetTabNodeForTab(0, &tab_node_id);
+  sync_pb::SessionSpecifics specifics;
+  specifics.set_session_tag(local_tag);
+  specifics.set_tab_node_id(tab_node_id);
+  SyncData d = CreateRemoteData(specifics);
   syncer::SyncDataList in(&d, &d + 1);
-  changes.clear();
   TearDown();
   SetUp();
   InitWithSyncDataTakeOutput(in, &changes);
   EXPECT_EQ(1U, FilterOutLocalHeaderChanges(&changes)->size());
   EXPECT_EQ(SyncChange::ACTION_DELETE, changes[0].change_type());
-  EXPECT_EQ(TabNodePool::TabIdToTag(local_tag, tab_node_id),
+  EXPECT_EQ(TabNodeIdToTag(local_tag, tab_node_id),
             syncer::SyncDataLocal(changes[0].sync_data()).GetTag());
 }
 
 // Verifies that we drop both headers and tabs during merge if their stored tag
 // hash doesn't match a computer tag hash. This mitigates potential failures
 // while cleaning up bad foreign data, see crbug.com/604657.
 TEST_F(SessionsSyncManagerTest, MergeDeletesBadHash) {
   syncer::SyncDataList foreign_data;
   std::vector<SessionID::id_type> empty_ids;
   std::vector<sync_pb::SessionSpecifics> empty_tabs;
@@ skipping 48 matching lines @@
   InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out);
   ASSERT_EQ(2U, out.size());  // Header add and update.
   EXPECT_EQ(
       0,
       out[1].sync_data().GetSpecifics().session().header().window_size());
   out.clear();
 
   // Go to a sync-interesting URL.
   NavigateAndCommitActiveTab(GURL("http://foo2"));
 
-  EXPECT_EQ(3U, out.size());  // Tab add, update, and header update.
+  EXPECT_EQ(2U, out.size());  // Tab add and header update.
 
   EXPECT_TRUE(
       base::StartsWith(syncer::SyncDataLocal(out[0].sync_data()).GetTag(),
                        manager()->current_machine_tag(),
                        base::CompareCase::SENSITIVE));
   EXPECT_EQ(manager()->current_machine_tag(),
             out[0].sync_data().GetSpecifics().session().session_tag());
   EXPECT_EQ(SyncChange::ACTION_ADD, out[0].change_type());
 
-  EXPECT_TRUE(
-      base::StartsWith(syncer::SyncDataLocal(out[1].sync_data()).GetTag(),
-                       manager()->current_machine_tag(),
-                       base::CompareCase::SENSITIVE));
-  EXPECT_EQ(manager()->current_machine_tag(),
-            out[1].sync_data().GetSpecifics().session().session_tag());
-  EXPECT_TRUE(out[1].sync_data().GetSpecifics().session().has_tab());
+  EXPECT_TRUE(out[1].IsValid());
   EXPECT_EQ(SyncChange::ACTION_UPDATE, out[1].change_type());
-
-  EXPECT_TRUE(out[2].IsValid());
-  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[2].change_type());
-  const SyncData data(out[2].sync_data());
+  const SyncData data(out[1].sync_data());
   EXPECT_EQ(manager()->current_machine_tag(),
             syncer::SyncDataLocal(data).GetTag());
   const sync_pb::SessionSpecifics& specifics(data.GetSpecifics().session());
   EXPECT_EQ(manager()->current_machine_tag(), specifics.session_tag());
   EXPECT_TRUE(specifics.has_header());
   const sync_pb::SessionHeader& header_s = specifics.header();
   EXPECT_EQ(1, header_s.window_size());
   EXPECT_EQ(1, header_s.window(0).tab_size());
 }
 
@@ skipping 11 matching lines @@
 
   const GURL foo1("http://foo/1");
   const GURL foo2("http://foo/2");
   const GURL bar1("http://bar/1");
   const GURL bar2("http://bar/2");
   AddTab(browser(), foo1);
   NavigateAndCommitActiveTab(foo2);
   AddTab(browser(), bar1);
   NavigateAndCommitActiveTab(bar2);
 
-  // One add, one update for each AddTab.
+  // One add for each AddTab.
   // One update for each NavigateAndCommit.
-  // = 6 total tab updates.
+  // = 4 total tab updates.
   // One header update corresponding to each of those.
   // = 6 total header updates.
-  // 12 total updates.
-  ASSERT_EQ(12U, out.size());
+  // 10 total updates.
+  ASSERT_EQ(10U, out.size());
 
   // Verify the tab node creations and updates to ensure the SyncProcessor
   // sees the right operations.
-  for (int i = 0; i < 12; i++) {
+  for (int i = 0; i < 10; i++) {

[Patrick Noland, 2017/01/25 19:23:41]

Can you make 5 and 10 variables with names? Tracking the meaning throughout the
function is tricky

[Nicolas Zea, 2017/01/25 23:55:04]

Done.

     SCOPED_TRACE(i);
     EXPECT_TRUE(out[i].IsValid());
     const SyncData data(out[i].sync_data());
     EXPECT_TRUE(base::StartsWith(syncer::SyncDataLocal(data).GetTag(),
                                  manager()->current_machine_tag(),
                                  base::CompareCase::SENSITIVE));
     const sync_pb::SessionSpecifics& specifics(data.GetSpecifics().session());
     EXPECT_EQ(manager()->current_machine_tag(), specifics.session_tag());
-    if (i % 6 == 0) {
+    if (i % 5 == 0) {
       // First thing on an AddTab is a no-op header update for parented tab.
       EXPECT_EQ(header.SerializeAsString(),
                 data.GetSpecifics().SerializeAsString());
       EXPECT_EQ(manager()->current_machine_tag(),
                 syncer::SyncDataLocal(data).GetTag());
-    } else if (i % 6 == 1) {
-      // Next, the TabNodePool should create the tab node.
+    } else if (i % 5 == 1) {
+      // Next, the tab should be added.
       EXPECT_EQ(SyncChange::ACTION_ADD, out[i].change_type());
-      EXPECT_EQ(TabNodePool::TabIdToTag(
-                    manager()->current_machine_tag(),
-                    data.GetSpecifics().session().tab_node_id()),
+      EXPECT_EQ(TabNodeIdToTag(manager()->current_machine_tag(),
+                               data.GetSpecifics().session().tab_node_id()),
                 syncer::SyncDataLocal(data).GetTag());
-    } else if (i % 6 == 2) {
-      // Then we see the tab update to the URL.
-      EXPECT_EQ(SyncChange::ACTION_UPDATE, out[i].change_type());
-      EXPECT_EQ(TabNodePool::TabIdToTag(
-                    manager()->current_machine_tag(),
-                    data.GetSpecifics().session().tab_node_id()),
-                syncer::SyncDataLocal(data).GetTag());
-      ASSERT_TRUE(specifics.has_tab());
-    } else if (i % 6 == 3) {
+    } else if (i % 5 == 2) {
       // The header needs to be updated to reflect the new window state.
       EXPECT_EQ(SyncChange::ACTION_UPDATE, out[i].change_type());
       EXPECT_TRUE(specifics.has_header());
-    } else if (i % 6 == 4) {
+    } else if (i % 5 == 3) {
       // Now we move on to NavigateAndCommit.  Update the tab.
       EXPECT_EQ(SyncChange::ACTION_UPDATE, out[i].change_type());
-      EXPECT_EQ(TabNodePool::TabIdToTag(
-                    manager()->current_machine_tag(),
-                    data.GetSpecifics().session().tab_node_id()),
+      EXPECT_EQ(TabNodeIdToTag(manager()->current_machine_tag(),
+                               data.GetSpecifics().session().tab_node_id()),
                 syncer::SyncDataLocal(data).GetTag());
       ASSERT_TRUE(specifics.has_tab());
-    } else if (i % 6 == 5) {
+    } else if (i % 5 == 4) {
       // The header needs to be updated to reflect the new window state.
       EXPECT_EQ(SyncChange::ACTION_UPDATE, out[i].change_type());
       ASSERT_TRUE(specifics.has_header());
       header = data.GetSpecifics();
     }
   }
 
   // Verify the actual content to ensure sync sees the right data.
   // When it's all said and done, the header should reflect two tabs.
   const sync_pb::SessionHeader& session_header = header.session().header();
   ASSERT_EQ(1, session_header.window_size());
   EXPECT_EQ(2, session_header.window(0).tab_size());
 
   // ASSERT_TRUEs above allow us to dive in freely here.
   // Verify first tab.
   const sync_pb::SessionTab& tab1_1 =
-      out[2].sync_data().GetSpecifics().session().tab();
+      out[1].sync_data().GetSpecifics().session().tab();
   ASSERT_EQ(1, tab1_1.navigation_size());
   EXPECT_EQ(foo1.spec(), tab1_1.navigation(0).virtual_url());
   const sync_pb::SessionTab& tab1_2 =
-      out[4].sync_data().GetSpecifics().session().tab();
+      out[3].sync_data().GetSpecifics().session().tab();
   ASSERT_EQ(2, tab1_2.navigation_size());
   EXPECT_EQ(foo1.spec(), tab1_2.navigation(0).virtual_url());
   EXPECT_EQ(foo2.spec(), tab1_2.navigation(1).virtual_url());
 
   // Verify second tab.
   const sync_pb::SessionTab& tab2_1 =
-      out[8].sync_data().GetSpecifics().session().tab();
+      out[6].sync_data().GetSpecifics().session().tab();
   ASSERT_EQ(1, tab2_1.navigation_size());
   EXPECT_EQ(bar1.spec(), tab2_1.navigation(0).virtual_url());
   const sync_pb::SessionTab& tab2_2 =
-      out[10].sync_data().GetSpecifics().session().tab();
+      out[8].sync_data().GetSpecifics().session().tab();
   ASSERT_EQ(2, tab2_2.navigation_size());
   EXPECT_EQ(bar1.spec(), tab2_2.navigation(0).virtual_url());
   EXPECT_EQ(bar2.spec(), tab2_2.navigation(1).virtual_url());
+
+  // Verify tab delegates have Sync ids.
+  std::set<const SyncedWindowDelegate*> window_delegates =
+      get_synced_window_getter()->GetSyncedWindowDelegates();
+  // Sync ids are in reverse order because tabs are inserted at the beginning
+  // of the tab list.
+  EXPECT_EQ(1, (*window_delegates.begin())->GetTabAt(0)->GetSyncId());
+  EXPECT_EQ(0, (*window_delegates.begin())->GetTabAt(1)->GetSyncId());
 }
 
 // Check that if a tab becomes uninteresting (for example no syncable URLs),
 // we correctly remove it from the header node.
 TEST_F(SessionsSyncManagerTest, TabBecomesUninteresting) {
   syncer::SyncChangeList out;
   // Init with no local data, relies on MergeLocalSessionNoTabs.
   InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out);
   ASSERT_FALSE(manager()->current_machine_tag().empty());
   ASSERT_EQ(2U, out.size());
   out.clear();
 
   const GURL kValidUrl("http://foo/1");
   const GURL kInternalUrl("chrome://internal");
 
   // Add an interesting tab.
   AddTab(browser(), kValidUrl);
-  // No-op header update, tab creation, tab update, header update.
-  ASSERT_EQ(4U, out.size());
+  // No-op header update, tab creation, header update.
+  ASSERT_EQ(3U, out.size());
   // The last two are the interesting updates.
-  ASSERT_TRUE(out[2].sync_data().GetSpecifics().session().has_tab());
-  EXPECT_EQ(kValidUrl.spec(), out[2]
+  ASSERT_TRUE(out[1].sync_data().GetSpecifics().session().has_tab());
+  EXPECT_EQ(kValidUrl.spec(), out[1]
                                   .sync_data()
                                   .GetSpecifics()
                                   .session()
                                   .tab()
                                   .navigation(0)
                                   .virtual_url());
-  ASSERT_TRUE(out[3].sync_data().GetSpecifics().session().has_header());
+  ASSERT_TRUE(out[2].sync_data().GetSpecifics().session().has_header());
   ASSERT_EQ(1,
-            out[3].sync_data().GetSpecifics().session().header().window_size());
-  ASSERT_EQ(1, out[3]
+            out[2].sync_data().GetSpecifics().session().header().window_size());
+  ASSERT_EQ(1, out[2]
                    .sync_data()
                    .GetSpecifics()
                    .session()
                    .header()
                    .window(0)
                    .tab_size());
 
   // Navigate five times to uninteresting urls to push the interesting one off
   // the back of the stack.
   NavigateAndCommitActiveTab(kInternalUrl);
@@ skipping 15 matching lines @@
 
 // Ensure model association associates the pre-existing tabs.
 TEST_F(SessionsSyncManagerTest, MergeLocalSessionExistingTabs) {
   AddTab(browser(), GURL("http://foo1"));
   NavigateAndCommitActiveTab(GURL("http://foo2"));  // Adds back entry.
   AddTab(browser(), GURL("http://bar1"));
   NavigateAndCommitActiveTab(GURL("http://bar2"));  // Adds back entry.
 
   syncer::SyncChangeList out;
   InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out);
-  ASSERT_EQ(6U, out.size());
+  ASSERT_EQ(4U, out.size());  // Header creation, add two tabs, header update
 
   // Check that this machine's data is not included in the foreign windows.
   std::vector<const SyncedSession*> foreign_sessions;
   ASSERT_FALSE(manager()->GetAllForeignSessions(&foreign_sessions));
 
   // Verify the header.
   EXPECT_TRUE(out[0].IsValid());
   EXPECT_EQ(SyncChange::ACTION_ADD, out[0].change_type());
   const SyncData data(out[0].sync_data());
   EXPECT_EQ(manager()->current_machine_tag(),
             syncer::SyncDataLocal(data).GetTag());
   const sync_pb::SessionSpecifics& specifics(data.GetSpecifics().session());
   EXPECT_EQ(manager()->current_machine_tag(), specifics.session_tag());
   EXPECT_TRUE(specifics.has_header());
   const sync_pb::SessionHeader& header_s = specifics.header();
   EXPECT_TRUE(header_s.has_device_type());
   EXPECT_EQ(GetLocalDeviceInfo()->client_name(), header_s.client_name());
   EXPECT_EQ(0, header_s.window_size());
 
   // Verify the tab node creations and updates with content.
-  for (int i = 1; i < 5; i++) {
+  for (int i = 1; i < 3; i++) {
     EXPECT_TRUE(out[i].IsValid());
     const SyncData data(out[i].sync_data());
     EXPECT_TRUE(base::StartsWith(syncer::SyncDataLocal(data).GetTag(),
                                  manager()->current_machine_tag(),
                                  base::CompareCase::SENSITIVE));
     const sync_pb::SessionSpecifics& specifics(data.GetSpecifics().session());
     EXPECT_EQ(manager()->current_machine_tag(), specifics.session_tag());
-    if (i % 2 == 1) {
-      EXPECT_EQ(SyncChange::ACTION_ADD, out[i].change_type());
-    } else {
-      EXPECT_EQ(SyncChange::ACTION_UPDATE, out[i].change_type());
-      EXPECT_TRUE(specifics.has_tab());
-    }
+    EXPECT_EQ(SyncChange::ACTION_ADD, out[i].change_type());
   }
 
   // Verify the header was updated to reflect new window state.
-  EXPECT_TRUE(out[5].IsValid());
-  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[5].change_type());
-  const SyncData data_2(out[5].sync_data());
+  EXPECT_TRUE(out[3].IsValid());
+  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[3].change_type());
+  const SyncData data_2(out[3].sync_data());
   EXPECT_EQ(manager()->current_machine_tag(),
             syncer::SyncDataLocal(data_2).GetTag());
   const sync_pb::SessionSpecifics& specifics2(data_2.GetSpecifics().session());
   EXPECT_EQ(manager()->current_machine_tag(), specifics2.session_tag());
   EXPECT_TRUE(specifics2.has_header());
   const sync_pb::SessionHeader& header_s2 = specifics2.header();
   EXPECT_EQ(1, header_s2.window_size());
 
-  // Verify TabLinks.
-  SessionsSyncManager::TabLinksMap tab_map = manager()->local_tab_map_;
-  ASSERT_EQ(2U, tab_map.size());
-  // Tabs are ordered by sessionid in tab_map, so should be able to traverse
-  // the tree based on order of tabs created
-  SessionsSyncManager::TabLinksMap::iterator iter = tab_map.begin();
-  ASSERT_EQ(2, iter->second->tab()->GetEntryCount());
-  EXPECT_EQ(GURL("http://foo1"), iter->second->tab()->GetVirtualURLAtIndex(0));
-  EXPECT_EQ(GURL("http://foo2"), iter->second->tab()->GetVirtualURLAtIndex(1));
-  iter++;
-  ASSERT_EQ(2, iter->second->tab()->GetEntryCount());
-  EXPECT_EQ(GURL("http://bar1"), iter->second->tab()->GetVirtualURLAtIndex(0));
-  EXPECT_EQ(GURL("http://bar2"), iter->second->tab()->GetVirtualURLAtIndex(1));
+  // Verify tab delegates have Sync ids.
+  std::set<const SyncedWindowDelegate*> window_delegates =
+      get_synced_window_getter()->GetSyncedWindowDelegates();
+  // Sync ids are in same order as tabs because the association happens after
+  // the tabs are opened (and therefore iterates through same order).
+  EXPECT_EQ(0, (*window_delegates.begin())->GetTabAt(0)->GetSyncId());
+  EXPECT_EQ(1, (*window_delegates.begin())->GetTabAt(1)->GetSyncId());
 }
 
 TEST_F(SessionsSyncManagerTest, ForeignSessionModifiedTime) {
   syncer::SyncDataList foreign_data;
   base::Time newest_time = base::Time::Now() - base::TimeDelta::FromDays(1);
   base::Time middle_time = base::Time::Now() - base::TimeDelta::FromDays(2);
   base::Time oldest_time = base::Time::Now() - base::TimeDelta::FromDays(3);
 
   {
     std::string session_tag = "tag1";
@@ skipping 82 matching lines @@
 
   // Now garbage collect and verify the non-stale session is still there.
   manager()->DoGarbageCollection();
   ASSERT_EQ(5U, output.size());
   EXPECT_EQ(SyncChange::ACTION_DELETE, output[0].change_type());
   const SyncData data(output[0].sync_data());
   EXPECT_EQ(tag1, syncer::SyncDataLocal(data).GetTag());
   for (int i = 1; i < 5; i++) {
     EXPECT_EQ(SyncChange::ACTION_DELETE, output[i].change_type());
     const SyncData data(output[i].sync_data());
-    EXPECT_EQ(TabNodePool::TabIdToTag(tag1, i),
-              syncer::SyncDataLocal(data).GetTag());
+    EXPECT_EQ(TabNodeIdToTag(tag1, i), syncer::SyncDataLocal(data).GetTag());
   }
 
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
   ASSERT_EQ(1U, foreign_sessions.size());
   std::vector<std::vector<SessionID::id_type> > session_reference;
   session_reference.push_back(tab_list2);
   helper()->VerifySyncedSession(tag2, session_reference,
                                 *(foreign_sessions[0]));
 }
 
@@ skipping 102 matching lines @@
 }
 
 // Test that swapping WebContents for a tab is properly observed and handled
 // by the SessionsSyncManager.
 TEST_F(SessionsSyncManagerTest, CheckPrerenderedWebContentsSwap) {
   AddTab(browser(), GURL("http://foo1"));
   NavigateAndCommitActiveTab(GURL("http://foo2"));
 
   syncer::SyncChangeList out;
   InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out);
-  ASSERT_EQ(4U, out.size());  // Header, tab ADD, tab UPDATE, header UPDATE.
+  ASSERT_EQ(3U, out.size());  // Header ADD, tab ADD, header UPDATE.
+  out.clear();
 
   // To simulate WebContents swap during prerendering, create new WebContents
   // and swap with old WebContents.
   std::unique_ptr<content::WebContents> old_web_contents;
   old_web_contents.reset(browser()->tab_strip_model()->GetActiveWebContents());
 
   // Create new WebContents, with the required tab helpers.
   WebContents* new_web_contents = WebContents::CreateWithSessionStorage(
       WebContents::CreateParams(profile()),
       old_web_contents->GetController().GetSessionStorageNamespaceMap());
   SessionTabHelper::CreateForWebContents(new_web_contents);
   TabContentsSyncedTabDelegate::CreateForWebContents(new_web_contents);
   new_web_contents->GetController()
       .CopyStateFrom(old_web_contents->GetController());
 
   // Swap the WebContents.
   int index = browser()->tab_strip_model()->GetIndexOfWebContents(
       old_web_contents.get());
   browser()->tab_strip_model()->ReplaceWebContentsAt(index, new_web_contents);
 
-  ASSERT_EQ(9U, out.size());
-  EXPECT_EQ(SyncChange::ACTION_ADD, out[4].change_type());
-  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[5].change_type());
+  ASSERT_EQ(4U, out.size());
+  EXPECT_EQ(SyncChange::ACTION_ADD, out[0].change_type());
+  out.clear();
 
-  // Navigate away.
+  // Navigate away. +1 tab updates, 1 header update.
   NavigateAndCommitActiveTab(GURL("http://bar2"));
 
   // Delete old WebContents. This should not crash.
+  // +1 no-op header update.
   old_web_contents.reset();
 
   // Try more navigations and verify output size. This can also reveal
   // bugs (leaks) on memcheck bots if the SessionSyncManager
   // didn't properly clean up the tab pool or session tracker.
+  // +1 tab updates, 1 header update.
   NavigateAndCommitActiveTab(GURL("http://bar3"));
 
+  // +1 no-op header update, tab add, header update.
   AddTab(browser(), GURL("http://bar4"));
+  // +1 tab update, header update.
   NavigateAndCommitActiveTab(GURL("http://bar5"));
-  ASSERT_EQ(19U, out.size());
+  ASSERT_EQ(10U, out.size());
 }
 
 // Test that NOTIFICATION_FOREIGN_SESSION_UPDATED is sent when processing
 // sync changes.
 TEST_F(SessionsSyncManagerTest, NotifiedOfUpdates) {
   ASSERT_FALSE(observer()->notified_of_update());
   InitWithNoSyncData();
 
   SessionID::id_type n[] = {5};
   std::vector<sync_pb::SessionSpecifics> tabs1;
@@ skipping 248 matching lines @@
   base::Time last_time;
   for (size_t i = 0; i < tabs.size(); ++i) {
     base::Time this_time = tabs[i]->timestamp;
     if (i > 0)
       ASSERT_GE(last_time, this_time);
     last_time = tabs[i]->timestamp;
   }
 }
 
 }  // namespace sync_sessions