[Sync] Refactor SessionsSyncManager unit tests

components/sync_sessions/sessions_sync_manager_unittest.cc

Index: components/sync_sessions/sessions_sync_manager_unittest.cc
diff --git a/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc b/components/sync_sessions/sessions_sync_manager_unittest.cc
similarity index 51%
rename from chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
rename to components/sync_sessions/sessions_sync_manager_unittest.cc
index 886c2a4bf72a1cf615ff1244ab5abf6a85fbddbd..521f8a5a0b622e9818819d284265a4327ba6f3e7 100644
--- a/chrome/browser/sync/sessions/sessions_sync_manager_unittest.cc
+++ b/components/sync_sessions/sessions_sync_manager_unittest.cc
@@ -10,44 +10,135 @@
 
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "build/build_config.h"
-#include "chrome/browser/sessions/session_tab_helper.h"
-#include "chrome/browser/sync/chrome_sync_client.h"
-#include "chrome/browser/sync/sessions/notification_service_sessions_router.h"
-#include "chrome/browser/ui/sync/browser_synced_window_delegates_getter.h"
-#include "chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
-#include "chrome/test/base/browser_with_test_window_test.h"
-#include "components/sessions/content/content_serialized_navigation_builder.h"
 #include "components/sessions/core/serialized_navigation_entry_test_helper.h"
 #include "components/sync/device_info/local_device_info_provider_mock.h"
+#include "components/sync/driver/fake_sync_client.h"
 #include "components/sync/driver/sync_api_component_factory.h"
 #include "components/sync/model/attachments/attachment_id.h"
 #include "components/sync/model/attachments/attachment_service_proxy_for_test.h"
 #include "components/sync/model/sync_error_factory_mock.h"
+#include "components/sync_sessions/fake_sync_sessions_client.h"
 #include "components/sync_sessions/session_sync_test_helper.h"
 #include "components/sync_sessions/sync_sessions_client.h"
 #include "components/sync_sessions/synced_tab_delegate.h"
 #include "components/sync_sessions/synced_window_delegate.h"
 #include "components/sync_sessions/synced_window_delegates_getter.h"
-#include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/web_contents.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using content::WebContents;
 using sessions::SerializedNavigationEntry;
 using sessions::SerializedNavigationEntryTestHelper;
 using syncer::DeviceInfo;
 using syncer::LocalDeviceInfoProvider;
 using syncer::LocalDeviceInfoProviderMock;
 using syncer::SyncChange;
+using syncer::SyncChangeList;
 using syncer::SyncData;
