[Sync] Componentize synced_tab_delegate

chrome/browser/sync/sessions/sessions_sync_manager.cc

Index: chrome/browser/sync/sessions/sessions_sync_manager.cc
diff --git a/chrome/browser/sync/sessions/sessions_sync_manager.cc b/chrome/browser/sync/sessions/sessions_sync_manager.cc
index 3aeaa772d084d8b672faf0d9c6f04e79bd9c31cf..f99386a3162e12333e50aa9db4d2e901c802eb22 100644
--- a/chrome/browser/sync/sessions/sessions_sync_manager.cc
+++ b/chrome/browser/sync/sessions/sessions_sync_manager.cc
@@ -10,25 +10,19 @@
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/search/search.h"
-#include "chrome/browser/sync/glue/synced_tab_delegate.h"
-#include "chrome/common/url_constants.h"
-#include "components/sessions/content/content_serialized_navigation_builder.h"
 #include "components/sync_driver/glue/synced_window_delegate.h"
 #include "components/sync_driver/local_device_info_provider.h"
 #include "components/sync_driver/sessions/synced_window_delegates_getter.h"
-#include "content/public/browser/favicon_status.h"
-#include "content/public/browser/navigation_entry.h"
+#include "components/sync_sessions/sync_sessions_client.h"
+#include "components/sync_sessions/synced_tab_delegate.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/common/url_constants.h"
 #include "sync/api/sync_error.h"
 #include "sync/api/sync_error_factory.h"
 #include "sync/api/sync_merge_result.h"
 #include "sync/api/time.h"
 
-using content::NavigationEntry;
-using sessions::ContentSerializedNavigationBuilder;
 using sessions::SerializedNavigationEntry;
 using sync_driver::DeviceInfo;
 using sync_driver::LocalDeviceInfoProvider;
@@ -68,16 +62,45 @@ bool SessionsRecencyComparator(const sync_driver::SyncedSession* s1,
   return s1->modified_time > s2->modified_time;
 }
 
