Revert 118198 - [Sync] Cleanup Sessions code and make tab syncability stricter.

We now only consider a tab syncable if it has at least one valid entry, where
valid is true iff the url is valid and the scheme is neither chrome or file.
This avoids syncing tabs with nothing but chrome:// or file:// navigations.

BUG=109301
TEST=unit_tests


Review URL: http://codereview.chromium.org/9114015

TBR=zea@chromium.org

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@118212 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 271 insertions, 170 deletions

diff --git a/chrome/browser/sync/glue/session_change_processor.cc b/chrome/browser/sync/glue/session_change_processor.cc
index f08b4d3..168716b 100644
--- a/chrome/browser/sync/glue/session_change_processor.cc
+++ b/chrome/browser/sync/glue/session_change_processor.cc
@@ -4,10 +4,12 @@
 
 #include "chrome/browser/sync/glue/session_change_processor.h"
 
+#include <sstream>
 #include <string>
 #include <vector>
 
 #include "base/logging.h"
+#include "base/memory/scoped_vector.h"
 #include "chrome/browser/extensions/extension_tab_helper.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/api/sync_error.h"
@@ -15,7 +17,6 @@
 #include "chrome/browser/sync/internal_api/change_record.h"
 #include "chrome/browser/sync/internal_api/read_node.h"
 #include "chrome/browser/sync/profile_sync_service.h"
-#include "chrome/browser/sync/protocol/session_specifics.pb.h"
 #include "chrome/browser/ui/sync/tab_contents_wrapper_synced_tab_delegate.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/common/chrome_notification_types.h"
diff --git a/chrome/browser/sync/glue/session_change_processor.h b/chrome/browser/sync/glue/session_change_processor.h
index d6cc011..fbd7576 100644
--- a/chrome/browser/sync/glue/session_change_processor.h
+++ b/chrome/browser/sync/glue/session_change_processor.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -8,6 +8,8 @@
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "chrome/browser/sessions/session_backend.h"
+#include "chrome/browser/sessions/session_service.h"
 #include "chrome/browser/sync/glue/change_processor.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
diff --git a/chrome/browser/sync/glue/session_model_associator.cc b/chrome/browser/sync/glue/session_model_associator.cc
index 1ce4f84..0ecdf53 100644
--- a/chrome/browser/sync/glue/session_model_associator.cc
+++ b/chrome/browser/sync/glue/session_model_associator.cc
@@ -25,7 +25,6 @@
 #include "chrome/browser/sync/internal_api/write_node.h"
 #include "chrome/browser/sync/internal_api/write_transaction.h"
 #include "chrome/browser/sync/profile_sync_service.h"
-#include "chrome/browser/sync/protocol/session_specifics.pb.h"
 #include "chrome/browser/sync/syncable/syncable.h"
 #include "chrome/browser/sync/util/get_session_name_task.h"
 #include "chrome/common/chrome_notification_types.h"