+using syncer::SyncDataList;
+using syncer::SyncDataLocal;
 
 namespace sync_sessions {
 
 namespace {
 
+const char kTitle[] = "title";
+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 char kTag1[] = "tag1";
+const char kTag2[] = "tag2";
+const int kTabIds1[] = {5, 10, 13, 17};
+const int kTabIds2[] = {7, 15, 18, 20};
+
+const base::Time kTime0 = base::Time::FromInternalValue(100);

[skym, 2017/02/22 22:25:15]

Is there a reason this isn't 0,1,2,3,... ? Just to make it more obvious you hit
timestamps if you print them out or something?

[Nicolas Zea, 2017/02/22 22:58:40]

Exactly.

+const base::Time kTime1 = base::Time::FromInternalValue(110);
+const base::Time kTime2 = base::Time::FromInternalValue(120);
+const base::Time kTime3 = base::Time::FromInternalValue(130);
+const base::Time kTime4 = base::Time::FromInternalValue(140);
+const base::Time kTime5 = base::Time::FromInternalValue(150);
+const base::Time kTime6 = base::Time::FromInternalValue(160);
+const base::Time kTime7 = base::Time::FromInternalValue(170);
+const base::Time kTime8 = base::Time::FromInternalValue(180);
+const base::Time kTime9 = base::Time::FromInternalValue(190);
+
+std::string TabNodeIdToTag(const std::string& machine_tag, int tab_node_id) {
+  return base::StringPrintf("%s %d", machine_tag.c_str(), tab_node_id);
+}
+
+size_t CountIfTagMatches(const SyncChangeList& changes,
+                         const std::string& tag) {
+  return std::count_if(
+      changes.begin(), changes.end(), [&tag](const SyncChange& change) {
+        return change.sync_data().GetSpecifics().session().session_tag() == tag;
+      });
+}
+
+size_t CountIfTagMatches(const std::vector<const SyncedSession*>& sessions,
+                         const std::string& tag) {
+  return std::count_if(sessions.begin(), sessions.end(),
+                       [&tag](const SyncedSession* session) {
+                         return session->session_tag == tag;
+                       });
+}
+
+testing::AssertionResult AllOfChangesAreType(
+    const SyncChangeList& changes,
+    const SyncChange::SyncChangeType type) {
+  for (const SyncChange& change : changes) {

[skym, 2017/02/22 22:25:11]

What? No fancy lambda? Could use find_if :)

[Nicolas Zea, 2017/02/22 22:58:40]

Good call!

+    if (type != change.change_type()) {
+      return testing::AssertionFailure()
+             << change.ToString() << " doesn't match "
+             << SyncChange::ChangeTypeToString(type);
+    }
+  }
+  return testing::AssertionSuccess();
+}
+
+testing::AssertionResult ChangeTypeMatches(

[skym, 2017/02/22 22:25:11]

This method is kind of odd. I don't like that we're tying each test case to a
specific ordering of changes. Something like map<url, vector<change type>> would
be reasonable, though i guess it doesn't work for headers.  multi_set<change
type> would also make me feel better.

[Nicolas Zea, 2017/02/22 22:58:41]

But the ordering matters. If we for example do an update for something before
adding it, the real change processor (GenericChangeProcessor) will trigger an
error).

[skym, 2017/02/22 23:17:26]

Yes, the order matters for a specific tab. But the order between the tab and the
window shouldn't matter. But I suppose this whole file is kind of making some
assumptions about the underlying implementation. 

+    const SyncChangeList& changes,
+    const std::vector<SyncChange::SyncChangeType>& types) {
+  auto types_iter = types.begin();
+  if (types.size() != static_cast<size_t>(std::count_if(

[skym, 2017/02/22 22:25:11]

std::any_if with a negated version of the body would exit early.

[Nicolas Zea, 2017/02/22 22:58:40]

Nice, done.

+                          changes.begin(), changes.end(),
+                          [&types_iter](const SyncChange& change) {
+                            SCOPED_TRACE(change.ToString());
+                            return change.change_type() == *types_iter++;

[skym, 2017/02/22 22:25:12]

Erm, if |types| is shorter than |changes|, bad things happen here, right? And
|types| could have extra elements on the end and we wouldn't notice?

[Nicolas Zea, 2017/02/22 22:58:40]

Fixed by adding an extra size condition (coupled with the any_if)

+                          }))) {
+    std::string type_string;
+    std::for_each(types.begin(), types.end(),
+                  [&type_string](const SyncChange::SyncChangeType& type) {
+                    (type_string) += SyncChange::ChangeTypeToString(type) + " ";
+                  });
+    std::string change_string;
+    std::for_each(changes.begin(), changes.end(),
+                  [&change_string](const SyncChange& change) {
+                    change_string += change.ToString();
+                  });
+    return testing::AssertionFailure()
+           << "Change type mismatch: " << type_string << " vs "
+           << change_string;
+  }
+  return testing::AssertionSuccess();
+}
+
+// Creates a field trial with the specified |trial_name| and |group_name| and
+// registers an associated |variation_id| for it for the given |service|.
+void CreateAndActivateFieldTrial(const std::string& trial_name,
+                                 const std::string& group_name,
+                                 variations::VariationID variation_id,
+                                 variations::IDCollectionKey service) {
+  base::FieldTrialList::CreateFieldTrial(trial_name, group_name);
+  variations::AssociateGoogleVariationID(service, trial_name, group_name,
+                                         variation_id);
+  // Access the trial to activate it.
+  base::FieldTrialList::FindFullName(trial_name);
+}
+
 class SessionNotificationObserver {
  public:
   SessionNotificationObserver()
@@ -68,74 +159,246 @@ class SessionNotificationObserver {
   bool notified_of_refresh_;
 };
 
-class SyncedWindowDelegateOverride : public SyncedWindowDelegate {
+// A SyncedTabDelegate fake for testing. It simulates a normal
+// SyncedTabDelegate with a proper WebContents. For a SyncedTabDelegate without
+// a WebContents, see PlaceholderTabDelegate below.
+class TestSyncedTabDelegate : public SyncedTabDelegate {
  public:
-  explicit SyncedWindowDelegateOverride(const SyncedWindowDelegate* wrapped)
-      : wrapped_(wrapped) {
+  TestSyncedTabDelegate() {}
+  ~TestSyncedTabDelegate() 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_; }
+  GURL GetVirtualURLAtIndex(int i) const override {
+    if (static_cast<size_t>(i) >= entries_.size())
+      return GURL();
+    return entries_[i]->virtual_url();
+  }
+  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]->transition_type();
+  }
+  void GetSerializedNavigationAtIndex(
+      int i,
+      sessions::SerializedNavigationEntry* serialized_entry) const override {
+    if (static_cast<size_t>(i) >= entries_.size())
+      return;
+    *serialized_entry = *entries_[i];
+  }
+  int GetEntryCount() const override { return entries_.size(); }
+  SessionID::id_type GetWindowId() const override { return window_id_.id(); }
+  SessionID::id_type GetSessionId() const override { return tab_id_.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 {
+    // This is just a simple filter that isn't meant to fully reproduce
+    // the TabContentsTabDelegate's ShouldSync logic.
+    // Verify all URL's are valid (which will ignore an initial blank page) and
+    // that there is at least one http:// url.
+    int http_count = 0;
+    for (auto& entry : entries_) {
+      if (!entry->virtual_url().is_valid())
+        return false;
+      if (entry->virtual_url().SchemeIsHTTPOrHTTPS())
+        http_count++;
+    }
+    return http_count > 0;
+  }
+
+  void AppendEntry(std::unique_ptr<sessions::SerializedNavigationEntry> 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_.set_id(window_id);
+  }
+
+  void SetSessionId(SessionID::id_type id) { tab_id_.set_id(id); }
+
+  void set_blocked_navigations(
+      const std::vector<std::unique_ptr<sessions::SerializedNavigationEntry>>&
+          navs) {
+    for (auto& entry : navs) {
+      blocked_navigations_.push_back(
+          base::MakeUnique<sessions::SerializedNavigationEntry>(*entry));
+    }
   }
-  ~SyncedWindowDelegateOverride() override {}
 
-  bool HasWindow() const override { return wrapped_->HasWindow(); }
+  void reset() {
+    current_entry_index_ = 0;
+    sync_id_ = TabNodePool::kInvalidTabNodeID;
+    entries_.clear();
+  }
+
+ private:
+  int current_entry_index_ = -1;
+  bool is_supervised_ = false;
+  int sync_id_ = -1;
+  SessionID tab_id_;
+  SessionID window_id_;
+  std::vector<std::unique_ptr<const sessions::SerializedNavigationEntry>>
+      blocked_navigations_;
+  std::vector<std::unique_ptr<const sessions::SerializedNavigationEntry>>
+      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; }
 
-  SessionID::id_type GetSessionId() const override {
-    return session_id_override_ >= 0 ? session_id_override_
-                                     : wrapped_->GetSessionId();
+  // 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_;
+};
+
+class TestSyncedWindowDelegate : public SyncedWindowDelegate {
+ public:
+  TestSyncedWindowDelegate() {}
+  ~TestSyncedWindowDelegate() override {}
+
+  bool HasWindow() const override { return true; }
 
-  int GetTabCount() const override { return wrapped_->GetTabCount(); }
+  SessionID::id_type GetSessionId() const override { return window_id_.id(); }
 
-  int GetActiveIndex() const override { return wrapped_->GetActiveIndex(); }
+  int GetTabCount() const override { return tab_delegates_.size(); }
 
-  bool IsApp() const override { return wrapped_->IsApp(); }
+  int GetActiveIndex() const override { return 0; }
 
-  bool IsTypeTabbed() const override { return wrapped_->IsTypeTabbed(); }
+  bool IsApp() const override { return false; }
 
-  bool IsTypePopup() const override { return wrapped_->IsTypePopup(); }
+  bool IsTypeTabbed() const override {
+    return window_type_ == sync_pb::SessionWindow_BrowserType_TYPE_TABBED;
+  }
+
+  bool IsTypePopup() const override {
+    return window_type_ == sync_pb::SessionWindow_BrowserType_TYPE_POPUP;
+  }
 
   bool IsTabPinned(const SyncedTabDelegate* tab) const override {
-    return wrapped_->IsTabPinned(tab);
+    return false;
   }
 
   SyncedTabDelegate* GetTabAt(int index) const override {
-    if (tab_overrides_.find(index) != tab_overrides_.end())
-      return tab_overrides_.find(index)->second;
+    if (tab_delegates_.find(index) != tab_delegates_.end())
+      return tab_delegates_.find(index)->second;
 
-    return wrapped_->GetTabAt(index);
+    return nullptr;
   }
 
-  void OverrideSessionId(SessionID::id_type id) { session_id_override_ = id; }
+  void OverrideWindowId(SessionID::id_type id) { window_id_.set_id(id); }
 
-  void OverrideTabAt(int index,
-                     SyncedTabDelegate* delegate,
-                     SessionID::id_type tab_id) {
-    tab_overrides_[index] = delegate;
-    tab_id_overrides_[index] = tab_id;
+  void OverrideTabAt(int index, SyncedTabDelegate* delegate) {
+    tab_delegates_[index] = delegate;
   }
 
   SessionID::id_type GetTabIdAt(int index) const override {
-    if (tab_id_overrides_.find(index) != tab_id_overrides_.end())
-      return tab_id_overrides_.find(index)->second;
-    return wrapped_->GetTabIdAt(index);
+    SyncedTabDelegate* delegate = GetTabAt(index);
+    if (!delegate)
+      return TabNodePool::kInvalidTabID;
+    return delegate->GetSessionId();
   }
 
-  bool IsSessionRestoreInProgress() const override {
-    return wrapped_->IsSessionRestoreInProgress();
-  }
+  bool IsSessionRestoreInProgress() const override { return false; }
 
-  bool ShouldSync() const override { return wrapped_->ShouldSync(); }
+  bool ShouldSync() const override { return true; }
 
  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;
+  std::map<int, SyncedTabDelegate*> tab_delegates_;
+  SessionID window_id_;
+  sync_pb::SessionWindow_BrowserType window_type_ =
+      sync_pb::SessionWindow_BrowserType_TYPE_TABBED;
 };
 
 class TestSyncedWindowDelegatesGetter : public SyncedWindowDelegatesGetter {
  public:
-  TestSyncedWindowDelegatesGetter(
-      const std::set<const SyncedWindowDelegate*>& delegates)
-      : delegates_(delegates) {}
+  TestSyncedWindowDelegatesGetter() {}
+  ~TestSyncedWindowDelegatesGetter() override {}
 
   std::set<const SyncedWindowDelegate*> GetSyncedWindowDelegates() override {
     return delegates_;
@@ -149,17 +412,20 @@ class TestSyncedWindowDelegatesGetter : public SyncedWindowDelegatesGetter {
     return nullptr;
   }
 
+  void AddSyncedWindowDelegate(const SyncedWindowDelegate* delegate) {
+    delegates_.insert(delegate);
+  }
+
  private:
-  const std::set<const SyncedWindowDelegate*> delegates_;
+  std::set<const SyncedWindowDelegate*> delegates_;
 };
 
-class TestSyncProcessorStub : public syncer::SyncChangeProcessor {
+class TestSyncChangeProcessor : public syncer::SyncChangeProcessor {
  public:
-  explicit TestSyncProcessorStub(syncer::SyncChangeList* output)
-      : output_(output) {}
+  explicit TestSyncChangeProcessor(SyncChangeList* output) : output_(output) {}
   syncer::SyncError ProcessSyncChanges(
       const tracked_objects::Location& from_here,
-      const syncer::SyncChangeList& change_list) override {
+      const SyncChangeList& change_list) override {
     if (error_.IsSet()) {
       syncer::SyncError error = error_;
       error_ = syncer::SyncError();
@@ -173,7 +439,7 @@ class TestSyncProcessorStub : public syncer::SyncChangeProcessor {
     return syncer::SyncError();
   }
 
-  syncer::SyncDataList GetAllSyncData(syncer::ModelType type) const override {
+  SyncDataList GetAllSyncData(syncer::ModelType type) const override {
     return sync_data_to_return_;
   }
 
@@ -186,7 +452,7 @@ class TestSyncProcessorStub : public syncer::SyncChangeProcessor {
   }
 
   void NotifyLocalChangeObservers() {
-    const syncer::SyncChange empty_change;
+    const SyncChange empty_change;
     for (syncer::LocalChangeObserver& observer : local_change_observers_)
       observer.OnLocalChange(nullptr, empty_change);
   }
@@ -195,118 +461,58 @@ class TestSyncProcessorStub : public syncer::SyncChangeProcessor {
     error_ = error;
   }
 
-  void SetSyncDataToReturn(const syncer::SyncDataList& data) {
+  void SetSyncDataToReturn(const SyncDataList& data) {
     sync_data_to_return_ = data;
   }
 
  private:
   syncer::SyncError error_;
-  syncer::SyncChangeList* output_;
-  syncer::SyncDataList sync_data_to_return_;
+  SyncChangeList* output_;
+  SyncDataList sync_data_to_return_;
   base::ObserverList<syncer::LocalChangeObserver> local_change_observers_;
 };
 
-void ExpectAllOfChangesType(const syncer::SyncChangeList& changes,
-                            const syncer::SyncChange::SyncChangeType type) {
-  for (const syncer::SyncChange& change : changes) {
-    EXPECT_EQ(type, change.change_type());
-  }
-}
-
-int CountIfTagMatches(const syncer::SyncChangeList& changes,
-                      const std::string& tag) {
-  return std::count_if(
-      changes.begin(), changes.end(), [&tag](const syncer::SyncChange& change) {
-        return change.sync_data().GetSpecifics().session().session_tag() == tag;
-      });
-}
-
-int CountIfTagMatches(const std::vector<const SyncedSession*>& sessions,
-                      const std::string& tag) {
-  return std::count_if(sessions.begin(), sessions.end(),
-                       [&tag](const SyncedSession* session) {
-                         return session->session_tag == tag;
-                       });
-}
-
-// Creates a field trial with the specified |trial_name| and |group_name| and
-// registers an associated |variation_id| for it for the given |service|.
-void CreateAndActivateFieldTrial(const std::string& trial_name,
-                                 const std::string& group_name,
-                                 variations::VariationID variation_id,
-                                 variations::IDCollectionKey service) {
-  base::FieldTrialList::CreateFieldTrial(trial_name, group_name);
-  variations::AssociateGoogleVariationID(service, trial_name, group_name,
-                                         variation_id);
-  // Access the trial to activate it.
-  base::FieldTrialList::FindFullName(trial_name);
-}
-
 class DummyRouter : public LocalSessionEventRouter {
  public:
   ~DummyRouter() override {}
-  void StartRoutingTo(LocalSessionEventHandler* handler) override {}
+  void StartRoutingTo(LocalSessionEventHandler* handler) override {
+    handler_ = handler;
+  }
   void Stop() override {}
-};
 
-std::unique_ptr<LocalSessionEventRouter> NewDummyRouter() {
-  return std::unique_ptr<LocalSessionEventRouter>(new DummyRouter());
-}
+  void NotifyNav(SyncedTabDelegate* tab) {
+    if (handler_)
+      handler_->OnLocalTabModified(tab);
+  }
+
+ private:
+  LocalSessionEventHandler* handler_ = nullptr;
+};
 
 // Provides ability to override SyncedWindowDelegatesGetter.
-// All other calls are passed through to the original SyncSessionsClient.
-class SyncSessionsClientShim : public SyncSessionsClient {
+// All other calls are passed through to the original FakeSyncSessionsClient.
+class SyncSessionsClientShim : public FakeSyncSessionsClient {
  public:
-  explicit SyncSessionsClientShim(SyncSessionsClient* sync_sessions_client)
-      : sync_sessions_client_(sync_sessions_client),
-        synced_window_getter_(nullptr) {}
+  explicit SyncSessionsClientShim(
+      SyncedWindowDelegatesGetter* synced_window_getter)
+      : synced_window_getter_(synced_window_getter) {}
   ~SyncSessionsClientShim() override {}
 
-  bookmarks::BookmarkModel* GetBookmarkModel() override {
-    return sync_sessions_client_->GetBookmarkModel();
-  }
-
-  favicon::FaviconService* GetFaviconService() override {
-    return sync_sessions_client_->GetFaviconService();
-  }
-
-  history::HistoryService* GetHistoryService() override {
-    return sync_sessions_client_->GetHistoryService();
-  }
-
-  bool ShouldSyncURL(const GURL& url) const override {
-    return sync_sessions_client_->ShouldSyncURL(url);
-  }
-
   SyncedWindowDelegatesGetter* GetSyncedWindowDelegatesGetter() override {
     // The idea here is to allow the test code override the default
     // SyncedWindowDelegatesGetter provided by |sync_sessions_client_|.
     // If |synced_window_getter_| is explicitly set, return it; otherwise return
     // the default one provided by |sync_sessions_client_|.
-    return synced_window_getter_
-               ? synced_window_getter_
-               : sync_sessions_client_->GetSyncedWindowDelegatesGetter();
-  }
-
-  std::unique_ptr<LocalSessionEventRouter> GetLocalSessionEventRouter()
-      override {
-    return sync_sessions_client_->GetLocalSessionEventRouter();
-  }
-
-  void set_synced_window_getter(
-      SyncedWindowDelegatesGetter* synced_window_getter) {
-    synced_window_getter_ = synced_window_getter;
+    return synced_window_getter_;
   }
 
  private:
-  SyncSessionsClient* const sync_sessions_client_;
   SyncedWindowDelegatesGetter* synced_window_getter_;
 };
 
 }  // namespace
 
-class SessionsSyncManagerTest
-    : public BrowserWithTestWindowTest {
+class SessionsSyncManagerTest : public testing::Test {
  protected:
   SessionsSyncManagerTest() : test_processor_(nullptr) {
     local_device_ = base::MakeUnique<LocalDeviceInfoProviderMock>(
@@ -315,18 +521,14 @@ class SessionsSyncManagerTest
   }
 
   void SetUp() override {
-    BrowserWithTestWindowTest::SetUp();
-    sync_client_ = base::MakeUnique<browser_sync::ChromeSyncClient>(profile());
-    sessions_client_shim_ = base::MakeUnique<SyncSessionsClientShim>(
-        sync_client_->GetSyncSessionsClient());
-    NotificationServiceSessionsRouter* router(
-        new NotificationServiceSessionsRouter(
-            profile(), GetSyncSessionsClient(),
-            syncer::SyncableService::StartSyncFlare()));
-    sync_prefs_ = base::MakeUnique<syncer::SyncPrefs>(profile()->GetPrefs());
+    sync_client_ = base::MakeUnique<syncer::FakeSyncClient>();
+    sessions_client_shim_ =
+        base::MakeUnique<SyncSessionsClientShim>(&window_getter_);
+    sync_prefs_ =
+        base::MakeUnique<syncer::SyncPrefs>(sync_client_->GetPrefService());
     manager_ = base::MakeUnique<SessionsSyncManager>(
-        GetSyncSessionsClient(), sync_prefs_.get(), local_device_.get(),
-        std::unique_ptr<LocalSessionEventRouter>(router),
+        sessions_client_shim(), sync_prefs_.get(), local_device_.get(),
+        std::unique_ptr<LocalSessionEventRouter>(NewDummyRouter()),
         base::Bind(&SessionNotificationObserver::NotifyOfUpdate,
                    base::Unretained(&observer_)),
         base::Bind(&SessionNotificationObserver::NotifyOfRefresh,
@@ -338,7 +540,6 @@ class SessionsSyncManagerTest
     helper()->Reset();
     sync_prefs_.reset();
     manager_.reset();
-    BrowserWithTestWindowTest::TearDown();
   }
 
   const DeviceInfo* GetLocalDeviceInfo() {
@@ -349,10 +550,21 @@ class SessionsSyncManagerTest
   SessionSyncTestHelper* helper() { return &helper_; }
   LocalDeviceInfoProvider* local_device() { return local_device_.get(); }
   SessionNotificationObserver* observer() { return &observer_; }
+  syncer::SyncPrefs* sync_prefs() { return sync_prefs_.get(); }
+  SyncSessionsClient* sessions_client_shim() {
+    return sessions_client_shim_.get();
+  }
+  SyncedWindowDelegatesGetter* window_getter() { return &window_getter_; }
+
+  std::unique_ptr<LocalSessionEventRouter> NewDummyRouter() {
+    std::unique_ptr<DummyRouter> router(new DummyRouter());
+    router_ = router.get();
+    return router;
+  }
 
-  void InitWithSyncDataTakeOutput(const syncer::SyncDataList& initial_data,
-                                  syncer::SyncChangeList* output) {
-    test_processor_ = new TestSyncProcessorStub(output);
+  void InitWithSyncDataTakeOutput(const SyncDataList& initial_data,
+                                  SyncChangeList* output) {
+    test_processor_ = new TestSyncChangeProcessor(output);
     syncer::SyncMergeResult result = manager_->MergeDataAndStartSyncing(
         syncer::SESSIONS, initial_data,
         std::unique_ptr<syncer::SyncChangeProcessor>(test_processor_),
@@ -362,26 +574,64 @@ class SessionsSyncManagerTest
   }
 
   void InitWithNoSyncData() {
-    InitWithSyncDataTakeOutput(syncer::SyncDataList(), nullptr);
+    InitWithSyncDataTakeOutput(SyncDataList(), nullptr);
   }
 
   void TriggerProcessSyncChangesError() {
-    test_processor_->FailProcessSyncChangesWith(syncer::SyncError(
-        FROM_HERE, syncer::SyncError::DATATYPE_ERROR, "Error",
-        syncer::SESSIONS));
-  }
-
-  void SetSyncData(const syncer::SyncDataList& data) {
-     test_processor_->SetSyncDataToReturn(data);
+    test_processor_->FailProcessSyncChangesWith(
+        syncer::SyncError(FROM_HERE, syncer::SyncError::DATATYPE_ERROR, "Error",

[skym, 2017/02/22 22:25:11]

using syncer::SyncError?

[Nicolas Zea, 2017/02/22 22:58:40]

Done.

+                          syncer::SESSIONS));
+  }
+
+  void SetSyncData(const SyncDataList& data) {
+    test_processor_->SetSyncDataToReturn(data);
+  }
+
+  void VerifyLocalHeaderChange(const SyncChange& change,
+                               int num_windows,
+                               int num_tabs) {
+    SCOPED_TRACE(change.ToString());
+    SyncDataLocal data(change.sync_data());
+    ASSERT_EQ(manager()->current_machine_tag(), data.GetTag());

[skym, 2017/02/22 22:25:18]

Lot of asserts, should some of these be expects?

[Nicolas Zea, 2017/02/22 22:58:40]

Done.

+    ASSERT_TRUE(data.GetSpecifics().session().has_header());
+    ASSERT_FALSE(data.GetSpecifics().session().has_tab());
+    ASSERT_TRUE(data.GetSpecifics().session().header().has_device_type());
+    ASSERT_EQ(GetLocalDeviceInfo()->client_name(),
+              data.GetSpecifics().session().header().client_name());
+    ASSERT_EQ(num_windows,
+              data.GetSpecifics().session().header().window_size());
+    int tab_count = 0;
+    for (auto& window : data.GetSpecifics().session().header().window()) {
+      tab_count += window.tab_size();
+    }
+    ASSERT_EQ(num_tabs, tab_count);
   }
 
-  syncer::SyncChangeList* FilterOutLocalHeaderChanges(
-      syncer::SyncChangeList* list) {
-    syncer::SyncChangeList::iterator it = list->begin();
+  void VerifyLocalTabChange(const SyncChange& change,
+                            int num_navigations,
+                            std::string final_url) {
+    SCOPED_TRACE(change.ToString());
+    SyncDataLocal data(change.sync_data());
+    ASSERT_TRUE(base::StartsWith(data.GetTag(),
+                                 manager()->current_machine_tag(),
+                                 base::CompareCase::SENSITIVE));
+    ASSERT_FALSE(data.GetSpecifics().session().has_header());
+    ASSERT_TRUE(data.GetSpecifics().session().has_tab());
+    ASSERT_EQ(num_navigations,
+              data.GetSpecifics().session().tab().navigation_size());
+    ASSERT_EQ(final_url, data.GetSpecifics()
+                             .session()
+                             .tab()
+                             .navigation(num_navigations - 1)
+                             .virtual_url());
+  }
+
+  SyncChangeList* FilterOutLocalHeaderChanges(SyncChangeList* list) {
+    SyncChangeList::iterator it = list->begin();
     bool found = false;
     while (it != list->end()) {
       if (it->sync_data().IsLocal() &&
-          syncer::SyncDataLocal(it->sync_data()).GetTag() ==
+          SyncDataLocal(it->sync_data()).GetTag() ==
               manager_->current_machine_tag()) {
         EXPECT_TRUE(SyncChange::ACTION_ADD == it->change_type() ||
                     SyncChange::ACTION_UPDATE == it->change_type());
@@ -395,58 +645,42 @@ class SessionsSyncManagerTest
     return list;
   }
 
-  SyncSessionsClient* GetSyncSessionsClient() {
-    return sessions_client_shim_.get();
-  }
-
-  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);
-  }
-
-  syncer::SyncChange MakeRemoteChange(
-      const sync_pb::SessionSpecifics& specifics,
-      SyncChange::SyncChangeType type) const {
-    return syncer::SyncChange(FROM_HERE, type, CreateRemoteData(specifics));
+  SyncChange MakeRemoteChange(const sync_pb::SessionSpecifics& specifics,
+                              SyncChange::SyncChangeType type) const {
+    return SyncChange(FROM_HERE, type, CreateRemoteData(specifics));
   }
 
   void AddTabsToChangeList(const std::vector<sync_pb::SessionSpecifics>& batch,
                            SyncChange::SyncChangeType type,
-                           syncer::SyncChangeList* change_list) const {
+                           SyncChangeList* change_list) const {
     for (const auto& specifics : batch) {
       change_list->push_back(
-          syncer::SyncChange(FROM_HERE, type, CreateRemoteData(specifics)));
+          SyncChange(FROM_HERE, type, CreateRemoteData(specifics)));
     }
   }
 
   void AddToSyncDataList(const sync_pb::SessionSpecifics& specifics,
-                         syncer::SyncDataList* list,
+                         SyncDataList* list,
                          base::Time mtime) const {
     list->push_back(CreateRemoteData(specifics, mtime));
   }
 
   void AddTabsToSyncDataList(const std::vector<sync_pb::SessionSpecifics>& tabs,
-                             syncer::SyncDataList* list) const {
-    for (size_t i = 0; i < tabs.size(); i++) {
+                             SyncDataList* list) const {
+    for (size_t i = 0; i < tabs.size(); ++i) {
       AddToSyncDataList(tabs[i], list, base::Time::FromInternalValue(i + 1));
     }
   }
 
-  syncer::SyncData CreateRemoteData(const sync_pb::SessionSpecifics& specifics,
-                                    base::Time mtime = base::Time()) const {
+  SyncData CreateRemoteData(const sync_pb::SessionSpecifics& specifics,
+                            base::Time mtime = base::Time()) const {
     sync_pb::EntitySpecifics entity;
     entity.mutable_session()->CopyFrom(specifics);
     return CreateRemoteData(entity, mtime);
   }
 
-  syncer::SyncData CreateRemoteData(const sync_pb::EntitySpecifics& entity,
-                                    base::Time mtime = base::Time()) const {
+  SyncData CreateRemoteData(const sync_pb::EntitySpecifics& entity,
+                            base::Time mtime = base::Time()) const {
     // The server ID is never relevant to these tests, so just use 1.
     return SyncData::CreateRemoteData(
         1, entity, mtime, syncer::AttachmentIdList(),
@@ -454,14 +688,72 @@ class SessionsSyncManagerTest
         SessionsSyncManager::TagHashFromSpecifics(entity.session()));
   }
 
+  TestSyncedWindowDelegate* GetWindowAtIndex(size_t index) {
+    if (index >= windows_.size())
+      return nullptr;
+    return windows_[index].get();
+  }
+
+  // Creates a new tab within the window specified by |window_id|, and points it
+  // at |url|. Returns the newly created TestSyncedTabDelegate (not owned).
+  TestSyncedTabDelegate* AddTab(SessionID::id_type window_id,
+                                const std::string& url,
+                                base::Time time) {
+    tabs_.push_back(base::MakeUnique<TestSyncedTabDelegate>());
+    for (auto& window : windows_) {
+      if (window->GetSessionId() == window_id) {
+        int tab_index = window->GetTabCount();
+        window->OverrideTabAt(tab_index, tabs_.back().get());
+      }
+    }
+
+    // Simulate the browser firing a tab parented notification, ahead of the
+    // actual navigation.
+    router_->NotifyNav(tabs_.back().get());
+
+    // Now do the actual navigation.
+    NavigateTab(tabs_.back().get(), url, time);
+    return tabs_.back().get();
+  }
+  TestSyncedTabDelegate* AddTab(SessionID::id_type window_id,
+                                const std::string& url) {
+    return AddTab(window_id, url, base::Time());
+  }
+
+  void NavigateTab(TestSyncedTabDelegate* delegate,
+                   const std::string& url,
+                   base::Time time) {
+    std::unique_ptr<sessions::SerializedNavigationEntry> entry(

[skym, 2017/02/22 22:25:13]

What do you think of

auto entry = base::MakeUnique<..>(..);

[Nicolas Zea, 2017/02/22 22:58:40]

Done.

+        new sessions::SerializedNavigationEntry(
+            SerializedNavigationEntryTestHelper::CreateNavigation(url,
+                                                                  kTitle)));
+    SerializedNavigationEntryTestHelper::SetTimestamp(time, entry.get());
+    delegate->AppendEntry(std::move(entry));
+    delegate->set_current_entry_index(delegate->GetCurrentEntryIndex() + 1);
+    router_->NotifyNav(delegate);
+  }
+  void NavigateTab(TestSyncedTabDelegate* delegate, const std::string& url) {
+    NavigateTab(delegate, url, base::Time());
+  }
+
+  TestSyncedWindowDelegate* AddWindow() {
+    windows_.push_back(base::MakeUnique<TestSyncedWindowDelegate>());
+    window_getter_.AddSyncedWindowDelegate(windows_.back().get());
+    return windows_.back().get();
+  }
+
  private:
-  std::unique_ptr<browser_sync::ChromeSyncClient> sync_client_;
+  std::unique_ptr<syncer::FakeSyncClient> sync_client_;
   std::unique_ptr<SyncSessionsClientShim> sessions_client_shim_;
   std::unique_ptr<syncer::SyncPrefs> sync_prefs_;
   SessionNotificationObserver observer_;
+  DummyRouter* router_;
   std::unique_ptr<SessionsSyncManager> manager_;
   SessionSyncTestHelper helper_;
-  TestSyncProcessorStub* test_processor_;
+  TestSyncChangeProcessor* test_processor_;
+  TestSyncedWindowDelegatesGetter window_getter_;
+  std::vector<std::unique_ptr<TestSyncedWindowDelegate>> windows_;
+  std::vector<std::unique_ptr<TestSyncedTabDelegate>> tabs_;
   std::unique_ptr<LocalDeviceInfoProviderMock> local_device_;
 };
 
@@ -473,8 +765,8 @@ TEST_F(SessionsSyncManagerTest, PopulateSessionHeader) {
 
   SyncedSession session;
   base::Time time = base::Time::Now();
-  SessionsSyncManager::PopulateSessionHeaderFromSpecifics(
-      header_s, time, &session);
+  SessionsSyncManager::PopulateSessionHeaderFromSpecifics(header_s, time,
+                                                          &session);
   ASSERT_EQ("Client 1", session.session_name);
   ASSERT_EQ(SyncedSession::TYPE_WIN, session.device_type);
   ASSERT_EQ(time, session.modified_time);
@@ -487,160 +779,26 @@ TEST_F(SessionsSyncManagerTest, PopulateSessionWindow) {
   window_s.set_browser_type(sync_pb::SessionWindow_BrowserType_TYPE_TABBED);
   window_s.set_selected_tab_index(1);
 
-  std::string tag = "tag";
-  SyncedSession* session = manager()->session_tracker_.GetSession(tag);
-  manager()->session_tracker_.PutWindowInSession(tag, 0);
-  manager()->BuildSyncedSessionFromSpecifics(tag, window_s, base::Time(),
+  SyncedSession* session = manager()->session_tracker_.GetSession(kTag1);
+  manager()->session_tracker_.PutWindowInSession(kTag1, 0);
+  manager()->BuildSyncedSessionFromSpecifics(kTag1, window_s, base::Time(),
                                              session->windows[0].get());
   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")));
+  ASSERT_EQ(1U, manager()->session_tracker_.num_synced_tabs(kTag1));
 }
 
-namespace {
-
-class SyncedTabDelegateFake : public SyncedTabDelegate {
- public:
-  SyncedTabDelegateFake()
-      : current_entry_index_(0), is_supervised_(false), sync_id_(-1) {}
-  ~SyncedTabDelegateFake() override {}
-
-  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();
-  }
-
-  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_;
-  }
-  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;
-    entries_.clear();
-  }
-
- private:
-  int current_entry_index_;
-  bool is_supervised_;
-  int sync_id_;
-  std::vector<std::unique_ptr<const sessions::SerializedNavigationEntry>>
-      blocked_navigations_;
-  std::vector<std::unique_ptr<content::NavigationEntry>> entries_;
-};
-
-}  // 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);
-static const base::Time kTime8 = base::Time::FromInternalValue(180);
-static const base::Time kTime9 = base::Time::FromInternalValue(190);
-
-// Populate the mock tab delegate with some data and navigation
+// Populate the fake tab delegate with some data and navigation
 // entries and make sure that setting a SessionTab from it preserves
 // those entries (and clobbers any existing data).
 TEST_F(SessionsSyncManagerTest, SetSessionTabFromDelegate) {
   // Create a tab with three valid entries.
-  SyncedTabDelegateFake tab;
-  std::unique_ptr<content::NavigationEntry> entry1(
-      content::NavigationEntry::Create());
-  GURL url1("http://www.google.com/");
-  entry1->SetVirtualURL(url1);
-  entry1->SetTimestamp(kTime1);
-  entry1->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry2(
-      content::NavigationEntry::Create());
-  GURL url2("http://www.noodle.com/");
-  entry2->SetVirtualURL(url2);
-  entry2->SetTimestamp(kTime2);
-  entry2->SetHttpStatusCode(201);
-  std::unique_ptr<content::NavigationEntry> entry3(
-      content::NavigationEntry::Create());
-  GURL url3("http://www.doodle.com/");
-  entry3->SetVirtualURL(url3);
-  entry3->SetTimestamp(kTime3);
-  entry3->SetHttpStatusCode(202);
-
-  tab.AppendEntry(std::move(entry1));
-  tab.AppendEntry(std::move(entry2));
-  tab.AppendEntry(std::move(entry3));
-  tab.set_current_entry_index(2);
+  TestSyncedTabDelegate* tab =
+      AddTab(AddWindow()->GetSessionId(), kFoo1, kTime1);
+  NavigateTab(tab, kBar1, kTime2);
+  NavigateTab(tab, kBaz1, kTime3);
 
   sessions::SessionTab session_tab;
   session_tab.window_id.set_id(1);
@@ -655,26 +813,26 @@ TEST_F(SessionsSyncManagerTest, SetSessionTabFromDelegate) {
       SerializedNavigationEntryTestHelper::CreateNavigation(
           "http://www.example.com", "Example"));
   session_tab.session_storage_persistent_id = "persistent id";
-  manager()->SetSessionTabFromDelegate(tab, kTime4, &session_tab);
+  manager()->SetSessionTabFromDelegate(*tab, kTime4, &session_tab);
 
-  EXPECT_EQ(0, session_tab.window_id.id());
-  EXPECT_EQ(0, session_tab.tab_id.id());
+  EXPECT_EQ(tab->GetWindowId(), session_tab.window_id.id());
+  EXPECT_EQ(tab->GetSessionId(), session_tab.tab_id.id());
   EXPECT_EQ(0, session_tab.tab_visual_index);
-  EXPECT_EQ(2, session_tab.current_navigation_index);
+  EXPECT_EQ(tab->GetCurrentEntryIndex(), session_tab.current_navigation_index);
   EXPECT_FALSE(session_tab.pinned);
   EXPECT_TRUE(session_tab.extension_app_id.empty());
   EXPECT_TRUE(session_tab.user_agent_override.empty());
   EXPECT_EQ(kTime4, session_tab.timestamp);
   ASSERT_EQ(3u, session_tab.navigations.size());
-  EXPECT_EQ(url1, session_tab.navigations[0].virtual_url());
-  EXPECT_EQ(url2, session_tab.navigations[1].virtual_url());
-  EXPECT_EQ(url3, session_tab.navigations[2].virtual_url());
+  EXPECT_EQ(GURL(kFoo1), session_tab.navigations[0].virtual_url());
+  EXPECT_EQ(GURL(kBar1), session_tab.navigations[1].virtual_url());
+  EXPECT_EQ(GURL(kBaz1), session_tab.navigations[2].virtual_url());
   EXPECT_EQ(kTime1, session_tab.navigations[0].timestamp());
   EXPECT_EQ(kTime2, session_tab.navigations[1].timestamp());
   EXPECT_EQ(kTime3, session_tab.navigations[2].timestamp());
   EXPECT_EQ(200, session_tab.navigations[0].http_status_code());
-  EXPECT_EQ(201, session_tab.navigations[1].http_status_code());
-  EXPECT_EQ(202, session_tab.navigations[2].http_status_code());
+  EXPECT_EQ(200, session_tab.navigations[1].http_status_code());
+  EXPECT_EQ(200, session_tab.navigations[2].http_status_code());
   EXPECT_EQ(SerializedNavigationEntry::STATE_INVALID,
             session_tab.navigations[0].blocked_state());
   EXPECT_EQ(SerializedNavigationEntry::STATE_INVALID,
@@ -687,123 +845,35 @@ TEST_F(SessionsSyncManagerTest, SetSessionTabFromDelegate) {
 // Ensure the current_navigation_index gets set properly when the navigation
 // stack gets trucated to +/- 6 entries.
 TEST_F(SessionsSyncManagerTest, SetSessionTabFromDelegateNavigationIndex) {
-  SyncedTabDelegateFake tab;
-  std::unique_ptr<content::NavigationEntry> entry0(
-      content::NavigationEntry::Create());
-  GURL url0("http://www.google.com/");
-  entry0->SetVirtualURL(url0);
-  entry0->SetTimestamp(kTime0);
-  entry0->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry1(
-      content::NavigationEntry::Create());
-  GURL url1("http://www.zoogle.com/");
-  entry1->SetVirtualURL(url1);
-  entry1->SetTimestamp(kTime1);
-  entry1->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry2(
-      content::NavigationEntry::Create());
-  GURL url2("http://www.noogle.com/");
-  entry2->SetVirtualURL(url2);
-  entry2->SetTimestamp(kTime2);
-  entry2->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry3(
-      content::NavigationEntry::Create());
-  GURL url3("http://www.doogle.com/");
-  entry3->SetVirtualURL(url3);
-  entry3->SetTimestamp(kTime3);
-  entry3->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry4(
-      content::NavigationEntry::Create());
-  GURL url4("http://www.yoogle.com/");
-  entry4->SetVirtualURL(url4);
-  entry4->SetTimestamp(kTime4);
-  entry4->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry5(
-      content::NavigationEntry::Create());
-  GURL url5("http://www.foogle.com/");
-  entry5->SetVirtualURL(url5);
-  entry5->SetTimestamp(kTime5);
-  entry5->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry6(
-      content::NavigationEntry::Create());
-  GURL url6("http://www.boogle.com/");
-  entry6->SetVirtualURL(url6);
-  entry6->SetTimestamp(kTime6);
-  entry6->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry7(
-      content::NavigationEntry::Create());
-  GURL url7("http://www.moogle.com/");
-  entry7->SetVirtualURL(url7);
-  entry7->SetTimestamp(kTime7);
-  entry7->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry8(
-      content::NavigationEntry::Create());
-  GURL url8("http://www.poogle.com/");
-  entry8->SetVirtualURL(url8);
-  entry8->SetTimestamp(kTime8);
-  entry8->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry9(
-      content::NavigationEntry::Create());
-  GURL url9("http://www.roogle.com/");
-  entry9->SetVirtualURL(url9);
-  entry9->SetTimestamp(kTime9);
-  entry9->SetHttpStatusCode(200);
-
-  tab.AppendEntry(std::move(entry0));
-  tab.AppendEntry(std::move(entry1));
-  tab.AppendEntry(std::move(entry2));
-  tab.AppendEntry(std::move(entry3));
-  tab.AppendEntry(std::move(entry4));
-  tab.AppendEntry(std::move(entry5));
-  tab.AppendEntry(std::move(entry6));
-  tab.AppendEntry(std::move(entry7));
-  tab.AppendEntry(std::move(entry8));
-  tab.AppendEntry(std::move(entry9));
-  tab.set_current_entry_index(8);
+  TestSyncedTabDelegate* tab = AddTab(AddWindow()->GetSessionId(), kFoo1);
+  const int kNavs = 10;

[skym, 2017/02/22 22:25:14]

This naming seems odd to me. I always thought the kMixedCase naming was for
static constants. Looking at our style guide
https://google.github.io/styleguide/cppguide.html#Constant_Names it seems the
kMixedCase is optional in this scenario. So free to keep.

[Nicolas Zea, 2017/02/22 22:58:40]

Acknowledged.

+  for (int i = 1; i < kNavs; ++i) {
+    NavigateTab(tab, base::StringPrintf("http://foo%i", i));
+  }
+  tab->set_current_entry_index(kNavs - 2);
 
   sessions::SessionTab session_tab;
-  manager()->SetSessionTabFromDelegate(tab, kTime9, &session_tab);
+  manager()->SetSessionTabFromDelegate(*tab, kTime9, &session_tab);
 
   EXPECT_EQ(6, session_tab.current_navigation_index);
   ASSERT_EQ(8u, session_tab.navigations.size());
-  EXPECT_EQ(url2, session_tab.navigations[0].virtual_url());
-  EXPECT_EQ(url3, session_tab.navigations[1].virtual_url());
-  EXPECT_EQ(url4, session_tab.navigations[2].virtual_url());
+  EXPECT_EQ(GURL("http://foo2"), session_tab.navigations[0].virtual_url());
+  EXPECT_EQ(GURL("http://foo3"), session_tab.navigations[1].virtual_url());
+  EXPECT_EQ(GURL("http://foo4"), session_tab.navigations[2].virtual_url());
 }
 
 // Ensure the current_navigation_index gets set to the end of the navigation
 // stack if the current navigation is invalid.
 TEST_F(SessionsSyncManagerTest, SetSessionTabFromDelegateCurrentInvalid) {
-  SyncedTabDelegateFake tab;
-  std::unique_ptr<content::NavigationEntry> entry0(
-      content::NavigationEntry::Create());
-  entry0->SetVirtualURL(GURL("http://www.google.com"));
-  entry0->SetTimestamp(kTime0);
-  entry0->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry1(
-      content::NavigationEntry::Create());
-  entry1->SetVirtualURL(GURL(""));
-  entry1->SetTimestamp(kTime1);
-  entry1->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry2(
-      content::NavigationEntry::Create());
-  entry2->SetVirtualURL(GURL("http://www.noogle.com"));
-  entry2->SetTimestamp(kTime2);
-  entry2->SetHttpStatusCode(200);
-  std::unique_ptr<content::NavigationEntry> entry3(
-      content::NavigationEntry::Create());
-  entry3->SetVirtualURL(GURL("http://www.doogle.com"));
-  entry3->SetTimestamp(kTime3);
-  entry3->SetHttpStatusCode(200);
-
-  tab.AppendEntry(std::move(entry0));
-  tab.AppendEntry(std::move(entry1));
-  tab.AppendEntry(std::move(entry2));
-  tab.AppendEntry(std::move(entry3));
-  tab.set_current_entry_index(1);
+  TestSyncedTabDelegate* tab =
+      AddTab(AddWindow()->GetSessionId(), kFoo1, kTime0);
+  NavigateTab(tab, std::string(""), kTime1);
+  NavigateTab(tab, kBar1, kTime2);
+  NavigateTab(tab, kBar2, kTime3);
+  tab->set_current_entry_index(1);
 
   sessions::SessionTab session_tab;
-  manager()->SetSessionTabFromDelegate(tab, kTime9, &session_tab);
+  manager()->SetSessionTabFromDelegate(*tab, kTime9, &session_tab);
 
   EXPECT_EQ(2, session_tab.current_navigation_index);
   ASSERT_EQ(3u, session_tab.navigations.size());
@@ -836,30 +906,26 @@ TEST_F(SessionsSyncManagerTest, SetVariationIds) {
 // Tests that for supervised users blocked navigations are recorded and marked
 // as such, while regular navigations are marked as allowed.
 TEST_F(SessionsSyncManagerTest, BlockedNavigations) {
-  SyncedTabDelegateFake tab;
-  std::unique_ptr<content::NavigationEntry> entry1(
-      content::NavigationEntry::Create());
-  GURL url1("http://www.google.com/");
-  entry1->SetVirtualURL(url1);
-  entry1->SetTimestamp(kTime1);
-  tab.AppendEntry(std::move(entry1));
-
-  std::unique_ptr<content::NavigationEntry> entry2(
-      content::NavigationEntry::Create());
+  TestSyncedTabDelegate* tab =
+      AddTab(AddWindow()->GetSessionId(), kFoo1, kTime1);
+
+  std::unique_ptr<sessions::SerializedNavigationEntry> entry2(
+      new sessions::SerializedNavigationEntry());
   GURL url2("http://blocked.com/foo");
-  entry2->SetVirtualURL(url2);
-  entry2->SetTimestamp(kTime2);
-  std::unique_ptr<content::NavigationEntry> entry3(
-      content::NavigationEntry::Create());
-  GURL url3("http://evil.com/");
-  entry3->SetVirtualURL(url3);
-  entry3->SetTimestamp(kTime3);
-  ScopedVector<const content::NavigationEntry> blocked_navigations;
+  SerializedNavigationEntryTestHelper::SetVirtualURL(GURL(url2), entry2.get());
+  SerializedNavigationEntryTestHelper::SetTimestamp(kTime2, entry2.get());
+  std::unique_ptr<sessions::SerializedNavigationEntry> entry3(

[skym, 2017/02/22 22:25:20]

MakeUnique

[Nicolas Zea, 2017/02/22 22:58:40]

Done.

+      new sessions::SerializedNavigationEntry());
+  GURL url3("http://evil.com");
+  SerializedNavigationEntryTestHelper::SetVirtualURL(GURL(url3), entry3.get());
+  SerializedNavigationEntryTestHelper::SetTimestamp(kTime3, entry3.get());
+  std::vector<std::unique_ptr<sessions::SerializedNavigationEntry>>
+      blocked_navigations;
   blocked_navigations.push_back(std::move(entry2));
   blocked_navigations.push_back(std::move(entry3));
 
-  tab.set_is_supervised(true);
-  tab.set_blocked_navigations(&blocked_navigations.get());
+  tab->set_is_supervised(true);
+  tab->set_blocked_navigations(blocked_navigations);
 
   sessions::SessionTab session_tab;
   session_tab.window_id.set_id(1);
@@ -874,10 +940,10 @@ TEST_F(SessionsSyncManagerTest, BlockedNavigations) {
       SerializedNavigationEntryTestHelper::CreateNavigation(
           "http://www.example.com", "Example"));
   session_tab.session_storage_persistent_id = "persistent id";
-  manager()->SetSessionTabFromDelegate(tab, kTime4, &session_tab);
+  manager()->SetSessionTabFromDelegate(*tab, kTime4, &session_tab);
 
-  EXPECT_EQ(0, session_tab.window_id.id());
-  EXPECT_EQ(0, session_tab.tab_id.id());
+  EXPECT_EQ(tab->GetWindowId(), session_tab.window_id.id());
+  EXPECT_EQ(tab->GetSessionId(), session_tab.tab_id.id());
   EXPECT_EQ(0, session_tab.tab_visual_index);
   EXPECT_EQ(0, session_tab.current_navigation_index);
   EXPECT_FALSE(session_tab.pinned);
@@ -885,7 +951,7 @@ TEST_F(SessionsSyncManagerTest, BlockedNavigations) {
   EXPECT_TRUE(session_tab.user_agent_override.empty());
   EXPECT_EQ(kTime4, session_tab.timestamp);
   ASSERT_EQ(3u, session_tab.navigations.size());
-  EXPECT_EQ(url1, session_tab.navigations[0].virtual_url());
+  EXPECT_EQ(GURL(kFoo1), session_tab.navigations[0].virtual_url());
   EXPECT_EQ(url2, session_tab.navigations[1].virtual_url());
   EXPECT_EQ(url3, session_tab.navigations[2].virtual_url());
   EXPECT_EQ(kTime1, session_tab.navigations[0].timestamp());
@@ -903,220 +969,103 @@ TEST_F(SessionsSyncManagerTest, BlockedNavigations) {
 // Tests that the local session header objects is created properly in
 // presence of no other session activity, once and only once.
 TEST_F(SessionsSyncManagerTest, MergeLocalSessionNoTabs) {
-  syncer::SyncChangeList out;
-  InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out);
+  // Add a single window with no tabs.
+  AddWindow();
+
+  SyncChangeList out;
+  InitWithSyncDataTakeOutput(SyncDataList(), &out);
   EXPECT_FALSE(manager()->current_machine_tag().empty());
 
-  EXPECT_EQ(2U, out.size());
-  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());
-
-  EXPECT_TRUE(out[1].IsValid());
-  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[1].change_type());
-  const SyncData data_2(out[1].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(0, header_s2.window_size());
+  // Header creation + update.
+  ASSERT_TRUE(ChangeTypeMatches(
+      out, {SyncChange::ACTION_ADD, SyncChange::ACTION_UPDATE}));
+  EXPECT_EQ(out.size(),
+            CountIfTagMatches(out, manager()->current_machine_tag()));
+  VerifyLocalHeaderChange(out[0], 0, 0);
+  VerifyLocalHeaderChange(out[1], 0, 0);
 
   // Now take that header node and feed it in as input.
-  SyncData d = CreateRemoteData(data.GetSpecifics());
-  syncer::SyncDataList in(&d, &d + 1);
-  out.clear();
-  SessionsSyncManager manager2(GetSyncSessionsClient(), sync_prefs(),
-                               local_device(), NewDummyRouter(),
-                               base::Closure(), base::Closure());
-  syncer::SyncMergeResult result = manager2.MergeDataAndStartSyncing(
-      syncer::SESSIONS, in, std::unique_ptr<syncer::SyncChangeProcessor>(
-                                new TestSyncProcessorStub(&out)),
-      std::unique_ptr<syncer::SyncErrorFactory>(
-          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) {

[Nicolas Zea, 2017/02/22 20:37:49]

This test was moved to the bottom of the file (along with the other restore
related test).

-  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 int kRestoredTabId = 1337;
-  const int kNewTabId = 2468;
-
-  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());
-
-  // 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();
-  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));
+  SyncData d = CreateRemoteData(out[1].sync_data().GetSpecifics());
+  SyncDataList in(&d, &d + 1);
   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.
-  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
-  // one for the tab_id update to t1.  t2 shouldn't need to be updated.
-  ASSERT_EQ(2U, FilterOutLocalHeaderChanges(&out)->size());
-  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[0].change_type());
-  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[1].change_type());
-  EXPECT_EQ(kNewTabId,
-            out[1].sync_data().GetSpecifics().session().tab().tab_id());
-
-  // 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.)
-}
-
-// Ensure model association updates the window ID for tabs whose window's ID has
-// changed.
-TEST_F(SessionsSyncManagerTest, WindowIdUpdatedOnRestore) {
-  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"));
   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();
-
-  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());
-
-  // 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());
+  ASSERT_TRUE(ChangeTypeMatches(out, {SyncChange::ACTION_UPDATE}));
+  EXPECT_TRUE(out[0].sync_data().GetSpecifics().session().has_header());
 }
 
 // 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<SessionID::id_type> tab_list1(std::begin(kTabIds1),

[skym, 2017/02/22 22:25:12]

This happens over and over. Are we not allowed to have const vectors?

[Nicolas Zea, 2017/02/22 22:58:40]

No, static consts must be POD.

+                                            std::end(kTabIds1));
   std::vector<sync_pb::SessionSpecifics> tabs1;
-  sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
-      tag, tab_list1, &tabs1));
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(kTag1, tab_list1, &tabs1));
   // Add a second window.
-  SessionID::id_type n2[] = {7, 15, 18, 20};
-  std::vector<SessionID::id_type> tab_list2(n2, n2 + arraysize(n2));
+  std::vector<SessionID::id_type> tab_list2(std::begin(kTabIds2),
+                                            std::end(kTabIds2));
   helper()->AddWindowSpecifics(1, tab_list2, &meta);
 
   // Set up initial data.
-  syncer::SyncDataList initial_data;
+  SyncDataList initial_data;
   initial_data.push_back(CreateRemoteData(meta));
   AddTabsToSyncDataList(tabs1, &initial_data);
-
-  for (size_t i = 0; i < tab_list2.size(); ++i) {
+  for (auto tab_id : tab_list2) {
     sync_pb::EntitySpecifics entity;
-    helper()->BuildTabSpecifics(tag, 0, tab_list2[i],
-                                entity.mutable_session());
+    helper()->BuildTabSpecifics(kTag1, 0, tab_id, entity.mutable_session());
     initial_data.push_back(CreateRemoteData(entity));
   }
 
-  syncer::SyncChangeList output;
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(initial_data, &output);
   EXPECT_TRUE(FilterOutLocalHeaderChanges(&output)->empty());
 
   std::vector<const SyncedSession*> foreign_sessions;
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
   ASSERT_EQ(1U, foreign_sessions.size());
-  std::vector<std::vector<SessionID::id_type> > session_reference;
+  std::vector<std::vector<SessionID::id_type>> session_reference;
   session_reference.push_back(tab_list1);
   session_reference.push_back(tab_list2);
-  helper()->VerifySyncedSession(tag, session_reference, *(foreign_sessions[0]));
+  helper()->VerifySyncedSession(kTag1, session_reference,
+                                *(foreign_sessions[0]));
+}
+
+// Ensure model association associates the pre-existing tabs.
+TEST_F(SessionsSyncManagerTest, MergeLocalSessionExistingTabs) {
+  SessionID::id_type window_id = AddWindow()->GetSessionId();
+  TestSyncedTabDelegate* tab = AddTab(window_id, kFoo1);
+  NavigateTab(tab, kBar1);  // Adds back entry.
+  NavigateTab(tab, kBaz1);  // Adds back entry.
+  TestSyncedTabDelegate* tab2 = AddTab(window_id, kFoo2);
+  NavigateTab(tab2, kBar2);  // Adds back entry.
+
+  SyncChangeList out;
+  InitWithSyncDataTakeOutput(SyncDataList(), &out);
+  // Header creation, add two tabs (and update them), header update.
+  ASSERT_TRUE(ChangeTypeMatches(
+      out, {SyncChange::ACTION_ADD, SyncChange::ACTION_ADD,
+            SyncChange::ACTION_UPDATE, SyncChange::ACTION_ADD,
+            SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE}));
+  EXPECT_EQ(out.size(),
+            CountIfTagMatches(out, manager()->current_machine_tag()));
+
+  // 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));
+
+  VerifyLocalHeaderChange(out[0], 0, 0);
+  VerifyLocalTabChange(out[2], tab->GetEntryCount(), kBaz1);
+  VerifyLocalTabChange(out[4], tab2->GetEntryCount(), kBar2);
+  VerifyLocalHeaderChange(out[5], 1, 2);
+
+  // Verify tab delegates have Sync ids.
+  std::set<const SyncedWindowDelegate*> window_delegates =
+      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());
 }
 
 // This is a combination of MergeWithInitialForeignSession and
@@ -1124,70 +1073,41 @@ TEST_F(SessionsSyncManagerTest, MergeWithInitialForeignSession) {
 // those tests to ensure the common mixed scenario works.
 TEST_F(SessionsSyncManagerTest, MergeWithLocalAndForeignTabs) {
   // Local.
-  AddTab(browser(), GURL("http://foo1"));
-  NavigateAndCommitActiveTab(GURL("http://foo2"));
+  TestSyncedTabDelegate* tab = AddTab(AddWindow()->GetSessionId(), kFoo1);
+  NavigateTab(tab, kFoo2);
 
   // Foreign.
-  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<SessionID::id_type> tab_list1(std::begin(kTabIds1),
+                                            std::end(kTabIds1));
   std::vector<sync_pb::SessionSpecifics> tabs1;
-  sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
-      tag, tab_list1, &tabs1));
-  syncer::SyncDataList foreign_data;
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(kTag1, tab_list1, &tabs1));
+  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());
-
-  // 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());
-
-  // 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_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());
+  SyncChangeList out;
+  InitWithSyncDataTakeOutput(foreign_data, &out);
+  // Should be one header add, 1 tab add (and update), and one header update.
+
+  ASSERT_TRUE(ChangeTypeMatches(
+      out, {SyncChange::ACTION_ADD, SyncChange::ACTION_ADD,
+            SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE}));
+  EXPECT_EQ(out.size(),
+            CountIfTagMatches(out, manager()->current_machine_tag()));
+
+  // Verify local data.
+  VerifyLocalHeaderChange(out[0], 0, 0);
+  VerifyLocalTabChange(out[2], tab->GetEntryCount(), kFoo2);
+  VerifyLocalHeaderChange(out[3], 1, 1);
 
   // Verify foreign data.
   std::vector<const SyncedSession*> foreign_sessions;
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
-  std::vector<std::vector<SessionID::id_type> > session_reference;
+  std::vector<std::vector<SessionID::id_type>> session_reference;
   session_reference.push_back(tab_list1);
-  helper()->VerifySyncedSession(tag, session_reference, *(foreign_sessions[0]));
+  helper()->VerifySyncedSession(kTag1, session_reference,
+                                *(foreign_sessions[0]));
   // There should be one and only one foreign session. If VerifySyncedSession
   // was successful above this EXPECT call ensures the local session didn't
   // get mistakenly added to foreign tracking (Similar to ExistingTabs test).
@@ -1198,40 +1118,46 @@ TEST_F(SessionsSyncManagerTest, MergeWithLocalAndForeignTabs) {
 // followed by updates flowing from sync and local.
 TEST_F(SessionsSyncManagerTest, UpdatesAfterMixedMerge) {
   // Add local and foreign data.
-  AddTab(browser(), GURL("http://foo1"));
-  NavigateAndCommitActiveTab(GURL("http://foo2"));
+  TestSyncedTabDelegate* tab = AddTab(AddWindow()->GetSessionId(), kFoo1);
+  NavigateTab(tab, kFoo2);
+  AddTab(AddWindow()->GetSessionId(), kBar1);
 
-  std::string tag1 = "tag1";
-  syncer::SyncDataList foreign_data1;
-  std::vector<std::vector<SessionID::id_type> > meta1_reference;
+  SyncDataList foreign_data1;
+  std::vector<std::vector<SessionID::id_type>> meta1_reference;
   sync_pb::SessionSpecifics meta1;
 
-  SessionID::id_type n1[] = {5, 10, 13, 17};
-  std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
+  std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1),
+                                            std::end(kTabIds1));
   meta1_reference.push_back(tab_list1);
   std::vector<sync_pb::SessionSpecifics> tabs1;
-  meta1 = helper()->BuildForeignSession(tag1, tab_list1, &tabs1);
+  meta1 = helper()->BuildForeignSession(kTag1, 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());
+  SyncChangeList out;
+  InitWithSyncDataTakeOutput(foreign_data1, &out);
+
+  // 1 header add, two tab adds, one header update.
+  ASSERT_TRUE(ChangeTypeMatches(
+      out, {SyncChange::ACTION_ADD, SyncChange::ACTION_ADD,
+            SyncChange::ACTION_UPDATE, SyncChange::ACTION_ADD,
+            SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE}));
+  EXPECT_EQ(out.size(),
+            CountIfTagMatches(out, manager()->current_machine_tag()));
+  VerifyLocalHeaderChange(out[5], 2, 2);
 
   // 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));
+  std::vector<SessionID::id_type> tab_list2(std::begin(kTabIds2),
+                                            std::end(kTabIds2));
   meta1_reference.push_back(tab_list2);
   helper()->AddWindowSpecifics(1, tab_list2, &meta1);
   std::vector<sync_pb::SessionSpecifics> tabs2;
   tabs2.resize(tab_list2.size());
   for (size_t i = 0; i < tab_list2.size(); ++i) {
-    helper()->BuildTabSpecifics(tag1, 0, tab_list2[i], &tabs2[i]);
+    helper()->BuildTabSpecifics(kTag1, 0, tab_list2[i], &tabs2[i]);
   }
 
-  syncer::SyncChangeList changes;
+  SyncChangeList changes;
   changes.push_back(MakeRemoteChange(meta1, SyncChange::ACTION_UPDATE));
   AddTabsToChangeList(tabs2, SyncChange::ACTION_ADD, &changes);
   manager()->ProcessSyncChanges(FROM_HERE, changes);
@@ -1243,15 +1169,13 @@ TEST_F(SessionsSyncManagerTest, UpdatesAfterMixedMerge) {
   ASSERT_EQ(1U, foreign_sessions.size());
   ASSERT_EQ(4U, foreign_sessions[0]->windows.find(0)->second->tabs.size());
   ASSERT_EQ(4U, foreign_sessions[0]->windows.find(1)->second->tabs.size());
-  helper()->VerifySyncedSession(tag1, meta1_reference, *(foreign_sessions[0]));
+  helper()->VerifySyncedSession(kTag1, meta1_reference, *(foreign_sessions[0]));
 
   // Add a new foreign session.
-  std::string tag2 = "tag2";
-  SessionID::id_type n2[] = {107, 115};
-  std::vector<SessionID::id_type> tag2_tab_list(n2, n2 + arraysize(n2));
+  std::vector<SessionID::id_type> tag2_tab_list = {107, 115};
   std::vector<sync_pb::SessionSpecifics> tag2_tabs;
-  sync_pb::SessionSpecifics meta2(helper()->BuildForeignSession(
-      tag2, tag2_tab_list, &tag2_tabs));
+  sync_pb::SessionSpecifics meta2(
+      helper()->BuildForeignSession(kTag2, tag2_tab_list, &tag2_tabs));
   changes.push_back(MakeRemoteChange(meta2, SyncChange::ACTION_ADD));
   AddTabsToChangeList(tag2_tabs, SyncChange::ACTION_ADD, &changes);
 
@@ -1259,11 +1183,11 @@ TEST_F(SessionsSyncManagerTest, UpdatesAfterMixedMerge) {
   changes.clear();
 
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
-  std::vector<std::vector<SessionID::id_type> > meta2_reference;
+  std::vector<std::vector<SessionID::id_type>> meta2_reference;
   meta2_reference.push_back(tag2_tab_list);
   ASSERT_EQ(2U, foreign_sessions.size());
   ASSERT_EQ(2U, foreign_sessions[1]->windows.find(0)->second->tabs.size());
-  helper()->VerifySyncedSession(tag2, meta2_reference, *(foreign_sessions[1]));
+  helper()->VerifySyncedSession(kTag2, meta2_reference, *(foreign_sessions[1]));
   foreign_sessions.clear();
 
   // Remove a tab from a window.
@@ -1275,7 +1199,7 @@ TEST_F(SessionsSyncManagerTest, UpdatesAfterMixedMerge) {
        iter != tab_list1.end(); ++iter) {
     win->add_tab(*iter);
   }
-  syncer::SyncChangeList removal;
+  SyncChangeList removal;
   removal.push_back(MakeRemoteChange(meta1, SyncChange::ACTION_UPDATE));
   AddTabsToChangeList(tabs1, SyncChange::ACTION_UPDATE, &removal);
   manager()->ProcessSyncChanges(FROM_HERE, removal);
@@ -1283,28 +1207,27 @@ TEST_F(SessionsSyncManagerTest, UpdatesAfterMixedMerge) {
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
   ASSERT_EQ(2U, foreign_sessions.size());
   ASSERT_EQ(3U, foreign_sessions[0]->windows.find(0)->second->tabs.size());
-  helper()->VerifySyncedSession(tag1, meta1_reference, *(foreign_sessions[0]));
+  helper()->VerifySyncedSession(kTag1, meta1_reference, *(foreign_sessions[0]));
 }
 
 // Tests that this SyncSessionManager knows how to delete foreign sessions
 // if it wants to.
 TEST_F(SessionsSyncManagerTest, DeleteForeignSession) {
   InitWithNoSyncData();
-  std::string tag = "tag1";
-  syncer::SyncChangeList changes;
+  SyncChangeList changes;
 
   std::vector<const SyncedSession*> foreign_sessions;
   ASSERT_FALSE(manager()->GetAllForeignSessions(&foreign_sessions));
-  manager()->DeleteForeignSessionInternal(tag, &changes);
+  manager()->DeleteForeignSessionInternal(kTag1, &changes);
   ASSERT_FALSE(manager()->GetAllForeignSessions(&foreign_sessions));
   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));
+  std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1),
+                                            std::end(kTabIds1));
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(kTag1, tab_list1, &tabs));
 
   // Update associator with the session's meta node, window, and tabs.
   manager()->UpdateTrackerWithForeignSession(meta, base::Time());
@@ -1316,22 +1239,21 @@ TEST_F(SessionsSyncManagerTest, DeleteForeignSession) {
   ASSERT_EQ(1U, foreign_sessions.size());
 
   // Now delete the foreign session.
-  manager()->DeleteForeignSessionInternal(tag, &changes);
+  manager()->DeleteForeignSessionInternal(kTag1, &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));
+  ASSERT_TRUE(AllOfChangesAreType(changes, SyncChange::ACTION_DELETE));
+  std::set<std::string> expected_tags(&kTag1, &kTag1 + 1);
+  for (int i = 0; i < 5; ++i)
+    expected_tags.insert(TabNodeIdToTag(kTag1, i));
 
-  for (int i = 0; i < 5; 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()));
+    EXPECT_EQ(1U, expected_tags.erase(
+                      SyncDataLocal(changes[i].sync_data()).GetTag()));
   }
 }
 
@@ -1342,13 +1264,13 @@ TEST_F(SessionsSyncManagerTest, WriteForeignSessionToNodeTabsFirst) {
 
   // Fill an instance of session specifics with a foreign session's data.
   std::string tag = "tag1";
-  SessionID::id_type nums1[] = {5, 10, 13, 17};
   std::vector<sync_pb::SessionSpecifics> tabs1;
-  std::vector<SessionID::id_type> tab_list1(nums1, nums1 + arraysize(nums1));
-  sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
-      tag, tab_list1, &tabs1));
+  std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1),
+                                            std::end(kTabIds1));
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(tag, tab_list1, &tabs1));
 
-  syncer::SyncChangeList adds;
+  SyncChangeList adds;
   // Add tabs for first window, then the meta node.
   AddTabsToChangeList(tabs1, SyncChange::ACTION_ADD, &adds);
   adds.push_back(MakeRemoteChange(meta, SyncChange::ACTION_ADD));
@@ -1358,7 +1280,7 @@ TEST_F(SessionsSyncManagerTest, WriteForeignSessionToNodeTabsFirst) {
   std::vector<const SyncedSession*> foreign_sessions;
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
   ASSERT_EQ(1U, foreign_sessions.size());
-  std::vector<std::vector<SessionID::id_type> > session_reference;
+  std::vector<std::vector<SessionID::id_type>> session_reference;
   session_reference.push_back(tab_list1);
   helper()->VerifySyncedSession(tag, session_reference, *(foreign_sessions[0]));
 }
@@ -1369,16 +1291,15 @@ TEST_F(SessionsSyncManagerTest, WriteForeignSessionToNodeMissingTabs) {
 
   // Fill an instance of session specifics with a foreign session's data.
   std::string tag = "tag1";
-  SessionID::id_type nums1[] = {5, 10, 13, 17};
   std::vector<sync_pb::SessionSpecifics> tabs1;
-  std::vector<SessionID::id_type> tab_list1(nums1, nums1 + arraysize(nums1));
-  sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
-      tag, tab_list1, &tabs1));
+  std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1),
+                                            std::end(kTabIds1));
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(tag, tab_list1, &tabs1));
   // Add a second window, but this time only create two tab nodes, despite the
   // window expecting four tabs.
