Reland v3 of Session refactor

chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc

Index: chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
diff --git a/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc b/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
index 886c2a4bf72a1cf615ff1244ab5abf6a85fbddbd..0226be9100654b0c563f51e9275b2a4402b5a362 100644
--- a/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
+++ b/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
@@ -48,6 +48,10 @@ 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()
@@ -128,7 +132,7 @@ class SyncedWindowDelegateOverride : public SyncedWindowDelegate {
   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 {
@@ -399,6 +403,10 @@ class SessionsSyncManagerTest
     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() {
@@ -502,40 +510,32 @@ TEST_F(SessionsSyncManagerTest, PopulateSessionWindow) {
 
 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 {
@@ -545,17 +545,9 @@ class SyncedTabDelegateFake : public SyncedTabDelegate {
         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_; }
@@ -564,6 +556,23 @@ class SyncedTabDelegateFake : public SyncedTabDelegate {
   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) {
@@ -574,31 +583,101 @@ class SyncedTabDelegateFake : public SyncedTabDelegate {
       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);
@@ -953,29 +1032,37 @@ TEST_F(SessionsSyncManagerTest, MergeLocalSessionNoTabs) {
 
 // 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));
@@ -986,9 +1073,8 @@ TEST_F(SessionsSyncManagerTest, SwappedOutOnRestore) {
   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,
@@ -1006,50 +1092,66 @@ TEST_F(SessionsSyncManagerTest, SwappedOutOnRestore) {
           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 =
@@ -1078,6 +1180,66 @@ TEST_F(SessionsSyncManagerTest, WindowIdUpdatedOnRestore) {
   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.
@@ -1140,7 +1302,8 @@ TEST_F(SessionsSyncManagerTest, MergeWithLocalAndForeignTabs) {
 
   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());
@@ -1168,12 +1331,12 @@ TEST_F(SessionsSyncManagerTest, MergeWithLocalAndForeignTabs) {
   }
   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());
@@ -1216,7 +1379,9 @@ TEST_F(SessionsSyncManagerTest, UpdatesAfterMixedMerge) {
 
   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.
@@ -1307,10 +1472,10 @@ TEST_F(SessionsSyncManagerTest, DeleteForeignSession) {
       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());
@@ -1322,7 +1487,7 @@ TEST_F(SessionsSyncManagerTest, DeleteForeignSession) {
   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());
@@ -1445,40 +1610,32 @@ TEST_F(SessionsSyncManagerTest, ProcessRemoteDeleteOfLocalSession) {
   // 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
@@ -1622,7 +1779,8 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithShadowing) {
       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());
@@ -1638,7 +1796,8 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithShadowing) {
   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());
@@ -1676,7 +1835,8 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithReusedNodeIds) {
   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());
@@ -1686,7 +1846,8 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithReusedNodeIds) {
   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());
 
@@ -1694,34 +1855,45 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithReusedNodeIds) {
   EXPECT_EQ(1U, output.size());
 }
 
-// TODO(shashishekhar): "Move this to TabNodePool unittests."
-TEST_F(SessionsSyncManagerTest, SaveUnassociatedNodesForReassociation) {
+TEST_F(SessionsSyncManagerTest, AssociationReusesNodes) {
   syncer::SyncChangeList changes;
-  InitWithNoSyncData();
-
-  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);
-
-  // 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);
+  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();
+
+  // 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();
-  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) {
@@ -1729,16 +1901,19 @@ TEST_F(SessionsSyncManagerTest, MergeDeletesCorruptNode) {
   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());
 }
 
@@ -1807,7 +1982,7 @@ TEST_F(SessionsSyncManagerTest, AssociateWindowsDontReloadTabs) {
   // 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(),
@@ -1817,18 +1992,9 @@ TEST_F(SessionsSyncManagerTest, AssociateWindowsDontReloadTabs) {
             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());
@@ -1860,17 +2026,17 @@ TEST_F(SessionsSyncManagerTest, OnLocalTabModified) {
   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());
@@ -1879,40 +2045,30 @@ TEST_F(SessionsSyncManagerTest, OnLocalTabModified) {
                                  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());
@@ -1929,25 +2085,33 @@ TEST_F(SessionsSyncManagerTest, OnLocalTabModified) {
   // 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),
@@ -1965,21 +2129,21 @@ TEST_F(SessionsSyncManagerTest, TabBecomesUninteresting) {
 
   // 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()
@@ -2015,7 +2179,7 @@ TEST_F(SessionsSyncManagerTest, MergeLocalSessionExistingTabs) {
 
   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;
@@ -2036,7 +2200,7 @@ TEST_F(SessionsSyncManagerTest, MergeLocalSessionExistingTabs) {
   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(),
@@ -2044,18 +2208,13 @@ TEST_F(SessionsSyncManagerTest, MergeLocalSessionExistingTabs) {
                                  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());
@@ -2064,19 +2223,13 @@ TEST_F(SessionsSyncManagerTest, MergeLocalSessionExistingTabs) {
   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) {
@@ -2179,8 +2332,7 @@ TEST_F(SessionsSyncManagerTest, DoGarbageCollection) {
   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));
@@ -2303,7 +2455,8 @@ TEST_F(SessionsSyncManagerTest, CheckPrerenderedWebContentsSwap) {
 
   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.
@@ -2324,24 +2477,28 @@ TEST_F(SessionsSyncManagerTest, CheckPrerenderedWebContentsSwap) {
       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