@@ -80,6 +79,7 @@ SessionModelAssociator::SessionModelAssociator(ProfileSyncService* sync_service,
       waiting_for_change_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(test_weak_factory_(this)) {
   DCHECK(CalledOnValidThread());
+  DCHECK(sync_service_);
 }
 
 SessionModelAssociator::~SessionModelAssociator() {
@@ -287,7 +287,7 @@ bool SessionModelAssociator::AssociateTab(const SyncedTabDelegate& tab) {
     return true;
   }
 
-  if (!ShouldSyncTab(tab))
+  if (!IsValidTab(tab))
     return true;
 
   TabLinksMap::const_iterator tablink = tab_map_.find(id);
@@ -1055,47 +1055,27 @@ void SessionModelAssociator::DeleteForeignSession(const std::string& tag) {
   }
 }
 
+// Valid local tab?
 bool SessionModelAssociator::IsValidTab(const SyncedTabDelegate& tab) const {
-  if ((!sync_service_ || tab.profile() != sync_service_->profile()) &&
-      !setup_for_test_) {
-    return false;
-  }
-  const SyncedWindowDelegate* window =
-      SyncedWindowDelegate::FindSyncedWindowDelegateWithId(
-          tab.GetWindowId());
-  if (!window && !setup_for_test_)
-    return false;
-  return true;
-}
-
-bool SessionModelAssociator::TabHasValidEntry(
-    const SyncedTabDelegate& tab) const {
-  int pending_index = tab.GetPendingEntryIndex();
-  int entry_count = tab.GetEntryCount();
-  bool found_valid_url = false;
-  if (entry_count == 0)
-    return false;  // This deliberately ignores a new pending entry.
-  for (int i = 0; i < entry_count; ++i) {
-    const content::NavigationEntry* entry = (i == pending_index) ?
-       tab.GetPendingEntry() : tab.GetEntryAtIndex(i);
+  DCHECK(CalledOnValidThread());
+  if ((tab.profile() == sync_service_->profile() ||
+       sync_service_->profile() == NULL)) {          // For tests.
+    const SyncedWindowDelegate* window =
+        SyncedWindowDelegate::FindSyncedWindowDelegateWithId(
+            tab.GetWindowId());
+    if (!window)
+      return false;
+    const NavigationEntry* entry = tab.GetActiveEntry();
     if (!entry)
       return false;
     if (entry->GetVirtualURL().is_valid() &&
-        !entry->GetVirtualURL().SchemeIs("chrome") &&
-        !entry->GetVirtualURL().SchemeIsFile()) {
-      found_valid_url = true;
+        (entry->GetVirtualURL().GetOrigin() !=
+            GURL(chrome::kChromeUINewTabURL) ||
+         tab.GetEntryCount() > 1)) {
+      return true;
     }
   }
-  return found_valid_url;
-}
-
-// If this functionality changes, SyncedSession::ShouldSyncSessionTab should be
-// modified to match.
-bool SessionModelAssociator::ShouldSyncTab(const SyncedTabDelegate& tab) const {
-  DCHECK(CalledOnValidThread());
-  if (!IsValidTab(tab))
-    return false;
-  return TabHasValidEntry(tab);
+  return false;
 }
 
 void SessionModelAssociator::QuitLoopForSubtleTesting() {
@@ -1119,6 +1099,157 @@ void SessionModelAssociator::BlockUntilLocalChangeForTest(
       base::TimeDelta::FromMilliseconds(timeout_milliseconds));
 }
 
+// ==========================================================================
+// The following methods are not currently used but will likely become useful
+// if we choose to sync the previous browser session.
+
+SessionService* SessionModelAssociator::GetSessionService() {
+  DCHECK(CalledOnValidThread());
+  DCHECK(sync_service_);
+  Profile* profile = sync_service_->profile();
+  DCHECK(profile);
+  SessionService* sessions_service =
+      SessionServiceFactory::GetForProfile(profile);
+  DCHECK(sessions_service);
+  return sessions_service;
+}
+
+void SessionModelAssociator::OnGotSession(
+    int handle,
+    std::vector<SessionWindow*>* windows) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(local_session_syncid_);
+
+  sync_pb::SessionSpecifics specifics;
+  specifics.set_session_tag(GetCurrentMachineTag());
+  sync_pb::SessionHeader* header_s = specifics.mutable_header();
+  PopulateSessionSpecificsHeader(*windows, header_s);
+
+  sync_api::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
+  sync_api::ReadNode root(&trans);
+  if (!root.InitByTagLookup(kSessionsTag)) {
+    LOG(ERROR) << kNoSessionsFolderError;
+    return;
+  }
+
+  sync_api::WriteNode header_node(&trans);
+  if (!header_node.InitByIdLookup(local_session_syncid_)) {
+    LOG(ERROR) << "Failed to load local session header node.";
+    return;
+  }
+
+  header_node.SetSessionSpecifics(specifics);
+}
+
+void SessionModelAssociator::PopulateSessionSpecificsHeader(
+    const std::vector<SessionWindow*>& windows,
+    sync_pb::SessionHeader* header_s) {
+  DCHECK(CalledOnValidThread());
+
+  // Iterate through the vector of windows, extracting the window data, along
+  // with the tab data to populate the session specifics.
+  for (size_t i = 0; i < windows.size(); ++i) {
+    if (SessionWindowHasNoTabsToSync(*(windows[i])))
+      continue;
+    sync_pb::SessionWindow* window_s = header_s->add_window();
+    PopulateSessionSpecificsWindow(*(windows[i]), window_s);
+    if (!SyncLocalWindowToSyncModel(*(windows[i])))
+      return;
+  }
+}
+
+// Called when populating session specifics to send to the sync model, called
+// when associating models, or updating the sync model.
+void SessionModelAssociator::PopulateSessionSpecificsWindow(
+    const SessionWindow& window,
+    sync_pb::SessionWindow* session_window) {
+  DCHECK(CalledOnValidThread());
+  session_window->set_window_id(window.window_id.id());
+  session_window->set_selected_tab_index(window.selected_tab_index);
+  if (window.type == Browser::TYPE_TABBED) {
+    session_window->set_browser_type(
+      sync_pb::SessionWindow_BrowserType_TYPE_TABBED);
+  } else if (window.type == Browser::TYPE_POPUP) {
+    session_window->set_browser_type(
+      sync_pb::SessionWindow_BrowserType_TYPE_POPUP);
+  } else {
+    // ignore
+    LOG(WARNING) << "Session Sync unable to handle windows of type" <<
+        window.type;
+    return;
+  }
+  for (std::vector<SessionTab*>::const_iterator i = window.tabs.begin();
+      i != window.tabs.end(); ++i) {
+    const SessionTab* tab = *i;
+    if (!IsValidSessionTab(*tab))
+      continue;
+    session_window->add_tab(tab->tab_id.id());
+  }
+}
+
+bool SessionModelAssociator::SyncLocalWindowToSyncModel(
+    const SessionWindow& window) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(tab_map_.empty());
+  for (size_t i = 0; i < window.tabs.size(); ++i) {
+    SessionTab* tab = window.tabs[i];
+    int64 id = tab_pool_.GetFreeTabNode();
+    if (id == sync_api::kInvalidId) {
+      LOG(ERROR) << "Failed to find/generate free sync node for tab.";
+      return false;
+    }
+
+    sync_api::WriteTransaction trans(FROM_HERE, sync_service_->GetUserShare());
+    if (!WriteSessionTabToSyncModel(*tab, id, &trans)) {
+      return false;
+    }
+
+    TabLinks t(id, tab);
+    tab_map_[tab->tab_id.id()] = t;
+  }
+  return true;
+}
+
+bool SessionModelAssociator::WriteSessionTabToSyncModel(
+    const SessionTab& tab,
+    const int64 sync_id,
+    sync_api::WriteTransaction* trans) {
+  DCHECK(CalledOnValidThread());
+  sync_api::WriteNode tab_node(trans);
+  if (!tab_node.InitByIdLookup(sync_id)) {
+    LOG(ERROR) << "Failed to look up tab node " << sync_id;
+    return false;
+  }
+
+  sync_pb::SessionSpecifics specifics;
+  specifics.set_session_tag(GetCurrentMachineTag());
+  sync_pb::SessionTab* tab_s = specifics.mutable_tab();
+  PopulateSessionSpecificsTab(tab, tab_s);
+  tab_node.SetSessionSpecifics(specifics);
+  return true;
+}
+
+// See PopulateSessionSpecificsWindow for use.
+void SessionModelAssociator::PopulateSessionSpecificsTab(
+    const SessionTab& tab,
+    sync_pb::SessionTab* session_tab) {
+  DCHECK(CalledOnValidThread());
+  session_tab->set_tab_id(tab.tab_id.id());
+  session_tab->set_window_id(tab.window_id.id());
+  session_tab->set_tab_visual_index(tab.tab_visual_index);
+  session_tab->set_current_navigation_index(
+      tab.current_navigation_index);
+  session_tab->set_pinned(tab.pinned);
+  session_tab->set_extension_app_id(tab.extension_app_id);
+  for (std::vector<TabNavigation>::const_iterator i =
+      tab.navigations.begin(); i != tab.navigations.end(); ++i) {
+    const TabNavigation navigation = *i;
+    sync_pb::TabNavigation* tab_navigation =
+        session_tab->add_navigation();
+    PopulateSessionSpecificsNavigation(&navigation, tab_navigation);
+  }
+}
+
 bool SessionModelAssociator::CryptoReadyIfNecessary() {
   // We only access the cryptographer while holding a transaction.
   sync_api::ReadTransaction trans(FROM_HERE, sync_service_->GetUserShare());
diff --git a/chrome/browser/sync/glue/session_model_associator.h b/chrome/browser/sync/glue/session_model_associator.h
index 76b2345..bfaab71 100644
--- a/chrome/browser/sync/glue/session_model_associator.h
+++ b/chrome/browser/sync/glue/session_model_associator.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -14,7 +14,9 @@
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/gtest_prod_util.h"
+#include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "base/stringprintf.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time.h"
@@ -26,7 +28,9 @@
 #include "chrome/browser/sync/glue/synced_session_tracker.h"
 #include "chrome/browser/sync/glue/synced_tab_delegate.h"
 #include "chrome/browser/sync/glue/synced_window_delegate.h"
+#include "chrome/browser/sync/protocol/session_specifics.pb.h"
 #include "chrome/browser/sync/syncable/model_type.h"
+#include "chrome/browser/sync/util/weak_handle.h"
 
 class Profile;
 class ProfileSyncService;
@@ -38,11 +42,7 @@ class WriteTransaction;
 }  // namespace sync_api
 
 namespace sync_pb {
-class SessionHeader;
 class SessionSpecifics;
-class SessionTab;
-class SessionWindow;
-class TabNavigation;
 }  // namespace sync_pb
 
 namespace browser_sync {
@@ -188,9 +188,9 @@ class SessionModelAssociator
   void DeleteForeignSession(const std::string& tag);
 
   // Control which local tabs we're interested in syncing.
-  // Ensures the profile matches sync's profile and that the tab has valid
-  // entries.
-  bool ShouldSyncTab(const SyncedTabDelegate& tab) const;
+  // Ensures the profile matches sync's profile and that the tab has at least
+  // one navigation entry and is not an empty tab.
+  bool IsValidTab(const SyncedTabDelegate& tab) const;
 
   // Returns the syncable model type.
   static syncable::ModelType model_type() { return syncable::SESSIONS; }
@@ -211,8 +211,6 @@ class SessionModelAssociator
                            WriteForeignSessionToNode);
   FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceSessionTest, TabNodePoolEmpty);
   FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceSessionTest, TabNodePoolNonEmpty);