-  SessionID::id_type tab_nums2[] = {7, 15, 18, 20};
-  std::vector<SessionID::id_type> tab_list2(
-      tab_nums2, tab_nums2 + arraysize(tab_nums2));
+  std::vector<SessionID::id_type> tab_list2(std::begin(kTabIds2),
+                                            std::end(kTabIds2));
   helper()->AddWindowSpecifics(1, tab_list2, &meta);
   std::vector<sync_pb::SessionSpecifics> tabs2;
   tabs2.resize(2);
@@ -1386,7 +1307,7 @@ TEST_F(SessionsSyncManagerTest, WriteForeignSessionToNodeMissingTabs) {
     helper()->BuildTabSpecifics(tag, 0, tab_list2[i], &tabs2[i]);
   }
 
-  syncer::SyncChangeList changes;
+  SyncChangeList changes;
   changes.push_back(MakeRemoteChange(meta, SyncChange::ACTION_ADD));
   AddTabsToChangeList(tabs1, SyncChange::ACTION_ADD, &changes);
   AddTabsToChangeList(tabs2, SyncChange::ACTION_ADD, &changes);
@@ -1413,7 +1334,7 @@ TEST_F(SessionsSyncManagerTest, WriteForeignSessionToNodeMissingTabs) {
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
   ASSERT_EQ(1U, foreign_sessions.size());
   ASSERT_EQ(1U, foreign_sessions[0]->windows.size());
-  std::vector<std::vector<SessionID::id_type> > session_reference;
+  std::vector<std::vector<SessionID::id_type>> session_reference;
   session_reference.push_back(tab_list1);
   helper()->VerifySyncedSession(tag, session_reference, *(foreign_sessions[0]));
 }
