sync: Route local sessions events to SessionsSyncManager

chrome/browser/sync/sessions2/sessions_sync_manager.cc

Index: chrome/browser/sync/sessions2/sessions_sync_manager.cc
diff --git a/chrome/browser/sync/sessions2/sessions_sync_manager.cc b/chrome/browser/sync/sessions2/sessions_sync_manager.cc
index 66ae4d31bb6ad92e29ed83a1fd462a9bc77e3050..93e5128c5276b49bf26d1351343cb229e206b797 100644
--- a/chrome/browser/sync/sessions2/sessions_sync_manager.cc
+++ b/chrome/browser/sync/sessions2/sessions_sync_manager.cc
@@ -37,14 +37,20 @@ static const int kMaxSyncFavicons = 200;
 // The maximum number of navigations in each direction we care to sync.
 static const int kMaxSyncNavigationCount = 6;
 
+// The URL at which the set of synced tabs is displayed. We treat it differently
+// from all other URL's as accessing it triggers a sync refresh of Sessions.
+static const char kNTPOpenTabSyncURL[] = "chrome://newtab/#open_tabs";
+
 SessionsSyncManager::SessionsSyncManager(
     Profile* profile,
-    SyncInternalApiDelegate* delegate)
+    SyncInternalApiDelegate* delegate,
+    scoped_ptr<LocalEventRouter> router)
     : favicon_cache_(profile, kMaxSyncFavicons),
       sync_prefs_(profile->GetPrefs()),
       profile_(profile),
       delegate_(delegate),