-  FRIEND_TEST_ALL_PREFIXES(ProfileSyncServiceSessionTest,
-                           ValidTabs);
   FRIEND_TEST_ALL_PREFIXES(SessionModelAssociatorTest, PopulateSessionHeader);
   FRIEND_TEST_ALL_PREFIXES(SessionModelAssociatorTest, PopulateSessionWindow);
   FRIEND_TEST_ALL_PREFIXES(SessionModelAssociatorTest, PopulateSessionTab);
@@ -400,15 +398,35 @@ class SessionModelAssociator
      const TabNavigation* navigation,
      sync_pb::TabNavigation* tab_navigation);
 
-  // Returns true if this tab belongs to this profile and belongs to a window,
-  // false otherwise.
-  bool IsValidTab(const SyncedTabDelegate& tab) const;
+  // Returns the session service from |sync_service_|.
+  SessionService* GetSessionService();
+
+  // Internal method used in the callback to obtain the current session.
+  // We don't own |windows|.
+  void OnGotSession(int handle, std::vector<SessionWindow*>* windows);
+
+  // Populate a session specifics header from a list of SessionWindows
+  void PopulateSessionSpecificsHeader(
+      const std::vector<SessionWindow*>& windows,
+      sync_pb::SessionHeader* header_s);
+
+  // Populates the window portion of the session specifics.
+  void PopulateSessionSpecificsWindow(const SessionWindow& window,
+                                      sync_pb::SessionWindow* session_window);
+
+  // Syncs all the tabs in |window| with the local sync db. Will allocate tab
+  // nodes if needed.
+  bool SyncLocalWindowToSyncModel(const SessionWindow& window);
+
+  // Fills a tab sync node with data from a SessionTab object.
+  // (from ReadCurrentSessions)
+  bool WriteSessionTabToSyncModel(const SessionTab& tab,
+                                  const int64 sync_id,
+                                  sync_api::WriteTransaction* trans);
 