@@ -1421,63 +1342,40 @@ TEST_F(SessionsSyncManagerTest, WriteForeignSessionToNodeMissingTabs) {
 // Tests that the SessionsSyncManager can handle a remote client deleting
 // sync nodes that belong to this local session.
 TEST_F(SessionsSyncManagerTest, ProcessRemoteDeleteOfLocalSession) {
-  syncer::SyncChangeList out;
-  InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out);
+  SessionID::id_type window_id = AddWindow()->GetSessionId();
+  SyncChangeList out;
+  InitWithSyncDataTakeOutput(SyncDataList(), &out);
   ASSERT_EQ(2U, out.size());
-  sync_pb::EntitySpecifics entity(out[0].sync_data().GetSpecifics());
-  SyncData d = CreateRemoteData(entity);
-  SetSyncData(syncer::SyncDataList(&d, &d + 1));
-  out.clear();
 
-  syncer::SyncChangeList changes;
-  changes.push_back(
-      MakeRemoteChange(entity.session(), SyncChange::ACTION_DELETE));
+  SyncChangeList changes;
+  changes.push_back(MakeRemoteChange(
+      out[1].sync_data().GetSpecifics().session(), SyncChange::ACTION_DELETE));
+  out.clear();
   manager()->ProcessSyncChanges(FROM_HERE, changes);
   EXPECT_TRUE(manager()->local_tab_pool_out_of_sync_);
   EXPECT_TRUE(out.empty());  // ChangeProcessor shouldn't see any activity.
 
   // This should trigger repair of the TabNodePool.
-  const GURL foo1("http://foo/1");
-  AddTab(browser(), foo1);
+  AddTab(window_id, kFoo1);
   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());
