sync: Route local sessions events to SessionsSyncManager

chrome/browser/sync/sessions2/sessions_sync_manager_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync/sessions2/sessions_sync_manager.h"
 
 #include "base/strings/string_util.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/sessions/session_id.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/sync/glue/device_info.h"
 #include "chrome/browser/sync/glue/session_sync_test_helper.h"
 #include "chrome/browser/sync/glue/synced_tab_delegate.h"
+#include "chrome/browser/sync/sessions2/notification_service_sessions_router.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/serialized_navigation_entry_test_helper.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/web_contents.h"
 #include "sync/api/sync_error_factory_mock.h"
@@ skipping 10 matching lines @@
 
 namespace {
 
 class TestSyncProcessorStub : public syncer::SyncChangeProcessor {
  public:
   explicit TestSyncProcessorStub(syncer::SyncChangeList* output)
       : output_(output) {}
   virtual syncer::SyncError ProcessSyncChanges(
       const tracked_objects::Location& from_here,
       const syncer::SyncChangeList& change_list) OVERRIDE {
+    if (error_.IsSet()) {
+      syncer::SyncError error = error_;
+      error_ = syncer::SyncError();
+      return error;
+    }
+
     if (output_)
-      output_->assign(change_list.begin(), change_list.end());
+      output_->insert(output_->end(), change_list.begin(), change_list.end());
+
     return syncer::SyncError();
   }
 
   virtual syncer::SyncDataList GetAllSyncData(syncer::ModelType type)
       const OVERRIDE {
     return syncer::SyncDataList();
   }
+
+  void FailProcessSyncChangesWith(const syncer::SyncError& error) {
+    error_ = error;
+  }
  private:
+  syncer::SyncError error_;
   syncer::SyncChangeList* output_;
 };
 
 syncer::SyncChange MakeRemoteChange(
     int64 id,
     const sync_pb::SessionSpecifics& specifics,
     SyncChange::SyncChangeType type) {
   sync_pb::EntitySpecifics entity;
   entity.mutable_session()->CopyFrom(specifics);
   return syncer::SyncChange(
@@ skipping 20 matching lines @@
 void AddTabsToSyncDataList(const std::vector<sync_pb::SessionSpecifics> tabs,
                            syncer::SyncDataList* list) {
   for (size_t i = 0; i < tabs.size(); i++) {
     sync_pb::EntitySpecifics entity;
     entity.mutable_session()->CopyFrom(tabs[i]);
     list->push_back(SyncData::CreateRemoteData(
         i + 2, entity, base::Time()));
   }
 }
 
+class DummyRouter : public SessionsSyncManager::LocalEventRouter {
+ public:
+  virtual ~DummyRouter() {}
+  virtual void StartRoutingTo(LocalSessionEventHandler* handler) OVERRIDE {}
+  virtual void Stop() OVERRIDE {}
+};
+
+scoped_ptr<SessionsSyncManager::LocalEventRouter> NewDummyRouter() {
+  return scoped_ptr<SessionsSyncManager::LocalEventRouter>(new DummyRouter());
+}
+
 }  // namespace
 
 class SessionsSyncManagerTest
     : public BrowserWithTestWindowTest,

[Nicolas Zea, 2013/11/21 02:34:52]

now that you're abstracting away all the real interactions, do you still need
this (which is what allows for the NavigateAndCommitActiveTab)? It'd be nice to
instead trigger calls directly via the local event router.

[tim (not reviewing), 2013/11/21 21:38:59]

I was thinking the same when creating the LocalEventRouter and I think you're
absolutely correct.  I think we should do that. I decided against it as part of
this patch because 

- There's non-trivial fake code to write to splice in at and use
GetSyncedWindowDelegates
- It affects all tests in here (including the ones that only do
MergeDataAndStartSyncing), slightly more than I was going for with this patch
alone
- I didn't see any other examples of code creating test implementations of
NavigationEntry, which we'd need
- There is a dependency on SyncedTabDelegate::GetWebContents (for
NetworkTimeHelper) and I didn't want to get into the business of trying to
return a fake WebContents because I think that's what BrowserWithTestWindowTest
is for.  

However, I could try to break that dependency (sent a thread offline about that)
and use SyncedTabDelegateFake (and a SyncedWindowDelegate thing I create) in
place of BrowserWithTestWindowTest.  It would be pretty awesome, more targeted,
faster, and likely produce less of that crappy verbose output (and be runnable
without xvfb-run over ssh :p).

I also think it'd be easier to do once SessionModelAssociator is gone and
there's one less place to reconcile, but I get how that may sound like an excuse
:) My next set of patches will be test-focused anyway (integration tests).
Rather than re-write this whole suite as part of this parity-goal CL, why don't
I leave it as is to get coverage in the meantime and see if I can dedicate a
future patch to revamping it?  

[Nicolas Zea, 2013/11/22 23:34:51]

sgtm

       public SessionsSyncManager::SyncInternalApiDelegate {
  public:
-  SessionsSyncManagerTest() {}
+  SessionsSyncManagerTest() : test_processor_(NULL) {}
 
   virtual void SetUp() OVERRIDE {
     BrowserWithTestWindowTest::SetUp();
-    manager_.reset(new SessionsSyncManager(profile(), this));
+    browser_sync::NotificationServiceSessionsRouter* router(
+        new browser_sync::NotificationServiceSessionsRouter(profile()));
+    router->set_flare(syncer::SyncableService::StartSyncFlare());
+    manager_.reset(new SessionsSyncManager(profile(), this,
+      scoped_ptr<SessionsSyncManager::LocalEventRouter>(router)));
   }
 
   virtual void TearDown() OVERRIDE {
+    test_processor_ = NULL;
     helper()->Reset();
     manager_.reset();
     BrowserWithTestWindowTest::TearDown();
   }
 
   virtual scoped_ptr<DeviceInfo> GetLocalDeviceInfo() const OVERRIDE {
     return scoped_ptr<DeviceInfo>(
         new DeviceInfo(GetLocalSyncCacheGUID(),
                        "Wayne Gretzky's Hacking Box",
                        "Chromium 10k",
                        "Chrome 10k",
                        sync_pb::SyncEnums_DeviceType_TYPE_LINUX));
   }
 
   virtual std::string GetLocalSyncCacheGUID() const OVERRIDE {
     return "cache_guid";
   }
 
   SessionsSyncManager* manager() { return manager_.get(); }
   SessionSyncTestHelper* helper() { return &helper_; }
 
   void InitWithSyncDataTakeOutput(const syncer::SyncDataList& initial_data,
                                   syncer::SyncChangeList* output) {
+    test_processor_ = new TestSyncProcessorStub(output);
     syncer::SyncMergeResult result = manager_->MergeDataAndStartSyncing(
         syncer::SESSIONS, initial_data,
         scoped_ptr<syncer::SyncChangeProcessor>(
-            new TestSyncProcessorStub(output)),
+            test_processor_),

[Nicolas Zea, 2013/11/21 02:34:52]

nit: fits on previous line?

[tim (not reviewing), 2013/11/21 21:38:59]

Done.

         scoped_ptr<syncer::SyncErrorFactory>(
             new syncer::SyncErrorFactoryMock()));
     EXPECT_FALSE(result.error().IsSet());
   }
 
   void InitWithNoSyncData() {
     InitWithSyncDataTakeOutput(syncer::SyncDataList(), NULL);
   }
 
+  void TriggerProcessSyncChangesError() {
+    test_processor_->FailProcessSyncChangesWith(syncer::SyncError(
+        FROM_HERE, syncer::SyncError::PERSISTENCE_ERROR, "Error",

[Nicolas Zea, 2013/11/21 02:34:52]

nit: prefer something other than persistence error, which doesn't really apply
to sessions (e.g. maybe DATATYPE_ERROR.

[tim (not reviewing), 2013/11/21 21:38:59]

Ah.  I actually wasn't clear on the difference from the comments in
sync_error.h.  Changed.

+        syncer::SESSIONS));
+  }
+
   syncer::SyncChangeList* FilterOutLocalHeaderChanges(
       syncer::SyncChangeList* list) {
     syncer::SyncChangeList::iterator it = list->begin();
     bool found = false;
     while (it != list->end()) {
       if (it->sync_data().GetTag() == manager_->current_machine_tag()) {
         EXPECT_TRUE(SyncChange::ACTION_ADD == it->change_type() ||
                     SyncChange::ACTION_UPDATE == it->change_type());
         it = list->erase(it);
         found = true;
       } else {
         ++it;
       }
     }
     EXPECT_TRUE(found);
     return list;
   }
 
  private:
   scoped_ptr<SessionsSyncManager> manager_;
   SessionSyncTestHelper helper_;
+  TestSyncProcessorStub* test_processor_;
 };
 
 TEST_F(SessionsSyncManagerTest, PopulateSessionHeader) {
   sync_pb::SessionHeader header_s;
   header_s.set_client_name("Client 1");
   header_s.set_device_type(sync_pb::SyncEnums_DeviceType_TYPE_WIN);
 
   SyncedSession session;
   base::Time time = base::Time::Now();
   SessionsSyncManager::PopulateSessionHeaderFromSpecifics(
@@ skipping 336 matching lines @@
   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());
 
   // Now take that header node and feed it in as input.
   SyncData d(SyncData::CreateRemoteData(1, data.GetSpecifics(), base::Time()));
   syncer::SyncDataList in(&d, &d + 1);
   out.clear();
-  SessionsSyncManager manager2(profile(), this);
+  SessionsSyncManager manager2(profile(), this, NewDummyRouter());
   syncer::SyncMergeResult result = manager2.MergeDataAndStartSyncing(
       syncer::SESSIONS, in,
       scoped_ptr<syncer::SyncChangeProcessor>(
           new TestSyncProcessorStub(&out)),
       scoped_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());
@@ skipping 372 matching lines @@
   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(SyncData::CreateRemoteData(1, entity, base::Time()));
   syncer::SyncDataList in(&d, &d + 1);
   changes.clear();
-  SessionsSyncManager manager2(profile(), this);
+  SessionsSyncManager manager2(profile(), this, NewDummyRouter());
   syncer::SyncMergeResult result = manager2.MergeDataAndStartSyncing(
       syncer::SESSIONS, in,
       scoped_ptr<syncer::SyncChangeProcessor>(
           new TestSyncProcessorStub(&changes)),
       scoped_ptr<syncer::SyncErrorFactory>(
           new syncer::SyncErrorFactoryMock()));
   ASSERT_FALSE(result.error().IsSet());
   EXPECT_TRUE(FilterOutLocalHeaderChanges(&changes)->empty());
 }
 
 TEST_F(SessionsSyncManagerTest, MergeDeletesCorruptNode) {
   syncer::SyncChangeList changes;
   InitWithNoSyncData();
 
   std::string local_tag = manager()->current_machine_tag();
   int tab_node_id = manager()->local_tab_pool_.GetFreeTabNode(&changes);
   SyncData d(SyncData::CreateRemoteData(
       1, changes[0].sync_data().GetSpecifics(), base::Time()));
   syncer::SyncDataList in(&d, &d + 1);
   changes.clear();
   TearDown();
   SetUp();
   InitWithSyncDataTakeOutput(in, &changes);
   EXPECT_EQ(1U, FilterOutLocalHeaderChanges(&changes)->size());
   EXPECT_EQ(SyncChange::ACTION_DELETE, changes[0].change_type());
   EXPECT_EQ(TabNodePool2::TabIdToTag(local_tag, tab_node_id),
             changes[0].sync_data().GetTag());
 }
 
+// Test that things work if a tab is initially ignored.
 TEST_F(SessionsSyncManagerTest, AssociateWindowsDontReloadTabs) {
   syncer::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());
+  ASSERT_EQ(2U, out.size());  // Header add and update.
   EXPECT_EQ(
       0,
       out[1].sync_data().GetSpecifics().session().header().window_size());
   out.clear();
 
   // Go to a sync-interesting URL.
   NavigateAndCommitActiveTab(GURL("http://foo2"));
 
-  // Simulate a selective association (e.g in response to tab event) as
-  // would occur in practice from ProcessSyncChanges.
-  content::WebContents* c =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  manager()->AssociateTab(SyncedTabDelegate::ImplFromWebContents(c), &out);
-  ASSERT_EQ(2U, out.size());
+  EXPECT_EQ(3U, out.size());  // Tab add, update, and header update.
 
   EXPECT_TRUE(StartsWithASCII(out[0].sync_data().GetTag(),
                               manager()->current_machine_tag(), true));
   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(StartsWithASCII(out[1].sync_data().GetTag(),
                               manager()->current_machine_tag(), true));
   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());
 