-  // Having a valid entry is defined as the url being valid and and having a
-  // syncable scheme (non chrome:// and file:// url's). In other words, we don't
-  // want to sync a tab that is nothing but chrome:// and file:// navigations or
-  // invalid url's.
-  bool TabHasValidEntry(const SyncedTabDelegate& tab) const;
+  // Populates the tab portion of the session specifics.
+  void PopulateSessionSpecificsTab(const SessionTab& tab,
+                                   sync_pb::SessionTab* session_tab);
 
   // For testing only.
   void QuitLoopForSubtleTesting();
diff --git a/chrome/browser/sync/glue/session_model_associator_unittest.cc b/chrome/browser/sync/glue/session_model_associator_unittest.cc
index 33d6838..959d78d 100644
--- a/chrome/browser/sync/glue/session_model_associator_unittest.cc
+++ b/chrome/browser/sync/glue/session_model_associator_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -6,22 +6,12 @@
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
-#include "base/message_loop.h"
 #include "chrome/browser/sessions/session_types.h"
 #include "chrome/browser/sync/glue/session_model_associator.h"
-#include "chrome/browser/sync/glue/synced_tab_delegate.h"
-#include "chrome/browser/sync/protocol/session_specifics.pb.h"
 #include "chrome/common/url_constants.h"
-#include "content/public/browser/navigation_entry.h"
 #include "content/public/common/page_transition_types.h"