-
-  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),
-            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());
+  // Rebuilding associations will trigger an initial header add and update,
+  // coupled with the tab creation/update and the header update to reflect the
+  // new tab. In total, that means four changes.
+  ASSERT_TRUE(
+      ChangeTypeMatches(out, {SyncChange::ACTION_ADD, SyncChange::ACTION_UPDATE,
+                              SyncChange::ACTION_ADD, SyncChange::ACTION_UPDATE,
+                              SyncChange::ACTION_UPDATE}));
 
   // Verify the actual content.
-  const sync_pb::SessionHeader& session_header =
-      out[3].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();
-  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());
+  VerifyLocalTabChange(out[3], 1, kFoo1);
+  VerifyLocalHeaderChange(out[4], 1, 1);
 
   // 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();
+  int tab_node_id = out[3].sync_data().GetSpecifics().session().tab_node_id();
+  int tab_id = out[3].sync_data().GetSpecifics().session().tab().tab_id();
   EXPECT_EQ(tab_node_id, tab_map.find(tab_id)->second->tab_node_id());
 }
 
@@ -1485,13 +1383,13 @@ TEST_F(SessionsSyncManagerTest, ProcessRemoteDeleteOfLocalSession) {
 // 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(
-      "tag1", tab_list, &tabs1));
+  std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1),
+                                            std::end(kTabIds1));
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession("tag1", tab_list1, &tabs1));
 