+// Helper to check if this is a valid tab that should be synced.
+bool ShouldSyncTab(sync_sessions::SyncSessionsClient* sessions_client,
+                   SyncedTabDelegate* tab_delegate) {
+  if (tab_delegate->GetSyncedWindowDelegate() == nullptr)
+    return false;
+
+  // Is there a valid NavigationEntry?
+  if (tab_delegate->ProfileIsSupervised() &&
+      tab_delegate->GetBlockedNavigations()->size() > 0)
+    return true;
+
+  if (tab_delegate->IsInitialBlankNavigation())
+    return false;  // This deliberately ignores a new pending entry.
+
+  int entry_count = tab_delegate->GetEntryCount();
+  for (int i = 0; i < entry_count; ++i) {
+    const GURL& virtual_url = tab_delegate->GetVirtualURLAtIndex(i);
+    if (!virtual_url.is_valid())
+      continue;
+
+    if (sessions_client->ShouldSyncURL(virtual_url))
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 // |local_device| is owned by ProfileSyncService, its lifetime exceeds
 // lifetime of SessionSyncManager.
 SessionsSyncManager::SessionsSyncManager(
+    sync_sessions::SyncSessionsClient* sessions_client,
     Profile* profile,
     LocalDeviceInfoProvider* local_device,
     scoped_ptr<LocalSessionEventRouter> router,
     scoped_ptr<SyncedWindowDelegatesGetter> synced_window_getter)
-    : favicon_cache_(FaviconServiceFactory::GetForProfile(
+    : sessions_client_(sessions_client),
+      session_tracker_(sessions_client),
+      favicon_cache_(FaviconServiceFactory::GetForProfile(
                          profile,
                          ServiceAccessType::EXPLICIT_ACCESS),
                      HistoryServiceFactory::GetForProfile(
@@ -92,7 +115,7 @@ SessionsSyncManager::SessionsSyncManager(
       stale_session_threshold_days_(kDefaultStaleSessionThresholdDays),
       local_event_router_(router.Pass()),
       synced_window_getter_(synced_window_getter.Pass()),
-      page_revisit_broadcaster_(this, profile) {}
+      page_revisit_broadcaster_(this, sessions_client, profile) {}
 
 LocalSessionEventRouter::~LocalSessionEventRouter() {}
 
@@ -297,8 +320,6 @@ void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab,
                                        syncer::SyncChangeList* change_output) {
   DCHECK(tab->HasWebContents());
   SessionID::id_type tab_id = tab->GetSessionId();
-  if (tab->profile() != profile_)
-    return;
 
   if (tab->IsBeingDestroyed()) {
     // This tab is closing.
@@ -313,7 +334,7 @@ void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab,
     return;
   }
 
-  if (!tab->ShouldSync())
+  if (!ShouldSyncTab(sessions_client_, tab))
     return;
 
   TabLinksMap::iterator local_tab_map_iter = local_tab_map_.find(tab_id);
@@ -353,12 +374,14 @@ void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab,
   change_output->push_back(syncer::SyncChange(
       FROM_HERE, syncer::SyncChange::ACTION_UPDATE, data));
 
-  const GURL new_url = GetCurrentVirtualURL(*tab);
+  int current_index = tab->GetCurrentEntryIndex();
+  const GURL new_url = tab->GetVirtualURLAtIndex(current_index);
   if (new_url != tab_link->url()) {
     tab_link->set_url(new_url);
-    favicon_cache_.OnFaviconVisited(new_url, GetCurrentFaviconURL(*tab));
+    favicon_cache_.OnFaviconVisited(new_url,
+                                    tab->GetFaviconURLAtIndex(current_index));
     page_revisit_broadcaster_.OnPageVisit(
-        new_url, tab->GetCurrentEntryMaybePending()->GetTransitionType());
+        new_url, tab->GetTransitionAtIndex(current_index));
   }
 
   session_tracker_.GetSession(current_machine_tag())->modified_time =
@@ -394,12 +417,14 @@ bool SessionsSyncManager::IsValidSessionHeader(
   return true;
 }
 
-void SessionsSyncManager::OnLocalTabModified(SyncedTabDelegate* modified_tab) {
-  const content::NavigationEntry* entry = modified_tab->GetActiveEntry();
-  if (!modified_tab->IsBeingDestroyed() &&
-      entry &&
-      entry->GetVirtualURL().is_valid() &&
-      entry->GetVirtualURL().spec() == kNTPOpenTabSyncURL) {
+bool SessionsSyncManager::OnLocalTabModified(SyncedTabDelegate* modified_tab) {
+  if (!ShouldSyncTab(sessions_client_, modified_tab))

[skym, 2015/10/16 16:55:47]

Won't this miss things like if you have a tab

www.google.com -> chrome://1 -> chrome://2 -> chrome://3 -> chrome://4 ->
chrome://5

As you transition to the next 'uninteresting' entry, say chrome://6, and the
last entry rolls off, foreign clients should remove this tab, but instead the
behavior will be it kind of freezes/gets stuck. Admittedly, this case probably
isn't very important.

I think the existing code had a similar problem, it wasn't flaring in this case.
But at least it was still processing the change.

[Nicolas Zea, 2015/10/20 23:14:42]

Good catch! You're right, previously we'd have removed the tab from the header
node, but this bypasses the AssociateWindows call. Fixed, and added a test case
for it.

[skym, 2015/10/21 00:38:36]

I'm a little sad to see this optimization go away, but it does keep things
easier/simpler. I think when we convert this to USS we're going to have to solve
this problem, since the directory won't be doing bit-wise diffs for us anymore.

+    return false;
+
+  GURL virtual_url =
+      modified_tab->GetVirtualURLAtIndex(modified_tab->GetCurrentEntryIndex());
+  if (!modified_tab->IsBeingDestroyed() && virtual_url.is_valid() &&
+      virtual_url.spec() == kNTPOpenTabSyncURL) {
     DVLOG(1) << "Triggering sync refresh for sessions datatype.";
     const syncer::ModelTypeSet types(syncer::SESSIONS);
     content::NotificationService::current()->Notify(
@@ -413,7 +438,7 @@ void SessionsSyncManager::OnLocalTabModified(SyncedTabDelegate* modified_tab) {
     // fix things up.  This takes care of the new tab modification as well.
     RebuildAssociations();
     DCHECK(!local_tab_pool_out_of_sync_);
-    return;
+    return true;
   }
 
   syncer::SyncChangeList changes;
@@ -424,6 +449,7 @@ void SessionsSyncManager::OnLocalTabModified(SyncedTabDelegate* modified_tab) {
   // to the window's tab information.
   AssociateWindows(DONT_RELOAD_TABS, syncer::SyncDataList(), &changes);
   sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
+  return true;
 }
 
 void SessionsSyncManager::OnFaviconsChanged(
@@ -884,24 +910,6 @@ bool SessionsSyncManager::DisassociateForeignSession(
   return session_tracker_.DeleteSession(foreign_session_tag);
 }
 
-// static
-GURL SessionsSyncManager::GetCurrentVirtualURL(
-    const SyncedTabDelegate& tab_delegate) {
-  const NavigationEntry* current_entry =
-      tab_delegate.GetCurrentEntryMaybePending();
-  return current_entry->GetVirtualURL();
-}
-
-// static
-GURL SessionsSyncManager::GetCurrentFaviconURL(
-    const SyncedTabDelegate& tab_delegate) {
-  const NavigationEntry* current_entry =
-      tab_delegate.GetCurrentEntryMaybePending();
-  return (current_entry->GetFavicon().valid ?
-          current_entry->GetFavicon().url :
-          GURL());
-}
-
 bool SessionsSyncManager::GetForeignSession(
     const std::string& tag,
     std::vector<const sessions::SessionWindow*>* windows) {
@@ -1036,17 +1044,16 @@ void SessionsSyncManager::SetSessionTabFromDelegate(
   session_tab->navigations.clear();
 
   for (int i = min_index; i < max_index; ++i) {
-    const NavigationEntry* entry = tab_delegate.GetEntryAtIndexMaybePending(i);
-    DCHECK(entry);
-    if (!entry->GetVirtualURL().is_valid())
+    if (!tab_delegate.GetVirtualURLAtIndex(i).is_valid())
       continue;
+    sessions::SerializedNavigationEntry serialized_entry;
+    tab_delegate.GetSerializedNavigationAtIndex(i, &serialized_entry);

[pavely, 2015/10/16 21:55:20]

Nit: It would make sense to move obtaining serialized_entry 4 lines down to
where it is pushed into navigations.

 
     // Set current_navigation_index to the index in navigations.
     if (i == current_index)
       session_tab->current_navigation_index = session_tab->navigations.size();
 
-    session_tab->navigations.push_back(
-        ContentSerializedNavigationBuilder::FromNavigationEntry(i, *entry));
+    session_tab->navigations.push_back(serialized_entry);
     if (is_supervised) {
       session_tab->navigations.back().set_blocked_state(
           SerializedNavigationEntry::STATE_ALLOWED);
@@ -1061,13 +1068,12 @@ void SessionsSyncManager::SetSessionTabFromDelegate(
   }
 
   if (is_supervised) {
-    const std::vector<const NavigationEntry*>& blocked_navigations =
-        *tab_delegate.GetBlockedNavigations();
     int offset = session_tab->navigations.size();
+    const std::vector<const SerializedNavigationEntry*>& blocked_navigations =
+        *tab_delegate.GetBlockedNavigations();
     for (size_t i = 0; i < blocked_navigations.size(); ++i) {
-      session_tab->navigations.push_back(
-          ContentSerializedNavigationBuilder::FromNavigationEntry(
-              i + offset, *blocked_navigations[i]));
+      session_tab->navigations.push_back(*blocked_navigations[i]);
+      session_tab->navigations.back().set_index(offset + i);
       session_tab->navigations.back().set_blocked_state(
           SerializedNavigationEntry::STATE_BLOCKED);
       // TODO(bauerb): Add categories