-  out.clear();
-  manager()->AssociateWindows(SessionsSyncManager::DONT_RELOAD_TABS,
-                              &out);
-
-  EXPECT_EQ(1U, out.size());
-  EXPECT_TRUE(out[0].IsValid());
-  EXPECT_EQ(SyncChange::ACTION_UPDATE, out[0].change_type());
-  const SyncData data(out[0].sync_data());
+  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(), 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());
 }
 
+TEST_F(SessionsSyncManagerTest, OnLocalTabModified) {
+  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());
+
+  // Copy the original header.
+  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());
+
+  // 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(StartsWithASCII(data.GetTag(),
+                                manager()->current_machine_tag(), true));
+    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());
+    } else if (i % 6 == 1) {
+      // Next, the TabNodePool should create the tab node.
+      EXPECT_EQ(SyncChange::ACTION_ADD, out[i].change_type());
+    } else if (i % 6 == 2) {
+      // Then we see the tab update to the URL.
+      EXPECT_EQ(SyncChange::ACTION_UPDATE, out[i].change_type());
+      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());
+      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();
+    }
+  }
+
+  // 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());
+}
+
 // Ensure model association associates the pre-existing tabs.
 TEST_F(SessionsSyncManagerTest, MergeLocalSessionExistingTabs) {
   AddTab(browser(), GURL("http://foo1"));
-  NavigateAndCommitActiveTab(GURL("http://foo2"));
+  NavigateAndCommitActiveTab(GURL("http://foo2"));  // Adds back entry.
   AddTab(browser(), GURL("http://bar1"));
-  NavigateAndCommitActiveTab(GURL("http://bar2"));
+  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.
@@ skipping 58 matching lines @@
 TEST_F(SessionsSyncManagerTest, CheckPrerenderedWebContentsSwap) {
   AddTab(browser(), GURL("http://foo1"));
   NavigateAndCommitActiveTab(GURL("http://foo2"));
 
   syncer::SyncChangeList out;
   InitWithSyncDataTakeOutput(syncer::SyncDataList(), &out);
   ASSERT_EQ(4U, out.size());  // Header, tab ADD, tab UPDATE, header UPDATE.
 
   // To simulate WebContents swap during prerendering, create new WebContents
   // and swap with old WebContents.
-  content::WebContents* old_web_contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
+  scoped_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);
+  int index = browser()->tab_strip_model()->GetIndexOfWebContents(
+      old_web_contents.get());
   browser()->tab_strip_model()->ReplaceWebContentsAt(index, new_web_contents);
 
-  manager()->AssociateWindows(SessionsSyncManager::RELOAD_TABS, &out);
-  ASSERT_EQ(7U, out.size());
+  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"));
-  manager()->AssociateWindows(SessionsSyncManager::RELOAD_TABS, &out);
 
   // Delete old WebContents. This should not crash.
-  delete old_web_contents;
-  manager()->AssociateWindows(SessionsSyncManager::RELOAD_TABS, &out);
+  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"));
-  manager()->AssociateWindows(SessionsSyncManager::RELOAD_TABS, &out);
 
   AddTab(browser(), GURL("http://bar4"));
-  manager()->AssociateWindows(SessionsSyncManager::RELOAD_TABS, &out);
   NavigateAndCommitActiveTab(GURL("http://bar5"));
-  manager()->AssociateWindows(SessionsSyncManager::RELOAD_TABS, &out);
-  ASSERT_EQ(20U, out.size());
+  ASSERT_EQ(19U, out.size());
 }
 
 namespace {
 class SessionNotificationObserver : public content::NotificationObserver {
  public:
-  SessionNotificationObserver() : notified_of_update_(false) {
+  SessionNotificationObserver() : notified_of_update_(false),
+                                  notified_of_refresh_(false) {
     registrar_.Add(this, chrome::NOTIFICATION_FOREIGN_SESSION_UPDATED,
                    content::NotificationService::AllSources());
+    registrar_.Add(this, chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
+                   content::NotificationService::AllSources());
   }
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE {
     switch (type) {
       case chrome::NOTIFICATION_FOREIGN_SESSION_UPDATED:
         notified_of_update_ = true;
         break;
+      case chrome::NOTIFICATION_SYNC_REFRESH_LOCAL:
+        notified_of_refresh_ = true;
+        break;
       default:
         NOTREACHED();
         break;
     }
   }
   bool notified_of_update() const { return notified_of_update_; }
-  void Reset() { notified_of_update_ = false; }
+  bool notified_of_refresh() const { return notified_of_refresh_; }
+  void Reset() {
+    notified_of_update_ = false;
+    notified_of_refresh_ = false;
+  }
  private:
   content::NotificationRegistrar registrar_;
   bool notified_of_update_;
+  bool notified_of_refresh_;
 };
 }  // namespace
 
 TEST_F(SessionsSyncManagerTest, NotifiedOfUpdates) {
   SessionNotificationObserver observer;
+  ASSERT_FALSE(observer.notified_of_update());
   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));
 
   syncer::SyncChangeList changes;
   changes.push_back(MakeRemoteChange(1, meta, SyncChange::ACTION_ADD));
   manager()->ProcessSyncChanges(FROM_HERE, changes);
   EXPECT_TRUE(observer.notified_of_update());
 
   changes.clear();
   observer.Reset();
   AddTabsToChangeList(tabs1, SyncChange::ACTION_ADD, &changes);
   manager()->ProcessSyncChanges(FROM_HERE, changes);
   EXPECT_TRUE(observer.notified_of_update());
 
   changes.clear();
   observer.Reset();
   changes.push_back(MakeRemoteChange(1, meta, SyncChange::ACTION_DELETE));
   manager()->ProcessSyncChanges(FROM_HERE, changes);
   EXPECT_TRUE(observer.notified_of_update());
 }
 
+TEST_F(SessionsSyncManagerTest, NotifiedOfRefresh) {
+  SessionNotificationObserver observer;
+  ASSERT_FALSE(observer.notified_of_refresh());
+  InitWithNoSyncData();
+  AddTab(browser(), GURL("http://foo1"));
+  EXPECT_FALSE(observer.notified_of_refresh());
+  NavigateAndCommitActiveTab(GURL("chrome://newtab/#open_tabs"));
+  EXPECT_TRUE(observer.notified_of_refresh());
+}
+
 }  // namespace browser_sync