-  syncer::SyncChangeList changes;
+  SyncChangeList changes;
   changes.push_back(MakeRemoteChange(meta, SyncChange::ACTION_ADD));
   AddTabsToChangeList(tabs1, SyncChange::ACTION_ADD, &changes);
   manager()->ProcessSyncChanges(FROM_HERE, changes);
@@ -1509,7 +1407,7 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDelete) {
 }
 
 TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabs) {
-  syncer::SyncDataList foreign_data;
+  SyncDataList foreign_data;
   base::Time stale_mtime = base::Time::Now() - base::TimeDelta::FromDays(15);
   std::string session_tag = "tag1";
 
@@ -1521,9 +1419,7 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabs) {
   // 5 will be deleted after header update, but header will still try to own.
   // 6 starts orphaned and then deleted before header update.
   // 7 starts orphaned and then deleted after header update.
-  SessionID::id_type tab_ids[] = {0, 1, 2, 3, 4, 5};
-  std::vector<SessionID::id_type> tab_list(tab_ids,
-                                           tab_ids + arraysize(tab_ids));
+  std::vector<SessionID::id_type> tab_list = {0, 1, 2, 3, 4, 5};
   std::vector<sync_pb::SessionSpecifics> tabs;
   sync_pb::SessionSpecifics meta(
       helper()->BuildForeignSession(session_tag, tab_list, &tabs));
@@ -1536,7 +1432,8 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabs) {
   helper()->BuildTabSpecifics(session_tag, 0, 7, &orphan7);
   AddToSyncDataList(orphan7, &foreign_data, stale_mtime);
 
-  syncer::SyncChangeList output;
+  AddWindow();
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(foreign_data, &output);
   ASSERT_EQ(2U, output.size());
   output.clear();
@@ -1550,7 +1447,7 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabs) {
     window->add_tab(i);
   }
 
-  syncer::SyncChangeList changes;
+  SyncChangeList changes;
   changes.push_back(MakeRemoteChange(tabs[2], SyncChange::ACTION_DELETE));
   changes.push_back(MakeRemoteChange(tabs[4], SyncChange::ACTION_DELETE));
   changes.push_back(MakeRemoteChange(orphan6, SyncChange::ACTION_DELETE));
@@ -1575,7 +1472,7 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabs) {
 }
 
 TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithShadowing) {
-  syncer::SyncDataList foreign_data;
+  SyncDataList foreign_data;
   base::Time stale_mtime = base::Time::Now() - base::TimeDelta::FromDays(16);
   std::string session_tag = "tag1";
 
@@ -1609,7 +1506,8 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithShadowing) {
   helper()->BuildTabSpecifics(session_tag, 0, 2, &tab2C);
   AddToSyncDataList(tab2C, &foreign_data, stale_mtime);
 
-  syncer::SyncChangeList output;
+  AddWindow();
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(foreign_data, &output);
   ASSERT_EQ(2U, output.size());
   output.clear();
@@ -1631,7 +1529,7 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithShadowing) {
   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;
+  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));
@@ -1649,7 +1547,7 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithShadowing) {
 }
 
 TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithReusedNodeIds) {
-  syncer::SyncDataList foreign_data;
+  SyncDataList foreign_data;
   base::Time stale_mtime = base::Time::Now() - base::TimeDelta::FromDays(16);
   std::string session_tag = "tag1";
   int tab_node_id_shared = 13;
@@ -1670,7 +1568,8 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithReusedNodeIds) {
   AddToSyncDataList(tab2A, &foreign_data,
                     stale_mtime + base::TimeDelta::FromMinutes(1));
 
-  syncer::SyncChangeList output;
+  AddWindow();
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(foreign_data, &output);
   ASSERT_EQ(2U, output.size());
   output.clear();
@@ -1681,7 +1580,7 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithReusedNodeIds) {
   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;
+  SyncChangeList changes;
   changes.push_back(MakeRemoteChange(tab1A, SyncChange::ACTION_DELETE));
   manager()->ProcessSyncChanges(FROM_HERE, changes);
 
@@ -1694,59 +1593,67 @@ TEST_F(SessionsSyncManagerTest, ProcessForeignDeleteTabsWithReusedNodeIds) {
   EXPECT_EQ(1U, output.size());
 }
 
-// TODO(shashishekhar): "Move this to TabNodePool unittests."
-TEST_F(SessionsSyncManagerTest, SaveUnassociatedNodesForReassociation) {
-  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);
+TEST_F(SessionsSyncManagerTest, AssociationReusesNodes) {
+  SyncChangeList changes;
+  AddTab(AddWindow()->GetSessionId(), kFoo1);
+  InitWithSyncDataTakeOutput(SyncDataList(), &changes);
+  ASSERT_TRUE(ChangeTypeMatches(
+      changes, {SyncChange::ACTION_ADD, SyncChange::ACTION_ADD,
+                SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE}));
+  ASSERT_TRUE(changes[2].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).
+  SyncDataList in;
+  in.push_back(
+      CreateRemoteData(changes[3].sync_data().GetSpecifics()));  // Header node.
+  sync_pb::SessionSpecifics new_tab(
+      changes[2].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[2].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(
-      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());
+
+  // Reassociate (with the same single tab/window open).
+  manager()->StopSyncing(syncer::SESSIONS);
+  InitWithSyncDataTakeOutput(in, &changes);
+
+  // No tab entities should be deleted. The original (lower) tab node id should
+  // be reused for association.
+  FilterOutLocalHeaderChanges(&changes);
+  ASSERT_TRUE(ChangeTypeMatches(changes, {SyncChange::ACTION_UPDATE}));
+  VerifyLocalTabChange(changes[0], 1, kFoo1);
+  EXPECT_EQ(tab_node_id,
+            changes[0].sync_data().GetSpecifics().session().tab_node_id());
 }
 
-TEST_F(SessionsSyncManagerTest, MergeDeletesCorruptNode) {
-  syncer::SyncChangeList changes;
+// Ensure that the merge process deletes a tab node without a tab id.
+TEST_F(SessionsSyncManagerTest, MergeDeletesTabMissingTabId) {
+  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());
-  syncer::SyncDataList in(&d, &d + 1);
-  changes.clear();
-  TearDown();
-  SetUp();
+  int tab_node_id = 0;
+  sync_pb::SessionSpecifics specifics;
+  specifics.set_session_tag(local_tag);
+  specifics.set_tab_node_id(tab_node_id);
+  SyncDataList in = {CreateRemoteData(specifics)};

[skym, 2017/02/22 22:25:11]

Might be able to inline this without causing a line wrap.

[Nicolas Zea, 2017/02/22 22:58:40]

Done.

+  manager()->StopSyncing(syncer::SESSIONS);
   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),
-            syncer::SyncDataLocal(changes[0].sync_data()).GetTag());
+  EXPECT_EQ(TabNodeIdToTag(local_tag, tab_node_id),
+            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;
+  SyncDataList foreign_data;
   std::vector<SessionID::id_type> empty_ids;
   std::vector<sync_pb::SessionSpecifics> empty_tabs;
   sync_pb::EntitySpecifics entity;
@@ -1777,73 +1684,50 @@ TEST_F(SessionsSyncManagerTest, MergeDeletesBadHash) {
       1, entity, base::Time(), syncer::AttachmentIdList(),
       syncer::AttachmentServiceProxyForTest::Create(), "bad_tab_tag_hash"));
 
-  syncer::SyncChangeList output;
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(foreign_data, &output);
   ASSERT_EQ(2U, FilterOutLocalHeaderChanges(&output)->size());
-  ExpectAllOfChangesType(output, SyncChange::ACTION_DELETE);
-  EXPECT_EQ(1, CountIfTagMatches(output, bad_header_tag));
-  EXPECT_EQ(1, CountIfTagMatches(output, bad_tab_tag));
+  ASSERT_TRUE(AllOfChangesAreType(output, SyncChange::ACTION_DELETE));
+  EXPECT_EQ(1U, CountIfTagMatches(output, bad_header_tag));
+  EXPECT_EQ(1U, CountIfTagMatches(output, bad_tab_tag));
 
   std::vector<const SyncedSession*> sessions;
   manager()->session_tracker_.LookupAllForeignSessions(
       &sessions, SyncedSessionTracker::RAW);
   ASSERT_EQ(2U, sessions.size());
-  EXPECT_EQ(1, CountIfTagMatches(sessions, good_header_tag));
-  EXPECT_EQ(1, CountIfTagMatches(sessions, good_tag_tab));
+  EXPECT_EQ(1U, CountIfTagMatches(sessions, good_header_tag));
+  EXPECT_EQ(1U, CountIfTagMatches(sessions, good_tag_tab));
 }
 
 // Test that things work if a tab is initially ignored.
 TEST_F(SessionsSyncManagerTest, AssociateWindowsDontReloadTabs) {
-  syncer::SyncChangeList out;
+  SyncChangeList out;
   // Go to a URL that is ignored by session syncing.
-  AddTab(browser(), GURL("chrome://preferences/"));
-  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());
+  TestSyncedTabDelegate* tab =
+      AddTab(AddWindow()->GetSessionId(), "chrome://preferences/");
+  InitWithSyncDataTakeOutput(SyncDataList(), &out);
+  ASSERT_TRUE(ChangeTypeMatches(
+      out, {SyncChange::ACTION_ADD, SyncChange::ACTION_UPDATE}));
+  VerifyLocalHeaderChange(out[1], 0, 0);
   out.clear();
 
   // Go to a sync-interesting URL.
-  NavigateAndCommitActiveTab(GURL("http://foo2"));
-
-  EXPECT_EQ(3U, out.size());  // Tab add, update, 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_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());
-  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());
+  NavigateTab(tab, kFoo1);
+
+  // The tab should be created/updated, coupled with a header update.
+  ASSERT_TRUE(
+      ChangeTypeMatches(out, {SyncChange::ACTION_ADD, SyncChange::ACTION_UPDATE,
+                              SyncChange::ACTION_UPDATE}));
+  VerifyLocalTabChange(out[1], 2, kFoo1);
+  VerifyLocalHeaderChange(out[2], 1, 1);
 }
 
 // Tests that the SyncSessionManager responds to local tab events properly.
 TEST_F(SessionsSyncManagerTest, OnLocalTabModified) {
-  syncer::SyncChangeList out;
+  SyncChangeList out;
   // Init with no local data, relies on MergeLocalSessionNoTabs.
-  InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out);
+  SessionID::id_type window_id = AddWindow()->GetSessionId();
+  InitWithSyncDataTakeOutput(SyncDataList(), &out);
   ASSERT_FALSE(manager()->current_machine_tag().empty());
   ASSERT_EQ(2U, out.size());
 