-#include "content/test/test_browser_thread.h"
-#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-using content::BrowserThread;
-using testing::NiceMock;
-using testing::Return;
-
 namespace browser_sync {
 
 typedef testing::Test SessionModelAssociatorTest;
@@ -41,9 +31,9 @@ TEST_F(SessionModelAssociatorTest, SessionWindowHasNoTabsToSync) {
   ASSERT_FALSE(SessionWindowHasNoTabsToSync(win));
 }
 
-TEST_F(SessionModelAssociatorTest, ShouldSyncSessionTab) {
+TEST_F(SessionModelAssociatorTest, IsValidSessionTab) {
   SessionTab tab;
-  ASSERT_FALSE(ShouldSyncSessionTab(tab));
+  ASSERT_FALSE(IsValidSessionTab(tab));
   TabNavigation nav(0, GURL(chrome::kChromeUINewTabURL),
                     content::Referrer(GURL("about:referrer"),
                                       WebKit::WebReferrerPolicyDefault),
@@ -51,7 +41,7 @@ TEST_F(SessionModelAssociatorTest, ShouldSyncSessionTab) {
                     std::string("state"), content::PageTransitionFromInt(0));
   tab.navigations.push_back(nav);
   // NewTab does not count as valid if it's the only navigation.
-  ASSERT_FALSE(ShouldSyncSessionTab(tab));
+  ASSERT_FALSE(IsValidSessionTab(tab));
   TabNavigation nav2(0, GURL("about:bubba"),
                      content::Referrer(GURL("about:referrer"),
                                        WebKit::WebReferrerPolicyDefault),
@@ -59,12 +49,12 @@ TEST_F(SessionModelAssociatorTest, ShouldSyncSessionTab) {
                     std::string("state"), content::PageTransitionFromInt(0));
   tab.navigations.push_back(nav2);
   // Once there's another navigation, the tab is valid.
-  ASSERT_TRUE(ShouldSyncSessionTab(tab));
+  ASSERT_TRUE(IsValidSessionTab(tab));
 }
 
-TEST_F(SessionModelAssociatorTest, ShouldSyncSessionTabIgnoresFragmentForNtp) {
+TEST_F(SessionModelAssociatorTest, IsValidSessionTabIgnoresFragmentForNtp) {
   SessionTab tab;
-  ASSERT_FALSE(ShouldSyncSessionTab(tab));
+  ASSERT_FALSE(IsValidSessionTab(tab));
   TabNavigation nav(0, GURL(std::string(chrome::kChromeUINewTabURL) +
                             "#bookmarks"),
                     content::Referrer(GURL("about:referrer"),
@@ -73,7 +63,7 @@ TEST_F(SessionModelAssociatorTest, ShouldSyncSessionTabIgnoresFragmentForNtp) {
                     std::string("state"), content::PageTransitionFromInt(0));
   tab.navigations.push_back(nav);
   // NewTab does not count as valid if it's the only navigation.
-  ASSERT_FALSE(ShouldSyncSessionTab(tab));
+  ASSERT_FALSE(IsValidSessionTab(tab));
 }
 
 TEST_F(SessionModelAssociatorTest, PopulateSessionHeader) {
@@ -187,71 +177,4 @@ TEST_F(SessionModelAssociatorTest, TabNodePool) {
   ASSERT_EQ(0U, pool.capacity());
 }
 
-namespace {
-
-class SyncedTabDelegateMock : public SyncedTabDelegate {
- public:
-  SyncedTabDelegateMock() {}
-  ~SyncedTabDelegateMock() {}
-
-  MOCK_CONST_METHOD0(GetWindowId, SessionID::id_type());
-  MOCK_CONST_METHOD0(GetSessionId, SessionID::id_type());
-  MOCK_CONST_METHOD0(IsBeingDestroyed, bool());
-  MOCK_CONST_METHOD0(profile, Profile*());
-  MOCK_CONST_METHOD0(HasExtensionAppId, bool());
-  MOCK_CONST_METHOD0(GetExtensionAppId, const std::string&());
-  MOCK_CONST_METHOD0(GetCurrentEntryIndex, int());
-  MOCK_CONST_METHOD0(GetEntryCount, int());
-  MOCK_CONST_METHOD0(GetPendingEntryIndex, int());
-  MOCK_CONST_METHOD0(GetPendingEntry, content::NavigationEntry*());
-  MOCK_CONST_METHOD1(GetEntryAtIndex, content::NavigationEntry*(int i));
-  MOCK_CONST_METHOD0(GetActiveEntry, content::NavigationEntry*());
-};
-
-}  // namespace.
-
-TEST_F(SessionModelAssociatorTest, ValidTabs) {
-  MessageLoopForUI message_loop;
-  content::TestBrowserThread ui_thread(BrowserThread::UI, &message_loop);
-  SessionModelAssociator model_associator(NULL, true);
-
-  NiceMock<SyncedTabDelegateMock> tab_mock;
-
-  // A null entry shouldn't crash.
-  EXPECT_CALL(tab_mock, GetCurrentEntryIndex()).WillRepeatedly(Return(0));
-  EXPECT_CALL(tab_mock, GetEntryAtIndex(0)).WillRepeatedly(
-      Return((content::NavigationEntry *)NULL));
-  EXPECT_CALL(tab_mock, GetEntryCount()).WillRepeatedly(Return(1));
-  EXPECT_CALL(tab_mock, GetPendingEntryIndex()).WillRepeatedly(Return(-1));
-  EXPECT_FALSE(model_associator.ShouldSyncTab(tab_mock));
-
-  // A chrome:// entry isn't valid.
-  content::NavigationEntry* entry = content::NavigationEntry::Create();
-  entry->SetVirtualURL(GURL("chrome://preferences/"));
-  EXPECT_FALSE(model_associator.ShouldSyncTab(tab_mock));
-
-  // A file:// entry isn't valid, even in addition to another entry.
-  content::NavigationEntry* entry2 = content::NavigationEntry::Create();
-  entry2->SetVirtualURL(GURL("file://bla"));
-  testing::Mock::VerifyAndClearExpectations(&tab_mock);
-  EXPECT_CALL(tab_mock, GetCurrentEntryIndex()).WillRepeatedly(Return(0));
-  EXPECT_CALL(tab_mock, GetEntryAtIndex(0)).WillRepeatedly(Return(entry));
-  EXPECT_CALL(tab_mock, GetEntryAtIndex(1)).WillRepeatedly(Return(entry2));
-  EXPECT_CALL(tab_mock, GetEntryCount()).WillRepeatedly(Return(2));
-  EXPECT_CALL(tab_mock, GetPendingEntryIndex()).WillRepeatedly(Return(-1));
-  EXPECT_FALSE(model_associator.ShouldSyncTab(tab_mock));
-
-  // Add a valid scheme entry to tab, making the tab valid.
-  content::NavigationEntry* entry3 = content::NavigationEntry::Create();
-  entry3->SetVirtualURL(GURL("http://www.google.com"));
-  testing::Mock::VerifyAndClearExpectations(&tab_mock);
-  EXPECT_CALL(tab_mock, GetCurrentEntryIndex()).WillRepeatedly(Return(0));
-  EXPECT_CALL(tab_mock, GetEntryAtIndex(0)).WillRepeatedly(Return(entry));
-  EXPECT_CALL(tab_mock, GetEntryAtIndex(1)).WillRepeatedly(Return(entry2));
-  EXPECT_CALL(tab_mock, GetEntryAtIndex(2)).WillRepeatedly(Return(entry3));
-  EXPECT_CALL(tab_mock, GetEntryCount()).WillRepeatedly(Return(3));
-  EXPECT_CALL(tab_mock, GetPendingEntryIndex()).WillRepeatedly(Return(-1));
-  EXPECT_TRUE(model_associator.ShouldSyncTab(tab_mock));
-}
-
 }  // namespace browser_sync
diff --git a/chrome/browser/sync/glue/synced_session.cc b/chrome/browser/sync/glue/synced_session.cc
index c25342a..5cfb423 100644
--- a/chrome/browser/sync/glue/synced_session.cc
+++ b/chrome/browser/sync/glue/synced_session.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -18,20 +18,24 @@ SyncedSession::~SyncedSession() {
   STLDeleteContainerPairSecondPointers(windows.begin(), windows.end());
 }
 
-// Note: if you modify this, make sure you modify
-// SessionModelAssociator::ShouldSyncTab to ensure the logic matches.
-bool ShouldSyncSessionTab(const SessionTab& tab) {
+// Note: if you modify this, make sure you modify IsValidTab in
+// SessionModelAssociator.
+bool IsValidSessionTab(const SessionTab& tab) {
   if (tab.navigations.empty())
     return false;
-  bool found_valid_url = false;
-  for (size_t i = 0; i < tab.navigations.size(); ++i) {
-    if (tab.navigations.at(i).virtual_url().is_valid() &&
-        !tab.navigations.at(i).virtual_url().SchemeIs("chrome") &&
-        !tab.navigations.at(i).virtual_url().SchemeIsFile()) {
-      found_valid_url = true;
-    }
+  int selected_index = tab.current_navigation_index;
+  selected_index = std::max(
+      0,
+      std::min(selected_index,
+          static_cast<int>(tab.navigations.size() - 1)));
+  if (selected_index == 0 &&
+      tab.navigations.size() == 1 &&
+      tab.navigations.at(selected_index).virtual_url().GetOrigin() ==
+          GURL(chrome::kChromeUINewTabURL)) {
+    // This is a new tab with no further history, skip.
+    return false;
   }
-  return found_valid_url;
+  return true;
 }
 
 bool SessionWindowHasNoTabsToSync(const SessionWindow& window) {
@@ -39,7 +43,7 @@ bool SessionWindowHasNoTabsToSync(const SessionWindow& window) {
   for (std::vector<SessionTab*>::const_iterator i = window.tabs.begin();
       i != window.tabs.end(); ++i) {
     const SessionTab* tab = *i;
-    if (ShouldSyncSessionTab(*tab))
+    if (IsValidSessionTab(*tab))
       num_populated++;
   }
   return (num_populated == 0);
diff --git a/chrome/browser/sync/glue/synced_session.h b/chrome/browser/sync/glue/synced_session.h
index c4037dd..7b00b8f 100644
--- a/chrome/browser/sync/glue/synced_session.h
+++ b/chrome/browser/sync/glue/synced_session.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -56,9 +56,9 @@ struct SyncedSession {
 };
 
 // Control which foreign tabs we're interested in syncing/displaying. Checks
-// that the tab has navigations and contains at least one valid url.
-// Note: chrome:// and file:// are not considered valid urls (for syncing).
-bool ShouldSyncSessionTab(const SessionTab& tab);
+// that the tab has navigations and is not a new tab (url == NTP).
+// Note: A new tab page with back/forward history is valid.
+bool IsValidSessionTab(const SessionTab& tab);
 
 // Checks whether the window has tabs to sync. If no tabs to sync, it returns
 // true, false otherwise.
diff --git a/chrome/browser/sync/glue/synced_session_tracker_unittest.cc b/chrome/browser/sync/glue/synced_session_tracker_unittest.cc
index 6a904ad..45dcaad 100644
--- a/chrome/browser/sync/glue/synced_session_tracker_unittest.cc
+++ b/chrome/browser/sync/glue/synced_session_tracker_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -63,8 +63,9 @@ TEST_F(SyncedSessionTrackerTest, LookupAllForeignSessions) {
   SessionTab* tab = tracker.GetTab("tag1", 15);
   ASSERT_TRUE(tab);
   tab->navigations.push_back(TabNavigation(
-      0, GURL("bla://valid_url"), content::Referrer(GURL("bla://referrer"),
-      WebKit::WebReferrerPolicyDefault), string16(ASCIIToUTF16("title")),
+      0, GURL("valid_url"),
+      content::Referrer(GURL("referrer"), WebKit::WebReferrerPolicyDefault),
+      string16(ASCIIToUTF16("title")),
       std::string("state"), content::PageTransitionFromInt(0)));
   ASSERT_TRUE(tracker.LookupAllForeignSessions(&sessions));
   // Only the session with a valid window and tab gets returned.
diff --git a/chrome/browser/sync/profile_sync_service_session_unittest.cc b/chrome/browser/sync/profile_sync_service_session_unittest.cc
index d858448..abc6e2f 100644
--- a/chrome/browser/sync/profile_sync_service_session_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_session_unittest.cc
@@ -15,6 +15,11 @@
 #include "base/scoped_temp_dir.h"
 #include "base/stl_util.h"
 #include "base/time.h"
+#include "chrome/browser/sessions/session_service.h"
+#include "chrome/browser/sessions/session_service_factory.h"
+#include "chrome/browser/sessions/session_service_test_helper.h"
+#include "chrome/browser/signin/signin_manager.h"
+#include "chrome/browser/signin/signin_manager_factory.h"
 #include "chrome/browser/sync/abstract_profile_sync_service_test.h"
 #include "chrome/browser/sync/glue/session_change_processor.h"
 #include "chrome/browser/sync/glue/session_data_type_controller.h"
@@ -163,12 +168,20 @@ class ProfileSyncServiceSessionTest
   TestIdFactory* ids() { return sync_service_->id_factory(); }
 
  protected:
+  SessionService* service() { return helper_.service(); }
+
   virtual void SetUp() {
     // BrowserWithTestWindowTest implementation.
     BrowserWithTestWindowTest::SetUp();
     io_thread_.StartIOThread();
     profile()->CreateRequestContext();
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    SessionService* session_service = new SessionService(temp_dir_.path());
+    helper_.set_service(session_service);
+    service()->SetWindowType(window_id_, Browser::TYPE_TABBED);
+    service()->SetWindowBounds(window_id_,
+                               window_bounds_,
+                               ui::SHOW_STATE_NORMAL);
     registrar_.Add(this, chrome::NOTIFICATION_FOREIGN_SESSION_UPDATED,
         content::NotificationService::AllSources());
   }
@@ -187,6 +200,11 @@ class ProfileSyncServiceSessionTest
   }
 
   virtual void TearDown() {
+    if (SessionServiceFactory::GetForProfileIfExisting(profile()) == service())
+      helper_.ReleaseService(); // we transferred ownership to profile
+    else
+      helper_.set_service(NULL);
+    SessionServiceFactory::SetForTestProfile(profile(), NULL);
     sync_service_.reset();
     profile()->ResetRequestContext();
 
@@ -217,6 +235,7 @@ class ProfileSyncServiceSessionTest
         ProfileSyncService::AUTO_START,
         false,
         callback));
+    SessionServiceFactory::SetForTestProfile(profile(), helper_.service());
 
     // Register the session data type.
     model_associator_ =
@@ -244,6 +263,7 @@ class ProfileSyncServiceSessionTest
   content::TestBrowserThread io_thread_;
   // Path used in testing.
   ScopedTempDir temp_dir_;
+  SessionServiceTestHelper helper_;
   SessionModelAssociator* model_associator_;
   SessionChangeProcessor* change_processor_;
   SessionID window_id_;
@@ -283,6 +303,7 @@ TEST_F(ProfileSyncServiceSessionTest, WriteSessionToNode) {
   CreateRootHelper create_root(this);
   ASSERT_TRUE(StartSyncService(create_root.callback(), false));
   ASSERT_TRUE(create_root.success());
+  ASSERT_EQ(model_associator_->GetSessionService(), helper_.service());
 
   // Check that the DataTypeController associated the models.
   bool has_nodes;
diff --git a/chrome/browser/sync/test/integration/multiple_client_sessions_sync_test.cc b/chrome/browser/sync/test/integration/multiple_client_sessions_sync_test.cc
index c6396e3..31f08d2 100644
--- a/chrome/browser/sync/test/integration/multiple_client_sessions_sync_test.cc
+++ b/chrome/browser/sync/test/integration/multiple_client_sessions_sync_test.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -46,7 +46,7 @@ IN_PROC_BROWSER_TEST_F(MultipleClientSessionsSyncTest, MAYBE_AllChanged) {
   for (int i = 0; i < num_clients(); ++i) {
     SessionWindowMap windows;
     ASSERT_TRUE(OpenTabAndGetLocalWindows(
-        i, GURL(StringPrintf("http://127.0.0.1/bubba%i", i)), &windows));
+        i, GURL(StringPrintf("about:bubba%i", i)), &windows));
     client_windows[i].Reset(&windows);
   }
 
@@ -82,7 +82,7 @@ IN_PROC_BROWSER_TEST_F(MultipleClientSessionsSyncTest,
   for (int i = 0; i < num_clients(); ++i) {
     SessionWindowMap windows;
     ASSERT_TRUE(OpenTabAndGetLocalWindows(
-        i, GURL(StringPrintf("http://127.0.0.1/bubba%i", i)), &windows));
+        i, GURL(StringPrintf("about:bubba%i", i)), &windows));
     client_windows[i].Reset(&windows);
   }
 
diff --git a/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc b/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
index f2bddd0..3e0aedb 100644
--- a/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
+++ b/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -39,7 +39,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientSessionsSyncTest, MAYBE_Sanity) {
 
   ScopedWindowMap old_windows;
   ASSERT_TRUE(OpenTabAndGetLocalWindows(0,
-                                        GURL("http://127.0.0.1/bubba"),
+                                        GURL("about:bubba"),
                                         old_windows.GetMutable()));
 
   ASSERT_TRUE(GetClient(0)->AwaitFullSyncCompletion(
diff --git a/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc b/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
index 841885d..2b762e4 100644
--- a/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
+++ b/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -28,8 +28,8 @@ class TwoClientSessionsSyncTest : public SyncTest {
 };
 
 static const char* kValidPassphrase = "passphrase!";
-static const char* kURL1 = "http://127.0.0.1/bubba1";
-static const char* kURL2 = "http://127.0.0.1/bubba2";
+static const char* kURL1 = "chrome://sync";
+static const char* kURL2 = "chrome://version";
 
 // TODO(zea): Test each individual session command we care about separately.
 // (as well as multi-window). We're currently only checking basic single-window/