-      local_session_header_node_id_(TabNodePool2::kInvalidTabNodeID) {
+      local_session_header_node_id_(TabNodePool2::kInvalidTabNodeID),
+      local_event_router_(router.Pass()) {
 }
 
 SessionsSyncManager::~SessionsSyncManager() {
@@ -116,6 +122,8 @@ syncer::SyncMergeResult SessionsSyncManager::MergeDataAndStartSyncing(
 
   merge_result.set_error(
       sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes));
+
+  local_event_router_->StartRoutingTo(this);
   return merge_result;
 }
 
@@ -293,13 +301,61 @@ void SessionsSyncManager::AssociateTab(SyncedTabDelegate* const tab,
       base::Time::Now();
 }
 
-void SessionsSyncManager::OnLocalTabModified(
-    const SyncedTabDelegate& modified_tab, syncer::SyncError* error) {
-  NOTIMPLEMENTED() << "TODO(tim): SessionModelAssociator::Observe equivalent.";
+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) {
+    DVLOG(1) << "Triggering sync refresh for sessions datatype.";
+    const syncer::ModelTypeSet types(syncer::SESSIONS);
+    content::NotificationService::current()->Notify(
+        chrome::NOTIFICATION_SYNC_REFRESH_LOCAL,
+        content::Source<Profile>(profile_),
+        content::Details<const syncer::ModelTypeSet>(&types));
+  }
+
+  syncer::SyncChangeList changes;
+  // Associate tabs first so the synced session tracker is aware of them.
+  AssociateTab(modified_tab, &changes);
+  // Note, we always associate windows because it's possible a tab became
+  // "interesting" by going to a valid URL, in which case it needs to be added
+  // to the window's tab information.
+  AssociateWindows(DONT_RELOAD_TABS, &changes);
+
+  syncer::SyncError error =
+      sync_processor_->ProcessSyncChanges(FROM_HERE, changes);
+  if (error.IsSet()) {

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

not necessarily for this patch, but I suspect triggering the re-association
shouldn't be based on the SyncChangeProcessor returning an error (currently, if
it does that, it'll also disable the datatype).

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

Hm. Right. Well, in the old code, we'd re-associate if any of the Read/WriteNode
operations failed to look things up.

I'm not sure what we want to do here in this case, if anything, then. 

For the delete case in 440, we need to reset TabNodePool, but at the same time I
don't want to thrash with another client as the comment suggests..

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

Yeah, the old version was kind of violating some layering in order to get that
information. The right approach is probably to, on delete, set a flag so that on
the next local change we do a GetAllSyncData + Reassociate.

+    // Note that if we fail to associate, it means something has gone wrong,
+    // such as our local session being deleted, so we disassociate and
+    // associate again.
+    NOTIMPLEMENTED() << "TODO(tim): Bug 98892.  Full re-association.";
+  }
 }
 
 void SessionsSyncManager::OnBrowserOpened() {
-  NOTIMPLEMENTED() << "TODO(tim): SessionModelAssociator::Observe equivalent.";
+  syncer::SyncChangeList changes;
+  AssociateWindows(DONT_RELOAD_TABS, &changes);
+  syncer::SyncError error =
+      sync_processor_->ProcessSyncChanges(FROM_HERE, changes);

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

nit: fits on one line?

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

nope

+  if (error.IsSet()) {

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

nit: remove curly brace

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

I'm with you, although as a rule of thumb we try to keep curly braces around
macros to quarantine the damage they might do.  

In this case though and the one above, maybe I should just remove the
NOTIMPLEMENTED()? I'm not sure what I'd do in this case.

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

I think we can probably ignore the error result. Sync's going to disable us
anyways.

+    NOTIMPLEMENTED() << "TODO(tim): Bug 98892.  Full re-association.";
+  }
+}
+
+void SessionsSyncManager::OnFaviconPageUrlsUpdated(
+    const std::set<GURL>& updated_favicon_page_urls) {
+  // TODO(zea): consider a separate container for tabs with outstanding favicon
+  // loads so we don't have to iterate through all tabs comparing urls.
+  for (std::set<GURL>::const_iterator i = updated_favicon_page_urls.begin();
+       i != updated_favicon_page_urls.end(); ++i) {
+    for (TabLinksMap::iterator tab_iter = local_tab_map_.begin();
+         tab_iter != local_tab_map_.end();
+         ++tab_iter) {
+      if (tab_iter->second->url() == *i)
+        favicon_cache_.OnPageFaviconUpdated(*i);
+    }
+  }
 }
 
 bool SessionsSyncManager::ShouldSyncTab(const SyncedTabDelegate& tab) const {
@@ -345,21 +401,6 @@ bool SessionsSyncManager::ShouldSyncWindow(
   return window->IsTypeTabbed() || window->IsTypePopup();
 }
 
-void SessionsSyncManager::ForwardRelevantFaviconUpdatesToFaviconCache(
-    const std::set<GURL>& updated_favicon_page_urls) {
-  // TODO(zea): consider a separate container for tabs with outstanding favicon
-  // loads so we don't have to iterate through all tabs comparing urls.
-  for (std::set<GURL>::const_iterator i = updated_favicon_page_urls.begin();
-       i != updated_favicon_page_urls.end(); ++i) {
-    for (TabLinksMap::iterator tab_iter = local_tab_map_.begin();
-         tab_iter != local_tab_map_.end();
-         ++tab_iter) {
-      if (tab_iter->second->url() == *i)
-        favicon_cache_.OnPageFaviconUpdated(*i);
-    }
-  }
-}
-
 void SessionsSyncManager::StopSyncing(syncer::ModelType type) {
   NOTIMPLEMENTED();
 }
@@ -396,6 +437,8 @@ syncer::SyncError SessionsSyncManager::ProcessSyncChanges(
           // Another client has attempted to delete our local data (possibly by
           // error or a clock is inaccurate). Just ignore the deletion for now
           // to avoid any possible ping-pong delete/reassociate sequence.
+          // TODO(tim): Bug 98892.  This corrupts TabNodePool. Perform full
+          // re-association.
           LOG(WARNING) << "Local session data deleted. Ignoring until next "
                        << "local navigation event.";
         } else if (session.has_header()) {
@@ -868,7 +911,6 @@ void SessionsSyncManager::SetSessionTabFromDelegate(
   session_tab->session_storage_persistent_id.clear();
 }
 
-
 FaviconCache* SessionsSyncManager::GetFaviconCache() {
   return &favicon_cache_;
 }