@@ -1851,236 +1735,75 @@ TEST_F(SessionsSyncManagerTest, OnLocalTabModified) {
   sync_pb::EntitySpecifics header(out[0].sync_data().GetSpecifics());
   out.clear();
 
-  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 update for each NavigateAndCommit.
-  // = 6 total tab updates.
-  // One header update corresponding to each of those.
-  // = 6 total header updates.
-  // 12 total updates.
-  ASSERT_EQ(12U, out.size());
+  NavigateTab(AddTab(window_id, kFoo1), kFoo2);
+  NavigateTab(AddTab(window_id, kBar1), kBar2);
+  std::vector<std::string> urls = {kFoo1, kFoo2, kBar1, kBar2};
+
+  // Change type breakdown:
+  // 1 tab add/update + 2 header updates.
+  const size_t kChangesPerTabCreation = 4;
+  // 1 tab update + 1 header update.
+  const size_t kChangesPerTabNav = 2;
+  const size_t kChangesPerTab = kChangesPerTabNav + kChangesPerTabCreation;
+  const size_t kNumTabs = 2;
+  const size_t kTotalUpdates = kChangesPerTab * kNumTabs;
+
+  std::vector<SyncChange::SyncChangeType> types = {
+      // Tab 1
+      SyncChange::ACTION_UPDATE, SyncChange::ACTION_ADD,
+      SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE,
+      SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE,
+      // Tab 2
+      SyncChange::ACTION_UPDATE, SyncChange::ACTION_ADD,
+      SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE,
+      SyncChange::ACTION_UPDATE, SyncChange::ACTION_UPDATE};
+  ASSERT_EQ(kTotalUpdates, types.size());
 
   // Verify the tab node creations and updates to ensure the SyncProcessor
   // sees the right operations.
-  for (int i = 0; i < 12; i++) {
-    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) {
-      // 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.
-      EXPECT_EQ(SyncChange::ACTION_ADD, out[i].change_type());
-      EXPECT_EQ(TabNodePool::TabIdToTag(
-                    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) {
-      // 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) {
-      // 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()),
-                syncer::SyncDataLocal(data).GetTag());
-      ASSERT_TRUE(specifics.has_tab());
-    } else if (i % 6 == 5) {
-      // 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();
+  ASSERT_TRUE(ChangeTypeMatches(out, types));
+  for (size_t i = 0; i < kNumTabs; ++i) {

[skym, 2017/02/22 22:25:11]

I find all of this really confusing and difficult to follow.

[Nicolas Zea, 2017/02/22 22:58:41]

I added some comments to try to clarify what this is verifying.

+    int index = kChangesPerTab * i;

[skym, 2017/02/22 22:25:11]

This really confuses me. So |out| is presumably of size 12, right? And we're
about to go through two iterations of the above for loop. The first time
through, |index| starts at 0. The second time, |index| starts at 2? Shouldn't it
start at 6?

[Nicolas Zea, 2017/02/22 22:58:40]

It does start at 6. kChangesPerTab is 6 (2 + 4)

[skym, 2017/02/22 23:17:26]

Oh, you're right, I kept reading the wrong line.

+    int nav_per_tab_count = 0;
+    {
+      SCOPED_TRACE(index);
+      VerifyLocalHeaderChange(out[index++], (i == 0 ? 0 : 1),

[skym, 2017/02/22 22:25:15]

isn't i == 0 ? 0 : 1 the same as just i when i only goes from 0 to 1?

[Nicolas Zea, 2017/02/22 22:58:40]

True, but that then is based on the assumption that kNumTabs will always be 2.
In this case this should only be 0 on the first iteration, regardless of the
total number of iterations.

+                              i);  // No-op header update for parented tab.

[skym, 2017/02/22 22:25:12]

Is this comment talking about the i param or the entire line? Maybe it would be
cleaner if this comment was above the code line instead of trailing.

[Nicolas Zea, 2017/02/22 22:58:41]

No, the entire line. Moved the comments for this and the others.

     }
-  }
-
-  // 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();
-  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();
-  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();
-  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();
-  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());
-}
-
-// 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());
-  // The last two are the interesting updates.
-  ASSERT_TRUE(out[2].sync_data().GetSpecifics().session().has_tab());
-  EXPECT_EQ(kValidUrl.spec(), out[2]
-                                  .sync_data()
-                                  .GetSpecifics()
-                                  .session()
-                                  .tab()
-                                  .navigation(0)
-                                  .virtual_url());
-  ASSERT_TRUE(out[3].sync_data().GetSpecifics().session().has_header());
-  ASSERT_EQ(1,
-            out[3].sync_data().GetSpecifics().session().header().window_size());
-  ASSERT_EQ(1, out[3]
-                   .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);
-  NavigateAndCommitActiveTab(kInternalUrl);
-  NavigateAndCommitActiveTab(kInternalUrl);
-  NavigateAndCommitActiveTab(kInternalUrl);
-
-  // Reset |out| so we only see the effects of the final navigation.
-  out.clear();
-  NavigateAndCommitActiveTab(kInternalUrl);
-
-  // Only the header node should be updated, and it should no longer have any
-  // valid windows/tabs.
-  ASSERT_EQ(2U, out.size());  // Two header updates (first is a no-op).
-  ASSERT_TRUE(out[1].sync_data().GetSpecifics().session().has_header());
-  EXPECT_EQ(1,
-            out[1].sync_data().GetSpecifics().session().header().window_size());
-}
-
-// 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());
-
-  // 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++) {
-    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());
+    {
+      SCOPED_TRACE(index);
+      nav_per_tab_count++;
+      index++;  // Ignore the tab add, which has no data.

[skym, 2017/02/22 22:25:19]

I think this would make more sense to put between this and the above scopes, or
just in the first scope.

[Nicolas Zea, 2017/02/22 22:58:41]

Done.

+      VerifyLocalTabChange(out[index++], nav_per_tab_count,
+                           urls[i * kChangesPerTabNav + nav_per_tab_count -
+                                1]);  // Tab creation.
+    }
+    {
+      SCOPED_TRACE(index);
+      VerifyLocalHeaderChange(out[index++], 1, i + 1);  // Updated window state.
+    }
+    {
+      SCOPED_TRACE(index);
+      nav_per_tab_count++;
+      VerifyLocalTabChange(out[index++], nav_per_tab_count,
+                           urls[i * kChangesPerTabNav + nav_per_tab_count -
+                                1]);  // Tab navigation.
+    }
+    {
+      SCOPED_TRACE(index);
+      VerifyLocalHeaderChange(out[index++], 1, i + 1);  // Updated window state.
     }
   }
 
-  // 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_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 =
+      window_getter()->GetSyncedWindowDelegates();
+  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;
+  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);
@@ -2121,7 +1844,8 @@ TEST_F(SessionsSyncManagerTest, ForeignSessionModifiedTime) {
     AddToSyncDataList(tabs[1], &foreign_data, newest_time);
   }
 
-  syncer::SyncChangeList output;
+  SyncChangeList output;
+  AddWindow();
   InitWithSyncDataTakeOutput(foreign_data, &output);
   ASSERT_EQ(2U, output.size());
   output.clear();
@@ -2137,29 +1861,28 @@ TEST_F(SessionsSyncManagerTest, ForeignSessionModifiedTime) {
 // Test garbage collection of stale foreign sessions.
 TEST_F(SessionsSyncManagerTest, DoGarbageCollection) {
   // Fill two instances of session specifics with a foreign session's data.
-  std::string tag1 = "tag1";
-  SessionID::id_type n1[] = {5, 10, 13, 17};
-  std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
+  std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1),
+                                            std::end(kTabIds1));
   std::vector<sync_pb::SessionSpecifics> tabs1;
-  sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
-      tag1, tab_list1, &tabs1));
-  std::string tag2 = "tag2";
-  SessionID::id_type n2[] = {8, 15, 18, 20};
-  std::vector<SessionID::id_type> tab_list2(n2, n2 + arraysize(n2));
+  sync_pb::SessionSpecifics meta(

[skym, 2017/02/22 22:25:17]

What do you think of using SessionSpecifics; ?

Also, why is this variable name meta and meta2 below? I don't understand.

[Nicolas Zea, 2017/02/22 22:58:40]

Almost all of the others "using" are for syncer::. Given sync_pb is different,
and implies proto specific semantics, I find it clearer to keep around.
Just due to the old code. Updated to better names. 

+      helper()->BuildForeignSession(kTag1, tab_list1, &tabs1));
+  std::vector<SessionID::id_type> tab_list2(std::begin(kTabIds2),
+                                            std::end(kTabIds2));
   std::vector<sync_pb::SessionSpecifics> tabs2;
-  sync_pb::SessionSpecifics meta2(helper()->BuildForeignSession(
-      tag2, tab_list2, &tabs2));
+  sync_pb::SessionSpecifics meta2(
+      helper()->BuildForeignSession(kTag2, tab_list2, &tabs2));
   // Set the modification time for tag1 to be 21 days ago, tag2 to 5 days ago.
   base::Time tag1_time = base::Time::Now() - base::TimeDelta::FromDays(21);
   base::Time tag2_time = base::Time::Now() - base::TimeDelta::FromDays(5);
 
-  syncer::SyncDataList foreign_data;
+  SyncDataList foreign_data;
   foreign_data.push_back(CreateRemoteData(meta, tag1_time));
   foreign_data.push_back(CreateRemoteData(meta2, tag2_time));
   AddTabsToSyncDataList(tabs1, &foreign_data);
   AddTabsToSyncDataList(tabs2, &foreign_data);
 
-  syncer::SyncChangeList output;
+  AddWindow();
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(foreign_data, &output);
   ASSERT_EQ(2U, output.size());
   output.clear();
@@ -2173,26 +1896,23 @@ TEST_F(SessionsSyncManagerTest, DoGarbageCollection) {
   // 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());
+  ASSERT_TRUE(AllOfChangesAreType(output, SyncChange::ACTION_DELETE));
+  EXPECT_EQ(kTag1, SyncDataLocal(output[0].sync_data()).GetTag());
+  for (int i = 1; i < 5; ++i) {
+    EXPECT_EQ(TabNodeIdToTag(kTag1, i),
+              SyncDataLocal(output[i].sync_data()).GetTag());
   }
 
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
   ASSERT_EQ(1U, foreign_sessions.size());
-  std::vector<std::vector<SessionID::id_type> > session_reference;
+  std::vector<std::vector<SessionID::id_type>> session_reference;
   session_reference.push_back(tab_list2);
-  helper()->VerifySyncedSession(tag2, session_reference,
+  helper()->VerifySyncedSession(kTag2, session_reference,
                                 *(foreign_sessions[0]));
 }
 
 TEST_F(SessionsSyncManagerTest, DoGarbageCollectionOrphans) {
-  syncer::SyncDataList foreign_data;
+  SyncDataList foreign_data;
   base::Time stale_mtime = base::Time::Now() - base::TimeDelta::FromDays(15);
 
   {
@@ -2236,7 +1956,8 @@ TEST_F(SessionsSyncManagerTest, DoGarbageCollectionOrphans) {
     AddToSyncDataList(meta, &foreign_data, stale_mtime);
   }
 
-  syncer::SyncChangeList output;
+  SyncChangeList output;
+  AddWindow();
   InitWithSyncDataTakeOutput(foreign_data, &output);
   ASSERT_EQ(2U, output.size());
   output.clear();
@@ -2251,32 +1972,32 @@ TEST_F(SessionsSyncManagerTest, DoGarbageCollectionOrphans) {
   manager()->DoGarbageCollection();
   // Expect 5 deletions. tag1 header only, tag2 tab only, tag3 header + 2x tabs.
   ASSERT_EQ(5U, output.size());
+  ASSERT_TRUE(AllOfChangesAreType(output, SyncChange::ACTION_DELETE));
 }
 
 // Test that an update to a previously considered "stale" session,
 // prior to garbage collection, will save the session from deletion.
 TEST_F(SessionsSyncManagerTest, GarbageCollectionHonoursUpdate) {
-  std::string tag1 = "tag1";
-  SessionID::id_type n1[] = {5, 10, 13, 17};
-  std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
+  std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1),
+                                            std::end(kTabIds1));
   std::vector<sync_pb::SessionSpecifics> tabs1;
-  sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
-      tag1, tab_list1, &tabs1));
-  syncer::SyncDataList foreign_data;
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(kTag1, tab_list1, &tabs1));
+  SyncDataList foreign_data;
   base::Time tag1_time = base::Time::Now() - base::TimeDelta::FromDays(21);
   foreign_data.push_back(CreateRemoteData(meta, tag1_time));
   AddTabsToSyncDataList(tabs1, &foreign_data);
-  syncer::SyncChangeList output;
+  SyncChangeList output;
+  AddWindow();
   InitWithSyncDataTakeOutput(foreign_data, &output);
   ASSERT_EQ(2U, output.size());
 
   // Update to a non-stale time.
   sync_pb::EntitySpecifics update_entity;
   update_entity.mutable_session()->CopyFrom(tabs1[0]);
-  syncer::SyncChangeList changes;
-  changes.push_back(
-      syncer::SyncChange(FROM_HERE, SyncChange::ACTION_UPDATE,
-                         CreateRemoteData(tabs1[0], base::Time::Now())));
+  SyncChangeList changes;
+  changes.push_back(SyncChange(FROM_HERE, SyncChange::ACTION_UPDATE,
+                               CreateRemoteData(tabs1[0], base::Time::Now())));
   manager()->ProcessSyncChanges(FROM_HERE, changes);
 
   // Check that the foreign session was associated and retrieve the data.
@@ -2289,59 +2010,10 @@ TEST_F(SessionsSyncManagerTest, GarbageCollectionHonoursUpdate) {
   manager()->DoGarbageCollection();
   ASSERT_TRUE(manager()->GetAllForeignSessions(&foreign_sessions));
   ASSERT_EQ(1U, foreign_sessions.size());
-  std::vector<std::vector<SessionID::id_type> > session_reference;
+  std::vector<std::vector<SessionID::id_type>> session_reference;
   session_reference.push_back(tab_list1);
-  helper()->VerifySyncedSession(
-      tag1, session_reference, *(foreign_sessions[0]));
-}
-
-// Test that swapping WebContents for a tab is properly observed and handled
-// by the SessionsSyncManager.
-TEST_F(SessionsSyncManagerTest, CheckPrerenderedWebContentsSwap) {

[Nicolas Zea, 2017/02/22 20:37:49]

This was deleted as the SessionSyncManager no longer has any dependencies
directly on the webcontents, so it's not actually testing any relevant logic.

-  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.
-
-  // 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());
-
-  // Navigate away.
-  NavigateAndCommitActiveTab(GURL("http://bar2"));
-
-  // Delete old WebContents. This should not crash.
-  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.
-  NavigateAndCommitActiveTab(GURL("http://bar3"));
-
-  AddTab(browser(), GURL("http://bar4"));
-  NavigateAndCommitActiveTab(GURL("http://bar5"));
-  ASSERT_EQ(19U, out.size());
+  helper()->VerifySyncedSession(kTag1, session_reference,
+                                *(foreign_sessions[0]));
 }
 
 // Test that NOTIFICATION_FOREIGN_SESSION_UPDATED is sent when processing
@@ -2353,10 +2025,10 @@ TEST_F(SessionsSyncManagerTest, NotifiedOfUpdates) {
   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(
-      "tag1", tab_list, &tabs1));
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession("tag1", tab_list, &tabs1));
 
-  syncer::SyncChangeList changes;
+  SyncChangeList changes;
   changes.push_back(MakeRemoteChange(meta, SyncChange::ACTION_ADD));
   manager()->ProcessSyncChanges(FROM_HERE, changes);
   EXPECT_TRUE(observer()->notified_of_update());
@@ -2382,10 +2054,10 @@ TEST_F(SessionsSyncManagerTest, NotifiedOfLocalRemovalOfForeignSession) {
   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(
-      tag, tab_list, &tabs1));
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(tag, tab_list, &tabs1));
 
-  syncer::SyncChangeList changes;
+  SyncChangeList changes;
   changes.push_back(MakeRemoteChange(meta, SyncChange::ACTION_ADD));
   manager()->ProcessSyncChanges(FROM_HERE, changes);
 
@@ -2395,19 +2067,18 @@ TEST_F(SessionsSyncManagerTest, NotifiedOfLocalRemovalOfForeignSession) {
   ASSERT_TRUE(observer()->notified_of_update());
 }
 
-#if defined(OS_ANDROID)
 // Tests that opening the other devices page triggers a session sync refresh.
 // This page only exists on mobile platforms today; desktop has a
 // search-enhanced NTP without other devices.
 TEST_F(SessionsSyncManagerTest, NotifiedOfRefresh) {
+  SessionID::id_type window_id = AddWindow()->GetSessionId();
   ASSERT_FALSE(observer()->notified_of_refresh());
   InitWithNoSyncData();
-  AddTab(browser(), GURL("http://foo1"));
+  TestSyncedTabDelegate* tab = AddTab(window_id, kFoo1);
   EXPECT_FALSE(observer()->notified_of_refresh());
-  NavigateAndCommitActiveTab(GURL("chrome://newtab/#open_tabs"));
+  NavigateTab(tab, "chrome://newtab/#open_tabs");
   EXPECT_TRUE(observer()->notified_of_refresh());
 }
-#endif  // defined(OS_ANDROID)
 
 // Tests receipt of duplicate tab IDs in the same window.  This should never
 // happen, but we want to make sure the client won't do anything bad if it does
@@ -2416,20 +2087,19 @@ TEST_F(SessionsSyncManagerTest, ReceiveDuplicateTabInSameWindow) {
   std::string tag = "tag1";
 
   // Reuse tab ID 10 in an attempt to trigger bad behavior.
-  SessionID::id_type n1[] = {5, 10, 10, 17};
-  std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
+  std::vector<SessionID::id_type> tab_list1 = {5, 10, 10, 17};
   std::vector<sync_pb::SessionSpecifics> tabs1;
   sync_pb::SessionSpecifics meta(
       helper()->BuildForeignSession(tag, tab_list1, &tabs1));
 
   // Set up initial data.
-  syncer::SyncDataList initial_data;
+  SyncDataList initial_data;
   sync_pb::EntitySpecifics entity;
   entity.mutable_session()->CopyFrom(meta);
   initial_data.push_back(CreateRemoteData(entity));
   AddTabsToSyncDataList(tabs1, &initial_data);
 
-  syncer::SyncChangeList output;
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(initial_data, &output);
 }
 
@@ -2438,21 +2108,17 @@ TEST_F(SessionsSyncManagerTest, ReceiveDuplicateTabInSameWindow) {
 // anything reasonable with this input, but we can expect that it doesn't
 // crash.
 TEST_F(SessionsSyncManagerTest, ReceiveDuplicateTabInOtherWindow) {
-  std::string tag = "tag1";
-
-  SessionID::id_type n1[] = {5, 10, 17};
-  std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
+  std::vector<SessionID::id_type> tab_list1 = {5, 10, 17};
   std::vector<sync_pb::SessionSpecifics> tabs1;
   sync_pb::SessionSpecifics meta(
-      helper()->BuildForeignSession(tag, tab_list1, &tabs1));
+      helper()->BuildForeignSession(kTag1, tab_list1, &tabs1));
 
   // Add a second window.  Tab ID 10 is a duplicate.
-  SessionID::id_type n2[] = {10, 18, 20};
-  std::vector<SessionID::id_type> tab_list2(n2, n2 + arraysize(n2));
+  std::vector<SessionID::id_type> tab_list2 = {10, 18, 20};
   helper()->AddWindowSpecifics(1, tab_list2, &meta);
 
   // Set up initial data.
-  syncer::SyncDataList initial_data;
+  SyncDataList initial_data;
   sync_pb::EntitySpecifics entity;
   entity.mutable_session()->CopyFrom(meta);
   initial_data.push_back(CreateRemoteData(entity));
@@ -2460,32 +2126,30 @@ TEST_F(SessionsSyncManagerTest, ReceiveDuplicateTabInOtherWindow) {
 
   for (size_t i = 0; i < tab_list2.size(); ++i) {
     sync_pb::EntitySpecifics entity;
-    helper()->BuildTabSpecifics(tag, 0, tab_list2[i], entity.mutable_session());
+    helper()->BuildTabSpecifics(kTag1, 0, tab_list2[i],
+                                entity.mutable_session());
     initial_data.push_back(CreateRemoteData(entity));
   }
 
-  syncer::SyncChangeList output;
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(initial_data, &output);
 }
 
 // Tests receipt of multiple unassociated tabs and makes sure that
 // the ones with later timestamp win
 TEST_F(SessionsSyncManagerTest, ReceiveDuplicateUnassociatedTabs) {
-  std::string tag = "tag1";
-
-  SessionID::id_type n1[] = {5, 10, 17};
-  std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
+  std::vector<SessionID::id_type> tab_list1 = {5, 10, 17};
   std::vector<sync_pb::SessionSpecifics> tabs1;
   sync_pb::SessionSpecifics meta(
-      helper()->BuildForeignSession(tag, tab_list1, &tabs1));
+      helper()->BuildForeignSession(kTag1, tab_list1, &tabs1));
 
   // Set up initial data.
-  syncer::SyncDataList initial_data;
+  SyncDataList initial_data;
   initial_data.push_back(CreateRemoteData(meta));
 
   sync_pb::EntitySpecifics entity;
 
-  for (size_t i = 0; i < tabs1.size(); i++) {
+  for (size_t i = 0; i < tabs1.size(); ++i) {
     entity.mutable_session()->CopyFrom(tabs1[i]);
     initial_data.push_back(
         CreateRemoteData(entity, base::Time::FromDoubleT(2000)));
@@ -2496,20 +2160,20 @@ TEST_F(SessionsSyncManagerTest, ReceiveDuplicateUnassociatedTabs) {
   // These two tabs get a different visual indices to distinguish them from the
   // tabs above that get visual index 1 by default.
   sync_pb::SessionSpecifics duplicating_tab1;
-  helper()->BuildTabSpecifics(tag, 0, 10, &duplicating_tab1);
+  helper()->BuildTabSpecifics(kTag1, 0, 10, &duplicating_tab1);
   duplicating_tab1.mutable_tab()->set_tab_visual_index(2);
   entity.mutable_session()->CopyFrom(duplicating_tab1);
   initial_data.push_back(
       CreateRemoteData(entity, base::Time::FromDoubleT(1000)));
 
   sync_pb::SessionSpecifics duplicating_tab2;
-  helper()->BuildTabSpecifics(tag, 0, 17, &duplicating_tab2);
+  helper()->BuildTabSpecifics(kTag1, 0, 17, &duplicating_tab2);
   duplicating_tab2.mutable_tab()->set_tab_visual_index(3);
   entity.mutable_session()->CopyFrom(duplicating_tab2);
   initial_data.push_back(
       CreateRemoteData(entity, base::Time::FromDoubleT(3000)));
 
-  syncer::SyncChangeList output;
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(initial_data, &output);
 
   std::vector<const SyncedSession*> foreign_sessions;
@@ -2528,20 +2192,18 @@ TEST_F(SessionsSyncManagerTest, ReceiveDuplicateUnassociatedTabs) {
 
 // Verify that GetAllForeignSessions returns all sessions sorted by recency.
 TEST_F(SessionsSyncManagerTest, GetAllForeignSessions) {
-  SessionID::id_type ids[] = {5, 10, 13, 17};
-  std::vector<SessionID::id_type> tab_list(ids, ids + arraysize(ids));
+  std::vector<SessionID::id_type> tab_list(std::begin(kTabIds1),
+                                           std::end(kTabIds1));
 
-  const std::string kTag = "tag1";
   std::vector<sync_pb::SessionSpecifics> tabs1;
-  sync_pb::SessionSpecifics meta1(helper()->BuildForeignSession(
-      kTag, tab_list, &tabs1));
+  sync_pb::SessionSpecifics meta1(
+      helper()->BuildForeignSession(kTag1, tab_list, &tabs1));
 
-  const std::string kTag2 = "tag2";
   std::vector<sync_pb::SessionSpecifics> tabs2;
-  sync_pb::SessionSpecifics meta2(helper()->BuildForeignSession(
-      kTag2, tab_list, &tabs2));
+  sync_pb::SessionSpecifics meta2(
+      helper()->BuildForeignSession(kTag2, tab_list, &tabs2));
 
-  syncer::SyncDataList initial_data;
+  SyncDataList initial_data;
   initial_data.push_back(
       CreateRemoteData(meta1, base::Time::FromInternalValue(10)));
   AddTabsToSyncDataList(tabs1, &initial_data);
@@ -2549,7 +2211,7 @@ TEST_F(SessionsSyncManagerTest, GetAllForeignSessions) {
       CreateRemoteData(meta2, base::Time::FromInternalValue(200)));
   AddTabsToSyncDataList(tabs2, &initial_data);
 
-  syncer::SyncChangeList output;
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(initial_data, &output);
 
   std::vector<const SyncedSession*> foreign_sessions;
@@ -2562,20 +2224,18 @@ TEST_F(SessionsSyncManagerTest, GetAllForeignSessions) {
 // Verify that GetForeignSessionTabs returns all tabs for a session sorted
 // by recency.
 TEST_F(SessionsSyncManagerTest, GetForeignSessionTabs) {
-  const std::string kTag = "tag1";
-
-  SessionID::id_type n1[] = {5, 10, 13, 17};
-  std::vector<SessionID::id_type> tab_list1(n1, n1 + arraysize(n1));
+  std::vector<SessionID::id_type> tab_list1(std::begin(kTabIds1),
+                                            std::end(kTabIds1));
   std::vector<sync_pb::SessionSpecifics> tabs1;
-  sync_pb::SessionSpecifics meta(helper()->BuildForeignSession(
-      kTag, tab_list1, &tabs1));
+  sync_pb::SessionSpecifics meta(
+      helper()->BuildForeignSession(kTag1, tab_list1, &tabs1));
   // Add a second window.
-  SessionID::id_type n2[] = {7, 15, 18, 20};
-  std::vector<SessionID::id_type> tab_list2(n2, n2 + arraysize(n2));
+  std::vector<SessionID::id_type> tab_list2(std::begin(kTabIds2),
+                                            std::end(kTabIds2));
   helper()->AddWindowSpecifics(1, tab_list2, &meta);
 
   // Set up initial data.
-  syncer::SyncDataList initial_data;
+  SyncDataList initial_data;
   initial_data.push_back(CreateRemoteData(meta));
 
   // Add the first window's tabs.
@@ -2584,19 +2244,18 @@ TEST_F(SessionsSyncManagerTest, GetForeignSessionTabs) {
   // Add the second window's tabs.
   for (size_t i = 0; i < tab_list2.size(); ++i) {
     sync_pb::EntitySpecifics entity;
-    helper()->BuildTabSpecifics(kTag, 0, tab_list2[i],
+    helper()->BuildTabSpecifics(kTag1, 0, tab_list2[i],
                                 entity.mutable_session());
-    // Order the tabs oldest to most ReceiveDuplicateUnassociatedTabs and
-    // left to right visually.
+    // Order the tabs oldest to most recent and left to right visually.
     initial_data.push_back(
         CreateRemoteData(entity, base::Time::FromInternalValue(i + 1)));
   }
 
-  syncer::SyncChangeList output;
+  SyncChangeList output;
   InitWithSyncDataTakeOutput(initial_data, &output);
 
   std::vector<const sessions::SessionTab*> tabs;
-  ASSERT_TRUE(manager()->GetForeignSessionTabs(kTag, &tabs));
+  ASSERT_TRUE(manager()->GetForeignSessionTabs(kTag1, &tabs));
   // Assert that the size matches the total number of tabs and that the order
   // is from most recent to least.
   ASSERT_EQ(tab_list1.size() + tab_list2.size(), tabs.size());
@@ -2609,4 +2268,96 @@ TEST_F(SessionsSyncManagerTest, GetForeignSessionTabs) {
   }
 }
 
+// Ensure model association associates the pre-existing tabs.
+TEST_F(SessionsSyncManagerTest, SwappedOutOnRestore) {
+  const int kRestoredTabId = 1337;
+  const int kNewTabId = 2468;
+
+  // Start with three tabs in a window.
+  TestSyncedWindowDelegate* window = AddWindow();
+  TestSyncedTabDelegate* tab1 = AddTab(window->GetSessionId(), kFoo1);
+  NavigateTab(tab1, kFoo2);
+  TestSyncedTabDelegate* tab2 = AddTab(window->GetSessionId(), kBar1);
+  NavigateTab(tab2, kBar2);
+  TestSyncedTabDelegate* tab3 = AddTab(window->GetSessionId(), kBaz1);
+  NavigateTab(tab3, kBaz2);
+
+  SyncDataList in;
+  SyncChangeList out;
+  InitWithSyncDataTakeOutput(in, &out);
+
+  // Should be one header add, 3 tab adds/updates, one header update.
+  ASSERT_EQ(8U, out.size());
+
+  // Now update the sync data to be:
+  // * one "normal" fully loaded tab
+  // * 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[2].sync_data().GetSpecifics();
+  sync_pb::EntitySpecifics t1_entity = out[4].sync_data().GetSpecifics();
+  t1_entity.mutable_session()->mutable_tab()->set_tab_id(kRestoredTabId);
+  sync_pb::EntitySpecifics t2_entity = out[6].sync_data().GetSpecifics();
+  in.push_back(CreateRemoteData(t0_entity));
+  in.push_back(CreateRemoteData(t1_entity));
+  in.push_back(CreateRemoteData(t2_entity));
+  out.clear();
+  manager()->StopSyncing(syncer::SESSIONS);
+
+  PlaceholderTabDelegate t1_override(kNewTabId, 1);
+  PlaceholderTabDelegate t2_override(t2_entity.session().tab().tab_id(), 2);
+  window->OverrideTabAt(1, &t1_override);
+  window->OverrideTabAt(2, &t2_override);
+  InitWithSyncDataTakeOutput(in, &out);
+
+  // The last change should be the final header update, reflecting 1 window
+  // and 3 tabs.
+  VerifyLocalHeaderChange(out.back(), 1, 3);
+
+  // There should be three changes, one for the fully associated tab, and
+  // one each for the tab_id updates to t1 and t2.
+  ASSERT_TRUE(AllOfChangesAreType(*FilterOutLocalHeaderChanges(&out),
+                                  SyncChange::ACTION_UPDATE));
+  ASSERT_EQ(3U, out.size());
+  VerifyLocalTabChange(out[0], 2, kFoo2);
+  VerifyLocalTabChange(out[1], 2, kBar2);
+  VerifyLocalTabChange(out[2], 2, kBaz2);
+}
+
+// Ensure model association updates the window ID for tabs whose window's ID has
+// changed.
+TEST_F(SessionsSyncManagerTest, WindowIdUpdatedOnRestore) {
+  const int kNewWindowId = 1337;
+  SyncDataList in;
+  SyncChangeList out;
+
+  // Set up one tab and start sync with it.
+  TestSyncedWindowDelegate* window = AddWindow();
+  AddTab(window->GetSessionId(), kFoo1);
+  InitWithSyncDataTakeOutput(in, &out);
+
+  // Should be one header add, 1 tab add/update, and one header update.
+  ASSERT_EQ(4U, out.size());
+  const sync_pb::EntitySpecifics t0_entity = out[2].sync_data().GetSpecifics();
+  ASSERT_TRUE(t0_entity.session().has_tab());
+
+  in.push_back(CreateRemoteData(t0_entity));
+  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.
+  window->OverrideTabAt(0, &t0_override);
+  window->OverrideWindowId(kNewWindowId);
+  InitWithSyncDataTakeOutput(in, &out);
+
+  // There should be one change for t0's window ID update.
+  ASSERT_EQ(1U, FilterOutLocalHeaderChanges(&out)->size());
+  VerifyLocalTabChange(out[0], 1, kFoo1);
+  EXPECT_EQ(kNewWindowId,
+            out[0].sync_data().GetSpecifics().session().tab().window_id());
+}
+
 }  // namespace sync_sessions